[FFmpeg-devel] stsz overflow

[Reimar Döffinger]

On Tue, Aug 25, 2009 at 10:16:52AM -0700, Frank Barchard wrote:
> On Tue, Aug 25, 2009 at 9:16 AM, Reimar D?ffinger
> <Reimar.Doeffinger at gmx.de>wrote:
> 
> > On Tue, Aug 25, 2009 at 09:11:21AM -0700, Baptiste Coudurier wrote:
> > > > Wow, what a mess (IMO). I think we are already at the point where it
> > > > would be simpler to just get rid of that buffer and directly read the
> > > > values "one by one" from the file.
> > >
> > > No, it was decided to be done that way when the patch was submitted.
> >
> > Obviously the review at that time did not take into account the
> > additional code and complexity of avoiding buffer overflows, which
> > unless someone comes up with a cleaner check is a really good reason
> > to reconsider that decision.
> 
> 
> Here is the simplest change that addresses the math overflow.  It limits
> stsz to 134,217,728 entries.
> 
> Index: libavformat/mov.c
> ===================================================================
> --- libavformat/mov.c   (revision 19697)
> +++ libavformat/mov.c   (working copy)
> @@ -1256,7 +1256,7 @@
>          return -1;
>      }
> 
> -    if(entries >= UINT_MAX / sizeof(int))
> +    if(entries >= UINT_MAX / 32)  /* avoids buffer overrun */
>          return -1;

Seems reasonable to me, except for the comment.
A buffer overrun/overflow is only the secondary effect.
The right comment should be something like "avoids integer
overflow in multiplication with field_size".
Particularly mentioning field_size may reduce the risk of forgetting
to change this if ever e.g. field_size == 64 should become possible.
Or
if (entries >= UINT_MAX / sizeof(int) || entries >= (UINT_MAX - 4) / field_size)
as a compromise.
Anyway, I think we have enough possibilities to choose from, just
need to pick one and go with it.
Though I still think that using get_bits isn't worth the 13 lines of
code it needs - though the alternative is a bit ugly as well.