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

[Jimmy Christensen]

On 2009-07-15 10:22, Reimar D?ffinger wrote:
> On Tue, Jul 14, 2009 at 04:42:27PM +0200, Jimmy Christensen wrote:
>> +    int variable_buffer_data_size
>
> Is only used inside the while loop ->  should be declared there.
> In general the whole header parsing is ugly enough that making
> it a separate function might be nice.
>

Will move it into a new function.

>> +    int bits_per_color_id = -1;
>> +    int channel_iter      = -1;
>
> These should also be declared locally where they are used.
>

These are actually assigned in the header parser and read when reading 
the pixels. So they can't really be moved.

>> +    if (buf_end - buf>  9) {
> [about 140 lines of code]
>> +    } else {
>> +        av_log(avctx, AV_LOG_ERROR, "Incomplete file\n");
>> +        return -1;
>> +    }
>
> Having more than 100 lines between the if and the else makes the code needlessly
> hard to read, particularly when the else part explains the reason for the whole
> check.
> Due to the return -1 in the else part it also increases the indentation leve needlessly.
> There are lots of place like this, they _all_ should be written like
>

Will do that.

>> +    if (buf_end - buf<  10) {
>> +        av_log(avctx, AV_LOG_ERROR, "Incomplete file\n");
>> +        return -1;
>> +    }
>
>
>> +                    variable_buffer_data_size = bytestream_get_le32(&buf);
>> +                    if (buf_end - buf>  variable_buffer_data_size) {
>> +                        const uint8_t *ptr_tmp = buf;
>> +                        if (buf_end - buf>  variable_buffer_data_size) {
>
> You still have the same check twice. It's not going to magically change the result
> in-between.
> Also most places lack a check for validating variable_buffer_data_size. You could
> try to verify that it does not cause a security issue there, but it would be
> simpler to just write a function that reads this value and checks its validity.
>

Will look into that.

>> +                        while(ptr_tmp + 1<  buf) {
>> +                            channel_iter++;
>> +                            if (buf - ptr_tmp>= 19&&  !strncmp(ptr_tmp, "R", 2)) {
>> +                                ptr_tmp += 2;
>> +                                red_channel = channel_iter;
>> +                                bits_per_color_id = bytestream_get_le32(&ptr_tmp);
>> +                                ptr_tmp += 16;
>> +                                continue;
>> +                            }
>
> As you pointed out to me, you do have the continues here because they are necessary.
> But you seemed to forget to add them to the checks in the outer loop? Both are
> exactly the same cases...

Hmm.. if you mean if I could replace "ptr_tmp + 1 <  buf" with "buf - 
ptr_tmp >= 19" it won't work like intended. Channels can be called 
something more than 1 character. Ofcourse you can argue that a channel 
does have to have atleast 1 character. So you are right that it can be 
replaced with "buf - ptr_tmp >= 19".

>
>> +                    // Check if the buffer has the required bytes needed from the offset
>> +                    if ((uint32_t)(buf_end - avpkt->data)>= line_offset + (8 + (channel_iter + 1) * 4 * xdelta)) {
>> +                        const uint8_t *red_channel_buffer   = avpkt->data + line_offset + 8 + (xdelta) * 4 * red_channel;
>> +                        const uint8_t *green_channel_buffer = avpkt->data + line_offset + 8 + (xdelta) * 4 * green_channel;
>> +                        const uint8_t *blue_channel_buffer  = avpkt->data + line_offset + 8 + (xdelta) * 4 * blue_channel;
>
> First, the way you use channel_iter here is not at all obvious.
> I'd suggest FFMAX3(red_channel, green_channel, blue_channel)
> Secondly, changing the order and the placement of () in the check compared to the
> later calculations makes it needlessly hard to find out how they are related.
> Thirdly, why the cast to uint32_t? You have to avoid an overflow in the calculation,
> so if anything you should cast the right side to some 64 bit value.
> And also what's with the () around (xdelta)?

Hmmm... you're right, After I thought about it I realized that it 
doesn't matter which channels are after the red, green and blue 
channels. channel_iter was to indicate how large the line for the 
channels would be. Eg. if there are 8 channels, the size of the line 
would be 8 * channel size * xdelta. Even that is not sufficient since 
channels can be of different sizes, so will need to create a variable in 
the header parser indicating the total sizes of the channels.