[FFmpeg-cvslog] r18088 - trunk/libavformat/mxfdec.c

[Reimar Döffinger]

On Sun, Mar 22, 2009 at 03:22:52AM -0700, Baptiste Coudurier wrote:
> On 3/22/2009 2:31 AM, Reimar D?ffinger wrote:
> >>> Modified: trunk/libavformat/mxfdec.c
> >>> ==============================================================================
> >>> --- trunk/libavformat/mxfdec.c	Sat Mar 21 01:50:19 2009	(r18087)
> >>> +++ trunk/libavformat/mxfdec.c	Sat Mar 21 01:50:51 2009	(r18088)
> >>> @@ -364,6 +364,8 @@ static int mxf_read_primer_pack(MXFConte
> >>>  
> >>>  static int mxf_add_metadata_set(MXFContext *mxf, void *metadata_set)
> >>>  {
> >>> +    if (mxf->metadata_sets_count+1 >= UINT_MAX / sizeof(*mxf->metadata_sets))
> >>> +        return AVERROR(ENOMEM);
> >>>      mxf->metadata_sets = av_realloc(mxf->metadata_sets, (mxf->metadata_sets_count + 1) * sizeof(*mxf->metadata_sets));
> >>>      if (!mxf->metadata_sets)
> >>>          return -1;
> >> Just to be sure, is the test sufficient and the best ?
> > 
> > Depends is metadata_sets_count int or unsigned?
> 
> int
> 
> > The +1 can most likely overflow, simple rule: a working overflow check
> > will almost always have the check variable standing alone on one side.
> > Writing the equation the naive way and moving everything else to the other
> > side often works, you just have to make sure rounding happens the right
> > way around.
> 
> the +1 yes, however the check is >=, so it should be safe no ? It would
> just fail one before the max, if I'm not mistaken.

Not even remotely, you are missing the cases
metadata_sets_count == INT_MAX -> metadata_sets_count + 1 == INT_MIN < your constant
metadata_sets_count < -1 -> metadata_sets_count + 1 < 0 < your constant

> What would be the best situation ? unsigned and > ? (unsigned) cast + 1
> and >= ?

As I said, mxf->metadata_sets_count _alone_, not touching it. If you
wanted to add 1 to it, you'd first have to check you _can_ add 1 to it.
if (mxf->metadata_sets_count < 0 || mxf->metadata_sets_count >= INT_MAX / sizeof(*mxf->metadata_sets) - 1)
should work.
I'd avoid casting in such checks, it just makes it easier to confuse
yourself. You could make metadata_sets_count unsigned, as well as some
other stuff, but I'd really think thrice if you need to support the
case of multi-million entries that much.