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

[Michael Niedermayer]

Hi

On Thu, Jul 05, 2007 at 11:04:46PM -0400, Ronen Mizrahi wrote:
> 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
[...]
> 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, 

it easily takes an hour, every single return -1 must be checked that its
correct at all, needed, doesnt introduce leaks and that its the proper
way to handle the specific error
see below for some examples where your changes are totally wrong

also even after all are checked this patch is not acceptable as it fails
the requirement or being minimal to achive its goal which is a bugfix
not some error handlng cleanup


[...]
> --- \avidec.c	Wed Jul 04 19:09:02 2007
> +++ avidec.c	Wed Jul 04 19:21:28 2007
> @@ -163,11 +163,14 @@
>              duration = get_le32(pb);
>              pos = url_ftell(pb);
>  
> -            url_fseek(pb, offset+8, SEEK_SET);
> -            read_braindead_odml_indx(s, frame_num);
> +            if (url_fseek(pb, offset+8, SEEK_SET) < 0)
> +                return -1;
> +            if (read_braindead_odml_indx(s, frame_num) < 0)
> +                return -1;

failure to seek to and load a specific index chunk is not sufficient
to fail loading the other chunks



>              frame_num += duration;
>  
> -            url_fseek(pb, pos, SEEK_SET);
> +            if (url_fseek(pb, pos, SEEK_SET) < 0)
> +                return -1;
>          }
>      }
>      avi->index_loaded=1;
> @@ -205,7 +208,8 @@
>      offset_t i = url_ftell(pb);
>      size += (size & 1);
>      get_strz(pb, buf, maxlen);
> -    url_fseek(pb, i+size, SEEK_SET);
> +    if (url_fseek(pb, i+size, SEEK_SET) < 0)
> +        return -1;
>      return 0;
>  }
>  
> @@ -476,30 +480,39 @@
>          case MKTAG('i', 'n', 'd', 'x'):
>              i= url_ftell(pb);
>              if(!url_is_streamed(pb) && !(s->flags & AVFMT_FLAG_IGNIDX)){
> -                read_braindead_odml_indx(s, 0);
> +                if (read_braindead_odml_indx(s, 0) < 0)
> +                    return -1;
>              }

failure to read the index is not sufficient to fatally fail


> -            url_fseek(pb, i+size, SEEK_SET);
> +            if (url_fseek(pb, i+size, SEEK_SET) < 0)
> +                return -1;
>              break;
>          case MKTAG('I', 'N', 'A', 'M'):
> -            avi_read_tag(pb, s->title, sizeof(s->title), size);
> +            if (avi_read_tag(pb, s->title, sizeof(s->title), size) < 0)
> +                return -1;
>              break;

failure to read some meaningless metadata is not enough to fail reading the
avi header


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

While the State exists there can be no freedom; when there is freedom there
will be no State. -- Vladimir Lenin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070706/d46dc2ac/attachment.pgp>