[FFmpeg-devel] [PATCH] OpenEXR decoder rev-4

[Reimar Döffinger]

On Fri, Jul 03, 2009 at 11:04:33AM +0200, Jimmy Christensen wrote:
> Reimar, perhaps you could help me out? :)

You mostly just need to change a few numbers.

> +static inline uint16_t exr_flt2uint(uint32_t v)
> +{
> +    int exp = v >> 23;
> +    if (v & 0x80000000)
> +        return 0;
> +    if (exp <= 127+7-24) // we would shift out all bits anyway
> +        return 0;
> +    if (exp >= 127)
> +        return 0xffff;
> +    v &= 0x007fffff;
> +    return (v+(1<<23)) >> (127+7-exp);
> +}


> >> +        strcpy(variable_buffer_name, buf);
> >> +        buf += strlen(variable_buffer_name)+1;
> >> +        strcpy(variable_buffer_type, buf);
> >> +        buf += strlen(variable_buffer_type)+1;
> >
> > One possible buffer overflow after the other.
> > Apart from that, I don't see the point why you make those copies,
> > variable_buffer_type is not used at all, and variable_buffer_name can
> > be compared in-place.
> 
> Not sure how to do this otherwise. The variable_buffer_name is always of 
> different size and only uses 0x0 as a delimiter.

What? variable_buffer_name is exactly 64 bit large and your code will
happily try to write any amount of data into it when there is no 0
termination. In addition it is on the stack, so this is _trivial_ to
exploit, anyone using this code could just make his passwords public and
he'd probably be just as safe.
Have you never heard of functions like strncpy (or the more useful
strlcpy, since it is not portable in FFmpeg as av_strlcpy?).
And all that is no excuse to make a copy of it just to compare it. Or
do you always when you read a book first copy it, then read the copy
while having the copy lying next to you and then burn the copy you made?
Because that is more or less what your code does...
Or to put in code what would make sense to do:
if (buf_end - buf >= 9 && !strncmp(buf, "channels", 9)) {
    buf += 9;
    skip_buffertype();
    ...

As long as you are using strcpy, strcmp, strlen or anything like that on
external data, unverified data you are doing it wrong.

> Keeping the 
> variable_buffer_type is to be able to use it for later in the 
> interpretation of the different variables. And it also serves for the 
> purpose of knowing how many bytes to skip to get to the actual data.

Huh? It is _not_ used anywhere, not one bit and nowhere at all. Copying
data into it is a complete waste of time.