[FFmpeg-devel] [PATCH] ff_split_xiph_headers returns broken header_len < 0

[Måns Rullgård]

Michael Niedermayer <michaelni at gmx.at> writes:

> On Sat, Apr 19, 2008 at 08:22:03PM +0100, M?ns Rullg?rd wrote:
>> Michael Niedermayer <michaelni at gmx.at> writes:
>> 
>> > On Sat, Apr 19, 2008 at 07:11:45PM +0100, M?ns Rullg?rd wrote:
>> >> Reimar D?ffinger <Reimar.Doeffinger at stud.uni-karlsruhe.de> writes:
>> >> 
>> >> > On Sat, Apr 19, 2008 at 05:29:02PM +0200, Michael Niedermayer wrote:
>> >> >> On Fri, Apr 18, 2008 at 11:02:56PM +0200, Reimar D?ffinger wrote:
>> >> >> >          }
>> >> >> > -    } else if (extradata[0] == 2) {
>> >> >> > +    } else if (extradata_size >= 3 && extradata_size + 0x1ff > 0 && extradata[0] == 2) {
>> >> >> 
>> >> >> this check will not work with gcc, have i not been loud enough about gccs
>> >> >> behaviour with signed overflow checks?
>> >> >
>> >> > Well, I am not sure whether there is a point in this check or not. Also
>> >> > I must have missed it, I was only aware of the "problem" when a pointer
>> >> > is involved.
>> >> 
>> >> There is no problem with pointers, or rather, with pointers the
>> >> problem is that the check is wrong in the first place, whatever the
>> >> compiler does.
>> >
>> > Adding "too much" to a pointer has undefined behaviour according to the C
>> > standard.
>> > A check like ptr+123 > ptr2 can have undefined behaviour depending on what
>> > ptr points to. This doesnt make the check wrong.
>> 
>> Yes, a check like that is meaningful, and is not a problem with gcc
>> either.  
>
>> A check like ptr+x < ptr is however useless, since even
>> without wraparound, the result may be outside the allocated buffer.
>
> Such a check is not useless.
> ptr + x > end_ptr || ptr+x < ptr
> certainly makes sense

Except that if either check fails (intuitively), it's undefined
behaviour according to the C standard.  Better to write
end_ptr - ptr < x instead.  It's both simpler and well-defined.

-- 
M?ns Rullg?rd
mans at mansr.com