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

Jimmy Christensen jimmy
Sat Apr 24 17:59:46 CEST 2010


It's been a while since I looked at this, but here goes :

On 2009-09-29 23:44, Michael Niedermayer wrote:
> On Mon, Sep 14, 2009 at 01:59:57PM +0200, Jimmy Christensen wrote:
> [...]
>> +/**
>> + * Convert from 32-bit float as uint32_t to uint16_t
>> + * @param v 32-bit float
>> + * @return normalized 16-bit unsigned int
>> + */
>> +static inline uint16_t exr_flt2uint(uint32_t v)
>> +{
>> +    int exp = v>>  23;
>> +    // "HACK": negative values result in exp<   0, so clipping them to 0
>> +    // is also handled by this condition, avoids explicit check for sign bit.
>
> v is unsigned, v>>23 should not have its sign bit set
>
>

Fixed.

> [...]
>> +/**
>> + * Checks if the variable name corresponds with it's data type
>> + * @param *buf the current pointer location in the header where
>> + * the variable name starts
>> + * @param *buf_end pointer location of the end of the buffer
>> + * @param *value_name name of the varible to check
>> + * @param *value_type type of the varible to check
>> + * @param minimum_length minimum length of the variable data
>> + * @return zero if variable is invalid
>> + */
>> +static int check_header_variable(const uint8_t *buf,
>> +                                 const uint8_t *buf_end,
>> +                                 const char *value_name,
>> +                                 const char *value_type,
>> +                                 unsigned int minimum_length)
>> +{
>> +    if (buf_end - buf>= minimum_length&&  !strcmp(buf, value_name)) {
>> +        buf += strlen(value_name)+1;
>> +        if (!strcmp(buf, value_type))
>> +            return 1;
>> +        av_log(NULL, AV_LOG_ERROR, "Unknown data type for header variable %s\n", value_name);
>                    ^^^^
> new av_logs should have non null contextes
>
>

Fixed

> [...]
>> +static int get_rgb_channel(const uint8_t **buf,
>> +                           const uint8_t *channel_list_end,
>> +                           EXRContext *const s,
>> +                           int *current_channel_offset)
>> +{
>> +    const uint8_t bytes_per_color_table[3] = {
>> +        1, 2, 4
>> +    };
>> +    int current_bits_per_color_id = -1;
>> +    int channel_index = -1;
>> +
>> +    if (!strncmp(*buf, "R", 2))
>> +        channel_index = 0;
>> +    if (!strncmp(*buf, "G", 2))
>> +        channel_index = 1;
>> +    if (!strncmp(*buf, "B", 2))
>> +        channel_index = 2;
>
>
>
>> +
>> +    while (bytestream_get_byte(buf)&&  *buf<  channel_list_end)
>> +        continue; /* skip */
>> +
>> +    if (channel_list_end - * buf<  4)
>> +        return -2;
>> +    current_bits_per_color_id = bytestream_get_le32(buf);
>> +
>> +    if (current_bits_per_color_id>  2)
>> +        return -1;
>> +
>> +    if (channel_index>= 0) {
>> +        if (s->bits_per_color_id != -1&&  s->bits_per_color_id != current_bits_per_color_id) {
>> +            return -3;
>
> these literal return codes are unacceptable
> and as the function is called just once it can be inlined making them
> unneeded
>
>

Changed.

>> +        }
>> +        s->bits_per_color_id  = current_bits_per_color_id;
>> +        s->channel_offsets[channel_index] = *current_channel_offset;
>> +    }
>
>> +    *current_channel_offset += bytes_per_color_table[current_bits_per_color_id];
>
> 1<<current_bits_per_color_id
>
>

Fixed.

>> +    *buf += 12;
>> +    return 1;
>> +}
>> +
>> +static int decode_frame(AVCodecContext *avctx,
>> +                        void *data,
>> +                        int *data_size,
>> +                        AVPacket *avpkt)
>> +{
>> +    const uint8_t *buf      = avpkt->data;
>> +    unsigned int   buf_size = avpkt->size;
>> +    const uint8_t *buf_end  = buf + buf_size;
>> +
>> +    EXRContext *const s = avctx->priv_data;
>> +    AVFrame *picture  = data;
>> +    AVFrame *const p =&s->picture;
>> +    uint8_t *ptr;
>> +
>> +    int x, y, stride, magic_number;
>> +    int w = 0;
>> +    int h = 0;
>> +    unsigned int xmin   = ~0;
>> +    unsigned int xmax   = ~0;
>> +    unsigned int ymin   = ~0;
>> +    unsigned int ymax   = ~0;
>> +    unsigned int xdelta = ~0;
>> +
>> +    unsigned int current_channel_offset = 0;
>> +
>> +    s->channel_offsets[0] = -1;
>> +    s->channel_offsets[1] = -1;
>> +    s->channel_offsets[2] = -1;
>> +    s->bits_per_color_id = -1;
>> +
>> +    magic_number = bytestream_get_le32(&buf);
>> +    if (magic_number != 20000630) { // As per documentation of OpenEXR it's supposed to be int 20000630 little-endian
>> +        av_log(avctx, AV_LOG_ERROR, "Wrong magic number %d\n", magic_number);
>> +        return -1;
>> +    }
>> +
>
>> +    // Move to header start
>> +    buf += 4;
>
> the comment is poor, it should explain what it is that is skiped over here
>
>

Changed to actually read and use the version flag instead of just 
skipping it.

> [...]
>> +        // Process the lineOrder variable
>> +        if (check_header_variable(buf, buf_end, "lineOrder", "lineOrder", 25)) {
>> +            buf += 20;
>> +
>> +            variable_buffer_data_size = get_header_variable_length(&buf, buf_end);
>> +            if (!variable_buffer_data_size)
>> +                return -1;
>> +            if (*buf) {
>> +                av_log(avctx, AV_LOG_ERROR, "Doesn't support this line order : %d\n", *buf);
>> +                return -1;
>> +            }
>> +
>> +            buf += variable_buffer_data_size;
>> +            continue;
>> +        }
>> +
>> +        // Process the compression variable
>> +        if (check_header_variable(buf, buf_end, "compression", "compression", 29)) {
>
>> +            buf += 24;
>> +
>> +            variable_buffer_data_size = get_header_variable_length(&buf, buf_end);
>> +            if (!variable_buffer_data_size)
>> +                return -1;
>
> that code is repeated all over the place
>
>

Have moved the header jump to the check_header_variable function. which 
makes it a little less redundant. Hopefully enough.

> [...]
>> +    if (w != avctx->width || h != avctx->height) {
>> +        // Verify the xmin, xmax, ymin, ymax and xdelta before setting the actual image size
>> +        if (xmin>  w || xmin == ~0 ||
>> +            xmax>  w || xmax == ~0 ||
>> +            ymin>  h || ymin == ~0 ||
>> +            ymax>  h || ymax == ~0 ||
>
> these look partly redundant
>
>

Since these values are read from the file they could be wrong. Could 
perhaps move the check to earlier in the code.

>> +            xdelta == ~0 ||
>> +            xdelta>  avpkt->size / current_channel_offset) {
>> +            av_log(avctx, AV_LOG_ERROR, "Wrong sizing or missing size information\n");
>> +            return -1;
>> +        }
>> +        avcodec_set_dimensions(avctx, w, h);
>> +    }
>> +
>> +    if (avctx->get_buffer(avctx, p)<  0) {
>> +        av_log(avctx, AV_LOG_ERROR, "get_buffer() failed\n");
>> +        return -1;
>> +    }
>> +
>> +    ptr    = p->data[0];
>> +    stride = p->linesize[0];
>> +
>> +    // Zero out the start if ymin is not 0
>> +    for (y = 0; y<  ymin; y++) {
>> +        memset(ptr, 0, avctx->width * 6);
>> +        ptr += stride;
>> +    }
>> +
>> +    // Process the actual lines
>> +    for (y = ymin; y<= ymax; y++) {
>> +        uint16_t *ptr_x = (uint16_t *)ptr;
>> +        if (buf_end - buf>  8) {
>> +            const uint64_t line_offset = bytestream_get_le64(&buf) + 8;
>> +            // Check if the buffer has the required bytes needed from the offset
>> +            if (line_offset>  avpkt->size - xdelta * current_channel_offset) {
>
> this code looks a little obfuscated, but at least its missing overflow
> checks, also some critical checks further up are under ifs that if ever false
> would almost certainly be exploitable as the checks are no longer done ..
>

Have added more comments to explain the code. Could you point to which 
if statements you are talking about?

Also re-checked the developer documentation and I saw that I needed to 
bump the Minor version for libavcodec, so I did.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: openEXR-rev20.diff
Type: text/x-patch
Size: 20690 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100424/3fe85ab9/attachment.bin>



More information about the ffmpeg-devel mailing list