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

[Reimar Döffinger]

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.

> +    int bits_per_color_id = -1;
> +    int channel_iter      = -1;

These should also be declared locally where they are used.

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

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

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

> +                    // 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)?