[FFmpeg-devel] lavf/mov: Ignore avio_skip() return value

Hi!

Attached patch fixes ticket #6102.

Please comment, Carl Eugen
From 694daed9222e50d6245bf5d041e82523ee869451 Mon Sep 17 00:00:00 2001
From: Carl Eugen Hoyos <cehoyos@ag.or.at>
Date: Thu, 26 Jan 2017 00:32:23 +0100
Subject: [PATCH] lavf/mov: Ignore avio_skip() return value.

Fixes ticket #6102.
---
 libavformat/mov.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

On Thu, 26 Jan 2017 00:34:20 +0100
Carl Eugen Hoyos <cehoyos@ag.or.at> wrote:

> From 694daed9222e50d6245bf5d041e82523ee869451 Mon Sep 17 00:00:00 2001
> From: Carl Eugen Hoyos <cehoyos@ag.or.at>
> Date: Thu, 26 Jan 2017 00:32:23 +0100
> Subject: [PATCH] lavf/mov: Ignore avio_skip() return value.
> 
> Fixes ticket #6102.
> ---
>  libavformat/mov.c |    4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 7dc550e..6f7e80a 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -4794,9 +4794,7 @@ static int mov_read_uuid(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>              av_free(buffer);
>          } else {
>              // skip all uuid atom, which makes it fast for long uuid-xmp file
> -            ret = avio_skip(pb, len);
> -            if (ret < 0)
> -                return ret;
> +            avio_skip(pb, len);
>          }
>      } else if (!memcmp(uuid, uuid_spherical, sizeof(uuid))) {
>          size_t len = atom.size - sizeof(uuid);

Why would you just ignore the error?

2017-01-26 6:27 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
> On Thu, 26 Jan 2017 00:34:20 +0100
> Carl Eugen Hoyos <cehoyos@ag.or.at> wrote:
>
>> From 694daed9222e50d6245bf5d041e82523ee869451 Mon Sep 17 00:00:00 2001
>> From: Carl Eugen Hoyos <cehoyos@ag.or.at>
>> Date: Thu, 26 Jan 2017 00:32:23 +0100
>> Subject: [PATCH] lavf/mov: Ignore avio_skip() return value.
>>
>> Fixes ticket #6102.
>> ---
>>  libavformat/mov.c |    4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>> index 7dc550e..6f7e80a 100644
>> --- a/libavformat/mov.c
>> +++ b/libavformat/mov.c
>> @@ -4794,9 +4794,7 @@ static int mov_read_uuid(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>>              av_free(buffer);
>>          } else {
>>              // skip all uuid atom, which makes it fast for long uuid-xmp file
>> -            ret = avio_skip(pb, len);
>> -            if (ret < 0)
>> -                return ret;
>> +            avio_skip(pb, len);
>>          }
>>      } else if (!memcmp(uuid, uuid_spherical, sizeof(uuid))) {
>>          size_t len = atom.size - sizeof(uuid);
>
> Why would you just ignore the error?

It fixes a regression (that I suspect will hit you very soon) and
it's what all other (17? 24?) calls to avio_skip() in mov.c do.

Doesn't an error here indicate a hardware issue?

Carl Eugen

On Thu, 26 Jan 2017 08:25:15 +0100
Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> 2017-01-26 6:27 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
> > On Thu, 26 Jan 2017 00:34:20 +0100
> > Carl Eugen Hoyos <cehoyos@ag.or.at> wrote:
> >  
> >> From 694daed9222e50d6245bf5d041e82523ee869451 Mon Sep 17 00:00:00 2001
> >> From: Carl Eugen Hoyos <cehoyos@ag.or.at>
> >> Date: Thu, 26 Jan 2017 00:32:23 +0100
> >> Subject: [PATCH] lavf/mov: Ignore avio_skip() return value.
> >>
> >> Fixes ticket #6102.
> >> ---
> >>  libavformat/mov.c |    4 +---
> >>  1 file changed, 1 insertion(+), 3 deletions(-)
> >>
> >> diff --git a/libavformat/mov.c b/libavformat/mov.c
> >> index 7dc550e..6f7e80a 100644
> >> --- a/libavformat/mov.c
> >> +++ b/libavformat/mov.c
> >> @@ -4794,9 +4794,7 @@ static int mov_read_uuid(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> >>              av_free(buffer);
> >>          } else {
> >>              // skip all uuid atom, which makes it fast for long uuid-xmp file
> >> -            ret = avio_skip(pb, len);
> >> -            if (ret < 0)
> >> -                return ret;
> >> +            avio_skip(pb, len);
> >>          }
> >>      } else if (!memcmp(uuid, uuid_spherical, sizeof(uuid))) {
> >>          size_t len = atom.size - sizeof(uuid);  
> >
> > Why would you just ignore the error?  
> 
> It fixes a regression (that I suspect will hit you very soon) and
> it's what all other (17? 24?) calls to avio_skip() in mov.c do.
> 
> Doesn't an error here indicate a hardware issue?

We do not normally ignore errors, especially not hardware errors.

mov.c really does contain a whole lot of unchecked skip, read and seek
calls. Low code quality, I guess.

Anyway, in issue #6102 you mentioned that this was caused by this
commit:

http://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=25e35b34365ea4fc737f406992b7947a0610edcb

The original code had no unchecked calls. Are you sure the commit
shouldn't just reverted, instead of removing an error check? Can you
explain why it's ok to ignore the error in this case specifically? Does
it actually work, or just prevent read_header from returning early?

Also, please provide all the information in the commit message, instead
of just mentioning an issue. This is why the commit message field
exists; use it.