[FFmpeg-devel] [PATCH] OpenEXR decoder rev-19
Michael Niedermayer
michaelni
Tue Sep 29 23:44:23 CEST 2009
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
[...]
> +/**
> + * 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
[...]
> +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
> + }
> + 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
> + *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
[...]
> + // 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
[...]
> + 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
> + 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 ..
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Avoid a single point of failure, be that a person or equipment.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090929/fa166c4b/attachment.pgp>
More information about the ffmpeg-devel
mailing list