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

[Jimmy Christensen]

On 2009-07-03 14:16, Reimar D?ffinger wrote:
> 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);
>> +}
>

Ofcourse, but which and to what is the question :)

Used your code and the performance increased 25x fold. Thanks! :)

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

No I haven't heard about strncpy or strlcpy. But thanks for pointing me 
in the direction.

The problem is that I don't know what the next variable is called and 
how long it is. Also I don't know what all of them are called, since the 
creator have the option of making their own.

I suppose this should work?

>         av_strlcpy(variable_buffer_name, buf, (buf_end - buf));
>         buf += strlen(variable_buffer_name)+1;
>         av_strlcpy(variable_buffer_type, buf, (buf_end - buf));
>         buf += strlen(variable_buffer_type)+1;

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

I actually need it for 1 purpose. To get the length of the variable.

In this document on page 8 the layout of the header is explained :
http://www.openexr.com/openexrfilelayout.pdf

Personally I think it's quite stupid the way it's structured and a bit 
annoying to parse.