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

Michael Niedermayer michaelni
Sat Apr 19 20:37:41 CEST 2008


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.
For example a compiler could very well clearly define the behavior and
its users could then write code depending on this specific compilers
behaviour.

gcc chooses to take the "undefined" litterally and in the most radical sense.
Other compilers might choose to rather behave in a way which most non
language lawyers would expect. One surely can argue for either side.

Personally i would prefer a compiler behaving intuitivly and in a way
similar to the underlaying machine instructions(which really behave the
same across most common architectures) and in addition warn me
of any detected code with undefined behavior. At least by default.
Also has anyone ever asked what gcc gains by removing such checks? It
certainly has lead to security holes in practice. Is there any speed
gain from these assumtations at all? Or is it just done because it is
allowed by the C standard without any consideration of the consequences?


> 
> > I can make it extradata_size > INT_MAX - 0x1ff , I just considered it
> > quite obfuscated.
> 
> Why isn't extradata_size unsigned?

I suspect there is no real reason.

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

The worst form of inequality is to try to make unequal things equal.
-- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080419/e79167d9/attachment.pgp>



More information about the ffmpeg-devel mailing list