[FFmpeg-devel] avidec.c entering an infinite loop with HTTP URLs + Patch

[Ronen Mizrahi]

Michael Niedermayer wrote:
> Hi
>
> On Wed, Jul 04, 2007 at 07:34:38PM -0400, Ronen Mizrahi wrote:
>   
>> Hi,
>>
>> I have encountered a situation where url_fseek() fails, but its return 
>> value is not checked and hence the surrounding code enters an infinite loop.
>> The relevant code in in avidec.c and since I am running on Windows with 
>> MSVC I do not have a GDB dump to offer. I did however modify avidec.c 
>> (see patch attached) such that return values are checked and the problem 
>> was eliminated. I would be grateful if someone can apply the patch 
>> and/or comment to it. The patch wad made against the current SVN head 
>> (revision 8742 for avidec.c).
>>
>> Thank you,
>>
>> Ronen Mizrahi
>>     
>
>   
>> --- avidec.c	Wed Jul 04 19:21:28 2007
>> +++ \avidec.c	Wed Jul 04 19:09:02 2007
>> @@ -163,14 +163,11 @@
>>              duration = get_le32(pb);
>>              pos = url_ftell(pb);
>>  
>> -            if (url_fseek(pb, offset+8, SEEK_SET) < 0)
>> -                return -1;
>> -            if (read_braindead_odml_indx(s, frame_num) < 0)
>> -                return -1;
>> +            url_fseek(pb, offset+8, SEEK_SET);
>> +            read_braindead_odml_indx(s, frame_num);
>>              frame_num += duration;
>>  
>> -            if (url_fseek(pb, pos, SEEK_SET) < 0)
>> -                return -1;
>> +            url_fseek(pb, pos, SEEK_SET);
>>          }
>>      }
>>      avi->index_loaded=1;
>> @@ -208,8 +205,7 @@
>>      offset_t i = url_ftell(pb);
>>      size += (size & 1);
>>      get_strz(pb, buf, maxlen);
>> -    if (url_fseek(pb, i+size, SEEK_SET) < 0)
>> -        return -1;
>> +    url_fseek(pb, i+size, SEEK_SET);
>>      return 0;
>>  }
>>  
>>     
>
> the patch looks reversed, also it seems you check everything and return -1
> this is overkill also it would require me to do your work checking that
> this actually is correct and that return -1 from these places does not
> introduce bugs
> so please only change the code which is related to the bug
>
> if you think every url_fseek() should have its return checked then this 
> should be a seperate disscussion, thread and patch
>
> [...]
>   
> ------------------------------------------------------------------------
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at mplayerhq.hu
> http://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel

The patch was indeed reversed, my apologies. Please see attached a 
correct one.

Regarding your comments, I have verified that the -1 return value 
eventually bubbles up to the top level functions (avi_read_header, 
avi_read_packet, avi_read_close, avi_read_seek) and that these functions 
have other scenarios in which they return -1 (same is true for most of 
the intermediate cases and I did verify the rest). I also ran it on a 
large set of AVI files and compared the results.
The time it takes to review the changes is probably less than 10 
minutes, so if someone is able to do it, this is very much appreciated, 
otherwise I will try to submit a more specific patch when/if I get a chance.

Regarding the issue of checking return values of url_fseek() in ffmpeg, 
I do think every invocation throughout the code needs to check the 
return value, simply because networking is quite likely to fail (unlike 
local file access). As you say however this is a very different issue, 
and it involves a lot of refactoring.

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: avidec.c.diff
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070705/2824ea49/attachment.txt>