diff mbox series

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

Message ID 20201112221059.68098-1-martin@martin.st
State Accepted
Commit 104e5233f03f1ed3aa34b256cd2b9023a9076ffb
Headers show
Series [FFmpeg-devel] http: Check for AVERROR_EOF in the check for premature end
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Martin Storsjö Nov. 12, 2020, 10:10 p.m. UTC
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(-)

Comments

Martin Storsjö Nov. 17, 2020, 9 a.m. UTC | #1
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
Jan Ekström Nov. 20, 2020, 12:15 a.m. UTC | #2
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
Martin Storsjö Nov. 20, 2020, 8:13 a.m. UTC | #3
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
diff mbox series

Patch

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