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

[Reimar Döffinger]

On Sun, Jul 05, 2009 at 10:30:16PM +0200, Jimmy Christensen wrote:
> +    //int buf_end        = (buf + buf_size);

?

> +    int red_channel        .= -1;

I don't think that'll work with that . in there.

> +        if (buf_end - buf >= 20 && !strncmp(buf, "channels", 9)) {
> +            buf += 9;
> +            if (!strncmp(buf, "chlist", 7)) {
> +                buf += 7;

A function would probably make sense, e.g.
static int compare_str(const uint8_t **buf, const uint8_t *buf_end, const char *str) {
    int len = strlen(str) + 1;
    if (buf_end - *buf < len || strncmp(buf, str, len))
        return 0;
    *buf += len;
    return 1;
}

> +                variable_buffer_data_size = bytestream_get_le32(&buf);
> +                channel_iter = -1;
> +                if((buf + variable_buffer_data_size) < buf_end) {

buf + variable_buffer_data_size can overflow.
The unsafe data (e.g. the thing you read from the file and haven't
verified) should always stand alone.
Also the () is useless.

> +                    const uint8_t *ptr_tmp = buf;
> +                    if((buf_end - buf) > variable_buffer_data_size) {

What's supposed to be the difference between this check and the one
above??
Also you should decide on which part to put right and which left, not
change it around IMO.

> +                    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 = AV_RL32(ptr_tmp);
> +                            ptr_tmp += 16;
> +                            continue;
> +                        }
> +
> +                        if((buf - ptr_tmp) >= 19 && !strncmp(ptr_tmp, "G", 2)) {
> +                            ptr_tmp += 2;
> +                            green_channel = channel_iter;
> +                            ptr_tmp += 16;
> +                            continue;
> +                        }
> +
> +                        if((buf - ptr_tmp) >= 19 && !strncmp(ptr_tmp, "B", 2)) {
> +                            ptr_tmp += 2;
> +                            blue_channel = channel_iter;
> +                            ptr_tmp += 16;
> +                            continue;
> +                        }
> +
> +                        // Process other channels
> +                        if(ptr_tmp < buf) {
> +                            while(ptr_tmp[0] != 0x0) {
> +                                ptr_tmp++;
> +                            }
> +                            ptr_tmp += 17;
> +                        }

Both inside this loop as well as in the outer loop you can't handle the
"else" case this way. If someone stores the data in the order "B", "G",
"R", your "process other channels" part will throw away the "G".

> +        // Process unknown variables
> +        if((buf_end - buf) > 9) {
> +            // Skip variable name
> +            while(buf[0] != 0x0 && buf < buf_end) {

If buf[0] is 0, then that indicates the end of the header, it's not an
unknown variable.
Also first accessing the data and the checking if it is in bounds makes
little sense, even though due to padding it probably won't cause issues.

> +            // Skip variable type
> +            while(buf[0] != 0x0 && buf < buf_end) {
> +                buf++;
> +            }
> +            buf++;

Also it can be shortened to
while (buf < buf_end && *buf++) /**/;


> +                variable_buffer_data_size = bytestream_get_le32(&buf);
> +                if((buf_end - buf) > variable_buffer_data_size) {
> +                    buf += variable_buffer_data_size;
> +                } else {
> +                    av_log(avctx, AV_LOG_ERROR, "Incomplete header\n");
> +                    return -1;
> +                }

That's at least the second place where I see this code. A function might
be a good idea.

> +                uint16_t* ptr_x = (uint16_t*)ptr;

The * is placed inconsistently.

> +                const uint32_t line_offset = AV_RL64(buf);

bytestream_get_le64

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

You aren't checking that these actually point anywhere valid.
Try adding setting "line_offset = 0x4fffffff;" and you should see what I
mean.

> +                    *ptr_x++ = exr_flt2uint(AV_RL32(red_channel_buffer));
> +                    red_channel_buffer += 4;
> +                    *ptr_x++ = exr_flt2uint(AV_RL32(green_channel_buffer));
> +                    green_channel_buffer += 4;
> +                    *ptr_x++ = exr_flt2uint(AV_RL32(blue_channel_buffer));
> +                    blue_channel_buffer += 4;

bytestream_get_le32 for all of these.

> +                // Zero out the end if xmax+1 is not w
> +                memset(ptr_x, 0, (avctx->width - (xmax+1))*6);
> +                ptr_x += (avctx->width - (xmax+1))*3;

Incrementing ptr_x is pointless.

> +        // 16-bit
> +        case 1:

basically the same comments.