[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