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

Baptiste Coudurier baptiste.coudurier
Sun Mar 22 12:34:55 CET 2009


On 3/22/2009 3:37 AM, Reimar D?ffinger wrote:
> 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

I don't get it,
the case when metadata_sets_count == INT_MAX-1 will fail before the case
when metadata_sets_count == INT_MAX because of the >= , no ?
Then it's over, ideally, it will return failure and stop.

>> 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.

Yes, I got this. Better leave the value alone in one side.

> 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.

I just would like to know what is the more appropriate check.

in libavformat/utils.c the check is:
if((unsigned)st->nb_index_entries + 1 >= UINT_MAX/sizeof(AVIndexEntry))
    return -1;

I just notice that the check is not uniform everywhere.
Would introducting a macro for this and using it everywhere be useful ?

What do others think ?

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
checking for life_signs in -lkenny... no
FFmpeg maintainer                                  http://www.ffmpeg.org



More information about the ffmpeg-cvslog mailing list