[FFmpeg-devel] http: Check for AVERROR_EOF in the check for premature end

When the check was added (in 3668701f9600, in 2015), some IO
functions returned 0 on EOF (in particular, the TCP protocol
did, but the TLS protocol returned AVERROR_EOF). Since
0e1f771d2200d in 2017, the TCP protocol also returns AVERROR_EOF
instead of 0, making the check for premature end never have the
intended effect.
---
 libavformat/http.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

On Fri, 13 Nov 2020, Martin Storsjö wrote:

> When the check was added (in 3668701f9600, in 2015), some IO
> functions returned 0 on EOF (in particular, the TCP protocol
> did, but the TLS protocol returned AVERROR_EOF). Since
> 0e1f771d2200d in 2017, the TCP protocol also returns AVERROR_EOF
> instead of 0, making the check for premature end never have the
> intended effect.
> ---
> libavformat/http.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/libavformat/http.c b/libavformat/http.c
> index 3d25d652d3..2d24c00e18 100644
> --- a/libavformat/http.c
> +++ b/libavformat/http.c
> @@ -1436,7 +1436,8 @@ static int http_buf_read(URLContext *h, uint8_t *buf, int size)
>         if ((!s->willclose || s->chunksize == UINT64_MAX) && s->off >= target_end)
>             return AVERROR_EOF;
>         len = ffurl_read(s->hd, buf, size);
> -        if (!len && (!s->willclose || s->chunksize == UINT64_MAX) && s->off < target_end) {
> +        if ((!len || len == AVERROR_EOF) &&
> +            (!s->willclose || s->chunksize == UINT64_MAX) && s->off < target_end) {
>             av_log(h, AV_LOG_ERROR,
>                    "Stream ends prematurely at %"PRIu64", should be %"PRIu64"\n",
>                    s->off, target_end
> -- 
> 2.24.3 (Apple Git-128)

Ping?

// Martin

On Fri, Nov 13, 2020 at 12:17 AM Martin Storsjö <martin@martin.st> wrote:
>
> When the check was added (in 3668701f9600, in 2015), some IO
> functions returned 0 on EOF (in particular, the TCP protocol
> did, but the TLS protocol returned AVERROR_EOF). Since
> 0e1f771d2200d in 2017, the TCP protocol also returns AVERROR_EOF
> instead of 0, making the check for premature end never have the
> intended effect.
> ---
>  libavformat/http.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/libavformat/http.c b/libavformat/http.c
> index 3d25d652d3..2d24c00e18 100644
> --- a/libavformat/http.c
> +++ b/libavformat/http.c
> @@ -1436,7 +1436,8 @@ static int http_buf_read(URLContext *h, uint8_t *buf, int size)
>          if ((!s->willclose || s->chunksize == UINT64_MAX) && s->off >= target_end)
>              return AVERROR_EOF;
>          len = ffurl_read(s->hd, buf, size);
> -        if (!len && (!s->willclose || s->chunksize == UINT64_MAX) && s->off < target_end) {
> +        if ((!len || len == AVERROR_EOF) &&
> +            (!s->willclose || s->chunksize == UINT64_MAX) && s->off < target_end) {
>              av_log(h, AV_LOG_ERROR,
>                     "Stream ends prematurely at %"PRIu64", should be %"PRIu64"\n",
>                     s->off, target_end
> --
> 2.24.3 (Apple Git-128)

Change-wise should be OK. Empty or EOF with not enough read leading to
AVERROR(EIO), former being already current behavior.

That said, if this error case always was only regarding EOF use cases
we might want to check the case in ticket 4039 - if simplifying the
check to EOF only still has it work as expected. But that I think is a
separate check/change.

Jan

On Fri, 20 Nov 2020, Jan Ekström wrote:

> On Fri, Nov 13, 2020 at 12:17 AM Martin Storsjö <martin@martin.st> wrote:
>>
>> When the check was added (in 3668701f9600, in 2015), some IO
>> functions returned 0 on EOF (in particular, the TCP protocol
>> did, but the TLS protocol returned AVERROR_EOF). Since
>> 0e1f771d2200d in 2017, the TCP protocol also returns AVERROR_EOF
>> instead of 0, making the check for premature end never have the
>> intended effect.
>> ---
>>  libavformat/http.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavformat/http.c b/libavformat/http.c
>> index 3d25d652d3..2d24c00e18 100644
>> --- a/libavformat/http.c
>> +++ b/libavformat/http.c
>> @@ -1436,7 +1436,8 @@ static int http_buf_read(URLContext *h, uint8_t *buf, int size)
>>          if ((!s->willclose || s->chunksize == UINT64_MAX) && s->off >= target_end)
>>              return AVERROR_EOF;
>>          len = ffurl_read(s->hd, buf, size);
>> -        if (!len && (!s->willclose || s->chunksize == UINT64_MAX) && s->off < target_end) {
>> +        if ((!len || len == AVERROR_EOF) &&
>> +            (!s->willclose || s->chunksize == UINT64_MAX) && s->off < target_end) {
>>              av_log(h, AV_LOG_ERROR,
>>                     "Stream ends prematurely at %"PRIu64", should be %"PRIu64"\n",
>>                     s->off, target_end
>> --
>> 2.24.3 (Apple Git-128)
>
> Change-wise should be OK. Empty or EOF with not enough read leading to
> AVERROR(EIO), former being already current behavior.
>
> That said, if this error case always was only regarding EOF use cases
> we might want to check the case in ticket 4039 - if simplifying the
> check to EOF only still has it work as expected. But that I think is a
> separate check/change.

Thanks for having a look, will push this one soon then.

Yeah, removing the check for !len might be fine when/if 
FF_API_OLD_AVIO_EOF_0 gets disabled.

// Martin