On Wed, Jul 01, 2009 at 04:06:58PM +0200, Jimmy Christensen wrote:

+ EXRContext *const s = avctx->priv_data; + AVFrame *const p = &s->picture;

I don't see why const would be particularly appropriate.

+ char variable_buffer_name[64], variable_buffer_type[64], channel_name[31];

char can be signed or unsigned.

+ static int bits_per_color_table[3] = {8, 16, 32};

const. Also it looks like it is just 8 << i

+ uint8_t red_channel = -1; + uint8_t green_channel = -1; + uint8_t blue_channel = -1;

~0 looks nicer for unsigned values and also would work on non-twos-complement systems (though FFmpeg won't work on them anyway).

+ unsigned long line_offsets[4096];

This are either 32 or 64 bit, depending on the system, are you sure that is what you wanted? Also 32 kB on the stack really is not reasonable, such "monsters" usually belong e.g. into the context.

+ magic_number = AV_RL32(buf);

bytestream_get_le32 might be nicer (even though you'll still have the addition to skip 4 more bytes...

+ strcpy(variable_buffer_name, buf); + buf += strlen(variable_buffer_name)+1; + strcpy(variable_buffer_type, buf); + buf += strlen(variable_buffer_type)+1;

One possible buffer overflow after the other. Apart from that, I don't see the point why you make those copies, variable_buffer_type is not used at all, and variable_buffer_name can be compared in-place.

+ variable_buffer_data_size = AV_RL32(buf); + buf += 4;

bytestream_get_...

+ const uint8_t *ptr_tmp = buf; + for (channel_iter = 0; (buf + variable_buffer_data_size) > (ptr_tmp + 1); channel_iter++) {

buf + variable_buffer_data_size could overflow. Though that is not actually an issue here, the issue is that you compare nothing against the actual remaining size of what buf points to.

+ strcpy(channel_name, ptr_tmp); + ptr_tmp += strlen(channel_name) + 1; + if (!strcmp(channel_name, "R")) { + red_channel = channel_iter; + bits_per_color_id = AV_RL32(ptr_tmp); + } + if (!strcmp(channel_name, "G")) + green_channel = channel_iter; + if (!strcmp(channel_name, "B")) + blue_channel = channel_iter;

I can't see the point on the copy here - except another buffer overflow.

+ switch (bits_per_color_table[bits_per_color_id]) {

If you made the switch simply one bits_per_color_id it at least wouldn't crash when bits_per_color_id is too big.

+ av_log(avctx, AV_LOG_ERROR, "Unkown color format : %d\n", bits_per_color_id);

The word is "Unknown" (an 'n' is missing).

+ // Get the line offsets which are stored as unsigned long

Really? I wonder how they managed that. They are stored in a format that magically changes from 32 to 64 bits and back when you copy it from a 32 bit to a 64 bit Linux system?

+ for (i = ymin; i <= ymax; i++) { + line_offsets[i] = AV_RL64(buf); + buf += 8;

bytestream_get_... Obviously you'd have to check that those line_offsets actually are valid.

+ // Zero out the start if ymin is not 0 + for (y = 0; y < ymin; y++) { + uint16_t* ptr_x = (uint16_t*)ptr; + for (x = 0; x < avctx->width; x++) { + *ptr_x++ = 0; + *ptr_x++ = 0; + *ptr_x++ = 0; + }

That's what memset is there for.

+ // Zero out the start if xmin is not 0 + for (x = 0; x < xmin; x++) { + *ptr_x++ = 0; + *ptr_x++ = 0; + *ptr_x++ = 0; + }

same here.

+ // Process the actual pixels + switch (bits_per_color_table[bits_per_color_id]) { + // 32bit float + case 32: + for (x = 0; x < xdelta; x++) { + buf = avpkt->data + line_offsets[y] + 8 + (xdelta)*4*red_channel + (x)*4; + *ptr_x++ = (int)(av_int2flt(AV_RL32(buf)) * 65535); + buf = avpkt->data + line_offsets[y] + 8 + (xdelta)*4*green_channel + (x)*4; + *ptr_x++ = (int)(av_int2flt(AV_RL32(buf)) * 65535); + buf = avpkt->data + line_offsets[y] + 8 + (xdelta)*4*blue_channel + (x)*4; + *ptr_x++ = (int)(av_int2flt(AV_RL32(buf)) * 65535);

You'll probably want to use the same code as for half-float here, except that denormals are definitely 0 here.

+ } + break; + // 16bit half float + case 16: + for (x = 0; x < xdelta; x++) { + buf = avpkt->data + line_offsets[y] + 8 + (xdelta)*2*red_channel + (x)*2; + *ptr_x++ = exr_halflt2uint(AV_RL16(buf)); + buf = avpkt->data + line_offsets[y] + 8 + (xdelta)*2*green_channel + (x)*2; + *ptr_x++ = exr_halflt2uint(AV_RL16(buf)); + buf = avpkt->data + line_offsets[y] + 8 + (xdelta)*2*blue_channel + (x)*2; + *ptr_x++ = exr_halflt2uint(AV_RL16(buf));

You certainly don't need to recalculate buf each time from scratch.

On Wed, Jul 01, 2009 at 11:00:14PM +0200, Jimmy Christensen wrote:

It's stated in the documentation for the fileformat :

The line offset table allows random access to scan line blocks. The table is a sequence of scan line offsets, with one offset per scan line block. A scan line offset, of type unsigned long, indicates the distance, in bytes, between the start of the file and the start of the scan line block. In the table, scan line offsets are ordered according to increasing scan line y coordinates.

I would suspect that the idea perhaps is that on 64-bit systems it can use the full range, but not on 32-bit systems.

unsigned long is also 32 bit on 64 bit Windows systems. Either the documentation does not use the C meaning of that (what else?), the part you quoted lacks context, or the documentation is just braindead. Since you're probably not going to work with asm code, just remember the simple rule: if you are using "long" you are doing something wrong.

On Thu, Jul 02, 2009 at 07:54:32AM +0200, Jimmy Christensen wrote:

On 2009-07-01 23:24, Reimar D?ffinger wrote:

...

On Wed, Jul 01, 2009 at 11:00:14PM +0200, Jimmy Christensen wrote:

...

It's stated in the documentation for the fileformat :

The line offset table allows random access to scan line blocks. The table is a sequence of scan line offsets, with one offset per scan line block. A scan line offset, of type unsigned long, indicates the distance, in bytes, between the start of the file and the start of the scan line block. In the table, scan line offsets are ordered according to increasing scan line y coordinates.

I would suspect that the idea perhaps is that on 64-bit systems it can use the full range, but not on 32-bit systems.

unsigned long is also 32 bit on 64 bit Windows systems. Either the documentation does not use the C meaning of that (what else?), the part you quoted lacks context, or the documentation is just braindead. Since you're probably not going to work with asm code, just remember the simple rule: if you are using "long" you are doing something wrong.

The documentation is here : http://www.openexr.com/openexrfilelayout.pdf

After re-reading it, I found that it does say what it means exactly with "unsigned long", so will change it specifically to that. Didn't know there were such a difference in Windows (not surprised either).

Note that FFmpeg via avcodec_check_dimensions limits the size of a frame to 256 MPixels, which in case of the float format would be just 1GB. Thus caring about more than the lowest 32 bit seems pointless for FFmpeg, particular since such large frames can't be processed with FFmpeg on 32 bit systems at all, I doubt there is much of a point in supporting things that can only work on 64 bit systems at all.

On Thu, Jul 02, 2009 at 08:52:36AM +0200, Jimmy Christensen wrote:

On 2009-07-02 08:24, Reimar D?ffinger wrote:

...

Note that FFmpeg via avcodec_check_dimensions limits the size of a frame to 256 MPixels, which in case of the float format would be just 1GB. Thus caring about more than the lowest 32 bit seems pointless for FFmpeg, particular since such large frames can't be processed with FFmpeg on 32 bit systems at all, I doubt there is much of a point in supporting things that can only work on 64 bit systems at all.

I will probably move away from using the table at all, so if it's read as 32-bit or 64-bit should not matter that much anymore. Personally I would rather do it according to the documentation.

I'd strongly recommend against taking the documentation seriously. E.g. the "official" source code uses Int64 as type, so actually signed, treating negative values as a sign of incomplete/broken files.

On 2009-07-03 11:04, Jimmy Christensen wrote:

Have updated a lot of things and restructured the code a bit.

Still using the reference 32-bit float converter as I couldn't figure out how to make a function like Reimar's, work for 32-bit float.

Reimar, perhaps you could help me out? :)

...

...

+ strcpy(variable_buffer_name, buf); + buf += strlen(variable_buffer_name)+1; + strcpy(variable_buffer_type, buf); + buf += strlen(variable_buffer_type)+1;

One possible buffer overflow after the other. Apart from that, I don't see the point why you make those copies, variable_buffer_type is not used at all, and variable_buffer_name can be compared in-place.

Not sure how to do this otherwise. The variable_buffer_name is always of different size and only uses 0x0 as a delimiter. Keeping the variable_buffer_type is to be able to use it for later in the interpretation of the different variables. And it also serves for the purpose of knowing how many bytes to skip to get to the actual data.

Everything else should hopefully be fixed.

Just realized out that the 32-bit path will not work on big endian systems. This patch should fix it.

On 2009-07-05 22:27, Jimmy Christensen wrote:

On 2009-07-03 11:09, Jimmy Christensen wrote:

...

On 2009-07-03 11:04, Jimmy Christensen wrote:

...

Have updated a lot of things and restructured the code a bit.

Still using the reference 32-bit float converter as I couldn't figure out how to make a function like Reimar's, work for 32-bit float.

Reimar, perhaps you could help me out? :)

...

...

+ strcpy(variable_buffer_name, buf); + buf += strlen(variable_buffer_name)+1; + strcpy(variable_buffer_type, buf); + buf += strlen(variable_buffer_type)+1;

One possible buffer overflow after the other. Apart from that, I don't see the point why you make those copies, variable_buffer_type is not used at all, and variable_buffer_name can be compared in-place.

Not sure how to do this otherwise. The variable_buffer_name is always of different size and only uses 0x0 as a delimiter. Keeping the variable_buffer_type is to be able to use it for later in the interpretation of the different variables. And it also serves for the purpose of knowing how many bytes to skip to get to the actual data.

Everything else should hopefully be fixed.

Just realized out that the 32-bit path will not work on big endian systems. This patch should fix it.

Reworked the header parser to hopefully be more secure.

Removed an unnecessary comment...

On 2009-07-06 15:54, Reimar D?ffinger wrote:

On Sun, Jul 05, 2009 at 10:30:16PM +0200, Jimmy Christensen wrote:

...

+ //int buf_end = (buf + buf_size);

?

Hmmm.. missed a comment there. Will ofcourse remove it.

...

+ int red_channel .= -1;

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

How did that get 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; }

Yes, would make it a function when I was sure that the method was correct.

...

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

Will try to make it better.

...

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

You're right. Will change it.

...

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

No, and I know this since the files I use are actually stored as "B", "G", "R". Remember there is a continue inside each if. I know it's ugly.

...

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

Will make it inside another if.

...

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

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

Thanks. Will change that.

...

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

See above comment.

...

+ uint16_t* ptr_x = (uint16_t*)ptr;

The * is placed inconsistently.

Will change it.

...

+ const uint32_t line_offset = AV_RL64(buf);

bytestream_get_le64

Will use that instead.

...

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

In this revision I focused more on the header parser. Will ofcourse make more checks here.

...

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

Will use that instead.

...

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

True, was a copy from earlier in the code so didn't think about that.

Incrementing ptr_x is pointless.

...

+ // 16-bit + case 1:

basically the same comments.

-- Best Regards Jimmy Christensen Developer Ghost A/S

On 2009-07-08 08:56, Jimmy Christensen wrote:

Most things should be fixed now. Added a lot more checks for buffer overruns.

ping?

On 2009-07-15 10:22, Reimar D?ffinger wrote:

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.

Will move it into a new function.

...

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

These should also be declared locally where they are used.

These are actually assigned in the header parser and read when reading the pixels. So they can't really be moved.

...

+ 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

Will do that.

...

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

Will look into that.

...

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

Hmm.. if you mean if I could replace "ptr_tmp + 1 < buf" with "buf - ptr_tmp >= 19" it won't work like intended. Channels can be called something more than 1 character. Ofcourse you can argue that a channel does have to have atleast 1 character. So you are right that it can be replaced with "buf - ptr_tmp >= 19".

...

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

Hmmm... you're right, After I thought about it I realized that it doesn't matter which channels are after the red, green and blue channels. channel_iter was to indicate how large the line for the channels would be. Eg. if there are 8 channels, the size of the line would be 8 * channel size * xdelta. Even that is not sufficient since channels can be of different sizes, so will need to create a variable in the header parser indicating the total sizes of the channels.

On 2009-07-15 14:00, Reimar D?ffinger wrote:

On Wed, Jul 15, 2009 at 01:05:31PM +0200, Jimmy Christensen wrote:

...

On 2009-07-15 10:22, Reimar D?ffinger wrote:

...

...

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

Hmm.. if you mean if I could replace "ptr_tmp + 1< buf" with "buf - ptr_tmp>= 19" it won't work like intended. Channels can be called something more than 1 character. Ofcourse you can argue that a channel does have to have atleast 1 character. So you are right that it can be replaced with "buf - ptr_tmp>= 19".

Huh? I am talking about the continue statements and nothing else. In the inner loop you have if (!strcmp(...)) { ... continue; }

Because as you realized otherwise it won't work if the variable are in a different order. But exactly the same issue applies to the outer loop, e.g. if displayWindow came before dataWindow.

I misunderstood your comment, but you're absolutely right. Will change that. Thanks.

...

...

...

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

Hmmm... you're right, After I thought about it I realized that it doesn't matter which channels are after the red, green and blue channels. channel_iter was to indicate how large the line for the channels would be. Eg. if there are 8 channels, the size of the line would be 8 * channel size * xdelta. Even that is not sufficient since channels can be of different sizes, so will need to create a variable in the header parser indicating the total sizes of the channels.

No you certainly do not need an extra variable, that's actually a sure way you get a broken check. You need to make sure any data you try access is inside the buffer, you necessarily _must_ have all information necessary to check this right at the place where the access is done because otherwise the CPU wouldn't have enough information to calculate the address either.

What I meant was an extra variable and ofcourse the check. The current code expects all channels to be of the same size and will look in the wrong place if there is an alpha channel of 8-bit before the 16-bit R, G and B channels.

On Tue, Jul 21, 2009 at 08:45:10PM +0200, Jimmy Christensen wrote:

New patch. Re-arranged some small things. Now supports variable channel size for all other than the R, G and B channels which are expected to be of the same size.

Also added a few more checks for possible buffer overruns. [... +static inline uint16_t exr_flt2uint(uint32_t v) +{ + int exp = v >> 23;

+ if (v & 0x80000000) + return 0; + if (exp <= 127+7-24) // we would shift out all bits anyway + return 0;

if v where signed then the first if would be unneded

+ if (exp >= 127) + return 0xffff; + v &= 0x007fffff; + return (v+(1<<23)) >> (127+7-exp); +} [...] +static int decode_frame(AVCodecContext *avctx, + void *data, + int *data_size, + AVPacket *avpkt) +{ + const uint8_t *buf = avpkt->data; + 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; + int xmin = -1; + int xmax = -1; + int ymin = -1; + int ymax = -1; + int xdelta = -1; + + int red_channel = -1; + int green_channel = -1; + int blue_channel = -1; + int bits_per_color_id = -1;

+ int red_channel_offset = 0; + int green_channel_offset = 0; + int blue_channel_offset = 0; + int current_channel_offset = 0;

nitpick: int red_channel_offset = 0; int green_channel_offset = 0; int blue_channel_offset = 0;

+ + static int bytes_per_color_table[3] = {1, 2, 4};

missing const

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

+ if (buf_end - buf < 10) { + av_log(avctx, AV_LOG_ERROR, "Too short header to parse\n"); + return -1; + } else { + // Parse the header + while (buf[0] != 0x0) {

there is somethig wrong with the indention

+ int variable_buffer_data_size; + // Process the channel list + if (buf_end - buf >= 20 && !strncmp(buf, "channels", 9)) { + buf += 9; + if (strncmp(buf, "chlist", 7)) { + av_log(avctx, AV_LOG_ERROR, "Unknown type for channels\n"); + return -1; + } + buf += 7; + variable_buffer_data_size = bytestream_get_le32(&buf); + if (buf_end - buf <= variable_buffer_data_size) { + av_log(avctx, AV_LOG_ERROR, "Incomplete channel list\n"); + return -1;

have you considered negative variable_buffer_data_size ?

+ } else { + const uint8_t *ptr_tmp = buf; + int channel_iter = -1; + int current_bits_per_color_id = 0; + buf += variable_buffer_data_size; + while (ptr_tmp + 1 < buf) {

i think readability woud be better if there was an empty line in there somewhere

+ channel_iter++; + if (buf - ptr_tmp >= 19 && !strncmp(ptr_tmp, "R", 2)) { + ptr_tmp += 2; + red_channel = channel_iter; + current_bits_per_color_id = bytestream_get_le32(&ptr_tmp); + if (current_bits_per_color_id > 2) { + av_log(avctx, AV_LOG_ERROR, "Unkown color format : %d\n", bits_per_color_id); + return -1; + } + red_channel_offset = current_channel_offset; + current_channel_offset += bytes_per_color_table[current_bits_per_color_id]; + bits_per_color_id = current_bits_per_color_id; + ptr_tmp += 12; + continue; + } + if (buf - ptr_tmp >= 19 && !strncmp(ptr_tmp, "G", 2)) { + ptr_tmp += 2; + green_channel = channel_iter; + current_bits_per_color_id = bytestream_get_le32(&ptr_tmp); + if (current_bits_per_color_id > 2) { + av_log(avctx, AV_LOG_ERROR, "Unkown color format : %d\n", bits_per_color_id); + return -1; + } + green_channel_offset = current_channel_offset; + current_channel_offset += bytes_per_color_table[current_bits_per_color_id]; + ptr_tmp += 12; + continue; + }

code duplication [...]

+ // Skip variable name + while (buf++ < buf_end) { + if (buf[0] == 0x0) + break; + } + // Skip variable type + while (buf++ < buf_end) { + if (buf[0] == 0x0) + break; + }

code duplication [...]

+ if (red_channel != -1 && blue_channel != -1 && green_channel != -1) { + if (s->picture.data[0]) + avctx->release_buffer(avctx, &s->picture); + if (avcodec_check_dimensions(avctx, w, h)) + return -1; + 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 || xdelta < 0) { + av_log(avctx, AV_LOG_ERROR, "Wrong sizing or missing size information\n"); + return -1; + }

it looks like these variables should be unsigned [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB When the tyrant has disposed of foreign enemies by conquest or treaty, and there is nothing more to fear from them, then he is always stirring up some war or other, in order that the people may require a leader. -- Plato

On Wed, Jul 29, 2009 at 07:50:47AM +0200, Jimmy Christensen wrote:

On 2009-07-29 00:46, Michael Niedermayer wrote:

...

On Tue, Jul 21, 2009 at 08:45:10PM +0200, Jimmy Christensen wrote:

...

New patch. Re-arranged some small things. Now supports variable channel size for all other than the R, G and B channels which are expected to be of the same size.

Also added a few more checks for possible buffer overruns. [... +static inline uint16_t exr_flt2uint(uint32_t v) +{ + int exp = v>> 23;

...

+ if (v& 0x80000000) + return 0; + if (exp<= 127+7-24) // we would shift out all bits anyway + return 0;

if v where signed then the first if would be unneded

Not sure it would yield the expected result. If it's negative it needs to output 0 and not a positive number.

If v is signed, exp will be < for negative numbers. So yes it would work.

...

...

+ channel_iter++; + if (buf - ptr_tmp>= 19&& !strncmp(ptr_tmp, "R", 2)) { + ptr_tmp += 2; + red_channel = channel_iter; + current_bits_per_color_id = bytestream_get_le32(&ptr_tmp); + if (current_bits_per_color_id> 2) { + av_log(avctx, AV_LOG_ERROR, "Unkown color format : %d\n", bits_per_color_id); + return -1; + } + red_channel_offset = current_channel_offset; + current_channel_offset += bytes_per_color_table[current_bits_per_color_id]; + bits_per_color_id = current_bits_per_color_id; + ptr_tmp += 12; + continue; + } + if (buf - ptr_tmp>= 19&& !strncmp(ptr_tmp, "G", 2)) { + ptr_tmp += 2; + green_channel = channel_iter; + current_bits_per_color_id = bytestream_get_le32(&ptr_tmp); + if (current_bits_per_color_id> 2) { + av_log(avctx, AV_LOG_ERROR, "Unkown color format : %d\n", bits_per_color_id); + return -1; + } + green_channel_offset = current_channel_offset; + current_channel_offset += bytes_per_color_table[current_bits_per_color_id]; + ptr_tmp += 12; + continue; + }

code duplication

Could probably make it less duplicate code, but these steps needs to be done only for R, G and B and needs to assign specific variables accordingly.

That's what functions are there for. I think in quite a few places readability could be improved quite a bit by creating a well named function, but I think you so far ignored all my suggestions in that regard.

+ if (buf_end - buf < 10) { + av_log(avctx, AV_LOG_ERROR, "Too short header to parse\n"); + return -1; + } else {

Pointless else, that was basically the whole point why I suggested changing the order of this. If you are more used to languages like Java etc., think of this as a poor-man's exception.

+ if (buf_end - buf <= variable_buffer_data_size) { + av_log(avctx, AV_LOG_ERROR, "Incomplete channel list\n"); + return -1; + } else {

Same.

+ for (int i = 0; i < 2; i++) {

That will break gcc 2.95 compilation. Declare i normally.

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

Pointless () around buf_end - buf, also I still think this check is duplicated all over the place and could be factored out at worst by using a macro.

+ if (buf < buf_end) { + // Move pointer out of header + buf++; + } else { + av_log(avctx, AV_LOG_ERROR, "Incomplete file\n"); + return -1; + }

As in the other code, this is more compact and IMO readable by packing the execptional case into the lower indentation level, i.e.

+ if (buf >= buf_end) { + av_log(avctx, AV_LOG_ERROR, "Incomplete file\n"); + return -1; + } + buf++;

But since the input buffer is guaranteed to be padded by a few extra bytes, I think this check is not really necessary at all.

+ // 32-bit + case 2: + // Process the actual lines + for (y = ymin; y <= ymax; y++) { + uint16_t *ptr_x = (uint16_t*)ptr; + if (buf_end - buf > 8) { + const uint32_t line_offset = bytestream_get_le64(&buf) + 8; + // Check if the buffer has the required bytes needed from the offset + if ((uint32_t)(buf_end - avpkt->data) >= line_offset + (xdelta) * current_channel_offset) { + const uint8_t *red_channel_buffer = avpkt->data + line_offset + (xdelta) * red_channel_offset; + const uint8_t *green_channel_buffer = avpkt->data + line_offset + (xdelta) * green_channel_offset; + const uint8_t *blue_channel_buffer = avpkt->data + line_offset + (xdelta) * blue_channel_offset; + + // Zero out the start if xmin is not 0 + memset(ptr_x, 0, xmin*6); + ptr_x += xmin*3; + + for (x = 0; x < xdelta; x++) { + *ptr_x++ = exr_flt2uint(bytestream_get_le32(&red_channel_buffer)); + *ptr_x++ = exr_flt2uint(bytestream_get_le32(&green_channel_buffer)); + *ptr_x++ = exr_flt2uint(bytestream_get_le32(&blue_channel_buffer)); + } + + // 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; + + // Move to next line + ptr += stride; + } + } + } + break; + // 16-bit + case 1: + // Process the actual lines + for (y = ymin; y <= ymax; y++) { + uint16_t *ptr_x = (uint16_t*)ptr; + if (buf_end - buf > 8) { + const uint32_t line_offset = bytestream_get_le64(&buf) + 8; + // Check if the buffer has the required bytes needed from the offset + if ((uint32_t)(buf_end - avpkt->data) >= line_offset + (xdelta) * current_channel_offset) { + const uint8_t *red_channel_buffer = avpkt->data + line_offset + (xdelta) * red_channel_offset; + const uint8_t *green_channel_buffer = avpkt->data + line_offset + (xdelta) * green_channel_offset; + const uint8_t *blue_channel_buffer = avpkt->data + line_offset + (xdelta) * blue_channel_offset; + + // Zero out the start if xmin is not 0 + memset(ptr_x, 0, xmin*6); + ptr_x += xmin*3; + + for (x = 0; x < xdelta; x++) { + *ptr_x++ = exr_halflt2uint(bytestream_get_le16(&red_channel_buffer)); + *ptr_x++ = exr_halflt2uint(bytestream_get_le16(&green_channel_buffer)); + *ptr_x++ = exr_halflt2uint(bytestream_get_le16(&blue_channel_buffer)); + } + + // 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; + + // Move to next line + ptr += stride; + } + } + } + break;

These two are almost the same except for a factor 2 in some formulas and the innermost loop. I think the performance impact of merging these into one piece of code should be small enough that it would be reasonable to avoid the duplication...

On Wed, Jul 29, 2009 at 02:39:08PM +0200, Jimmy Christensen wrote:

...

That's what functions are there for. I think in quite a few places readability could be improved quite a bit by creating a well named function, but I think you so far ignored all my suggestions in that regard.

It's not as much as I ignored them. I tried making them into functions but since it needs to exit with a return -1, I didn't know how to make the function make the main function return -1.

E.g. If (get_value(...) < 0) return -1; That still duplicates the if, but should still be less code, at least the av_log is centralized. You could then consider if its reasonable to wrap even that into a macro.

...

Pointless () around buf_end - buf, also I still think this check is duplicated all over the place and could be factored out at worst by using a macro.

AFAIK Michael Niedermayer hates macros :)

I don't think so at all. But macros avoid code duplication only at the source level, not the code level and have some usability issues that make a static inline function often preferable over a macro (or for non-speed-critical code like here even without the inline).

On Wed, Jul 29, 2009 at 03:09:55PM +0200, Jimmy Christensen wrote:

On 2009-07-29 15:06, Reimar D?ffinger wrote:

...

On Wed, Jul 29, 2009 at 02:39:08PM +0200, Jimmy Christensen wrote:

...

...

That's what functions are there for. I think in quite a few places readability could be improved quite a bit by creating a well named function, but I think you so far ignored all my suggestions in that regard.

It's not as much as I ignored them. I tried making them into functions but since it needs to exit with a return -1, I didn't know how to make the function make the main function return -1.

E.g. If (get_value(...)< 0) return -1; That still duplicates the if, but should still be less code, at least the av_log is centralized. You could then consider if its reasonable to wrap even that into a macro.

This was also pretty much the conclusion that I got to. For most of the functions it wouldn't make the code much less.

But it will also give the code a name and a doxygen description. Also, it will help you avoid doing it one way in five place and a different way in a another (as you have done a few times), leaving in a few years everyone scared to change the code because the can't know if that case was done differently on purpose.

On 2009-09-08 09:20, Jimmy Christensen wrote:

On 2009-07-29 15:22, Reimar D?ffinger wrote:

...

On Wed, Jul 29, 2009 at 03:09:55PM +0200, Jimmy Christensen wrote:

...

On 2009-07-29 15:06, Reimar D?ffinger wrote:

...

On Wed, Jul 29, 2009 at 02:39:08PM +0200, Jimmy Christensen wrote:

...

...

That's what functions are there for. I think in quite a few places readability could be improved quite a bit by creating a well named function, but I think you so far ignored all my suggestions in that regard.

It's not as much as I ignored them. I tried making them into functions but since it needs to exit with a return -1, I didn't know how to make the function make the main function return -1.

E.g. If (get_value(...)< 0) return -1; That still duplicates the if, but should still be less code, at least the av_log is centralized. You could then consider if its reasonable to wrap even that into a macro.

This was also pretty much the conclusion that I got to. For most of the functions it wouldn't make the code much less.

But it will also give the code a name and a doxygen description. Also, it will help you avoid doing it one way in five place and a different way in a another (as you have done a few times), leaving in a few years everyone scared to change the code because the can't know if that case was done differently on purpose.

Been a long time since I submitted an update on this so here goes :

Re-designed a few things and made more things into functions. Don't know what you guys think of the added exr.h header, but I beleive it's the right thing to do for eg. future support for compression types.

Made some small fixes inside get_rgb_channel()...

On 2009-09-08 16:20, Reimar D?ffinger wrote:

On Tue, Sep 08, 2009 at 03:30:26PM +0200, Jimmy Christensen wrote:

...

+static const unsigned int bytes_per_color_table[3] = { + 1, 2, 4 +};

Currently that is of course the same as 1<< n. Anyway the main point is a) this is used only in one function so better declare it there b) should use uint8_t so it does not take up so much space.

Fixed in rev17.

...

+static inline 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)

I think all functions lack doxygen documentation. Also since speed is not very relevant for header parsing most of these probably should not be inline

Have added doygen documentation to all functions. And have removed inline from them.

...

+{ + if (buf_end - buf>= minimum_length&& !strncmp(buf, value_name, strlen(value_name))) { + buf += strlen(value_name)+1; + if (!strncmp(buf, value_type, strlen(value_type))) + return 1; + av_log(NULL, AV_LOG_ERROR, "Unknown data type for header variable %s\n", value_name); + } + return 0;

Please don't do this, for return values either use 1 good, 0 error or 0 good,< 0 error

I'm confused. It already does : 1 good, 0 error

...

+ unsigned int variable_buffer_data_size = bytestream_get_le32(buf); + if (buf_end - *buf<= variable_buffer_data_size) { + av_log(NULL, AV_LOG_ERROR, "Incomplete header\n"); + return 0; + } else { + return variable_buffer_data_size; + }

You return in the if path, there is no need for "else".

Fixed in rev17.

...

+ if (!strncmp(*buf, "R", 2)) + s->red_channel = channel_iter; + if (!strncmp(*buf, "G", 2)) + s->green_channel = channel_iter; + if (!strncmp(*buf, "B", 2)) + s->blue_channel = channel_iter;

strcmp, no need for strncmp.

Fixed in rev17.

...

+ if (channel_iter == s->red_channel || + channel_iter == s->green_channel || + channel_iter == s->blue_channel) { + *buf += 2; + current_bits_per_color_id = bytestream_get_le32(buf); + if (current_bits_per_color_id> 2) + return -1; + if (channel_iter == s->red_channel) { + s->bits_per_color_id = current_bits_per_color_id; + s->red_channel_offset = *current_channel_offset; + } + if (channel_iter == s->green_channel) + s->green_channel_offset = *current_channel_offset; + if (channel_iter == s->blue_channel) + s->blue_channel_offset = *current_channel_offset;

This seems very convoluted, why is red special? And are you misusing those "channel_iter == s->red_channel" tests to check which one you've just assigned?

Red is just the first one and I'm using that to determine the bit depth of the file. I will admit it's not the best solution since you in theory could have files which have a 16-bit red channel and 32-bit green and blue channels. However I don't see any reason for such things and I'm not so sure that any software will actually output files likes this.

This might be nicer with {red,green,blue}_channel_offset instead being and array channel_offsets and maybe enum {RED_CHANNEL, GREEN_CHANNEL, BLUE_CHANNEL} (or just 0 - 2 manually).

Good idea. Will do that instead.

...

+ if (*buf< channel_list_end) { + while (*buf[0] != 0x0) + *buf += 1; + *buf += 1;

e.g. while (bytestream_get(&buf)) /* skip */; More importantly though this still misses a check so it will not overrun beyond the end of the buffer.

Fixed in rev17.

...

+ current_bits_per_color_id = bytestream_get_le32(buf); + if (current_bits_per_color_id> 2) + return -1; + *current_channel_offset += bytes_per_color_table[current_bits_per_color_id]; + *buf += 12; + }

This duplicates code from the RGB case.

Fixed in rev17.

...

+ if (buf_end - buf< 10) { + av_log(avctx, AV_LOG_ERROR, "Too short header to parse\n"); + return -1; + } else {

Return in if -> do not add else to avoid unnecessary indentation and to avoid cluttering the "default" code path due to error handling.

Fixed in rev17.

...

+ channel_list_end += variable_buffer_data_size; + while (channel_list_end>= buf + 19) {

Thanks to padding it is probably not wrong, but it is inconsistent when channel_list_end - buf>= 19 is used elsewhere.

Is it?

...

+ if (get_rgb_channel(&buf, + channel_list_end, + s, + channel_iter, +¤t_channel_offset) == -1) {

...

+ // Process the dataWindow variable + if (check_header_variable(buf, buf_end, "dataWindow", "box2i", 31) == 1) {

Which so specific (i.e. == -1 and == 1?) If someone extended those functions to return other value, would it really make sense? Or in other words, it should be if (get_rgb_channel()< 0) and if (check_header_variable())

Good point, fixed in rev17.

...

+ while (buf++< buf_end) + if (buf[0] == 0x0) + break;

That skips a string just like another piece of code but checks for the buffer end. Maybe this should just be a function, then you can't forgot some of the checks in half of the places.

...

+ if (buf< buf_end) { + // Move pointer out of header + buf++; + } else { + av_log(avctx, AV_LOG_ERROR, "Incomplete file\n"); + return -1; + }

Huh? I thought you had the "fixed" once already: if (buf>= buf_end) { av_log(); return -1; } buf++;

Must have gotten mixed up when I redid some of the code. Thanks. Fixed in rev17.

...

+ if (s->red_channel != -1&& + s->blue_channel != -1&& + s->green_channel != -1) { + if (s->picture.data[0]) + avctx->release_buffer(avctx,&s->picture); + if (avcodec_check_dimensions(avctx, w, h)) + return -1; + 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 || + xdelta == ~0) { + 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; + } + } else { + if (s->red_channel == -1) + av_log(avctx, AV_LOG_ERROR, "Missing red channel\n"); + if (s->green_channel == -1) + av_log(avctx, AV_LOG_ERROR, "Missing green channel\n"); + if (s->blue_channel == -1) + av_log(avctx, AV_LOG_ERROR, "Missing blue channel\n"); + return -1; + }

As always, where possible do not indent the non-error code-path.

Fixed in rev17. Thanks for the review. Will post rev17 when I've fixed the comments in the other posts.

On Tue, Sep 08, 2009 at 07:47:51PM +0200, Reimar D?ffinger wrote:

On Tue, Sep 08, 2009 at 03:30:26PM +0200, Jimmy Christensen wrote:

...

+ switch (s->bits_per_color_id) { + // 32-bit + case 2: + avctx->pix_fmt = PIX_FMT_RGB48LE; + break; + // 16-bit + case 1: + avctx->pix_fmt = PIX_FMT_RGB48LE; + break;

I missed that, this obviously can't be right, you write the uint16_t values directly, thus in native format - not little-endian.

I.e. PIX_FMT_RGB48 Also it should of course not duplicate the code, i.e.

case 1: // 16-bit half-float input case 2: // 32-bit float input avctx->pix_fmt = PIX_FMT_RGB48; break;

And then I'd say the comments should be removed and instead proper enums/defines be used instead of 1, 2...

...

+ for (x = 0; x < xdelta; x++) { + *ptr_x++ = exr_flt2uint(bytestream_get_le32(&red_channel_buffer)); + *ptr_x++ = exr_flt2uint(bytestream_get_le32(&green_channel_buffer)); + *ptr_x++ = exr_flt2uint(bytestream_get_le32(&blue_channel_buffer)); + }

Not that this must used code suitable for unaligned access, thus e.g. on

Ouch. I meant: Note: bytestream_get_le32 must use code suitable for unaligned access.

Sparc *ptr_x++ = exr_flt2uint(le2me_32(*(uint32_t *)red_channel_buffer)); will be faster.

But I just realize that line_offset is in bytes, so you can't do that optimization. Also in the same area some more comments:

+ uint16_t *ptr_x = (uint16_t*)ptr;

Missing space before *

+ 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 ((uint32_t)(buf_end - avpkt->data) >= line_offset + xdelta * current_channel_offset) {

Silently skipping the data is not very nice and user-friendly. Particularly since it will give very strange artifacts since you do not increase ptr this means a single bad line_offset will shift all of the remaining image one line up... Also, what is that (uint32_t) cast supposed to do? It breaks files > 4GB on 64 bit systems without a good reason.

On 2009-09-08 19:47, Reimar D?ffinger wrote:

On Tue, Sep 08, 2009 at 03:30:26PM +0200, Jimmy Christensen wrote:

...

+ switch (s->bits_per_color_id) { + // 32-bit + case 2: + avctx->pix_fmt = PIX_FMT_RGB48LE; + break; + // 16-bit + case 1: + avctx->pix_fmt = PIX_FMT_RGB48LE; + break;

I missed that, this obviously can't be right, you write the uint16_t values directly, thus in native format - not little-endian.

You're right. Fixed in rev17.

...

+ for (x = 0; x< xdelta; x++) { + *ptr_x++ = exr_flt2uint(bytestream_get_le32(&red_channel_buffer)); + *ptr_x++ = exr_flt2uint(bytestream_get_le32(&green_channel_buffer)); + *ptr_x++ = exr_flt2uint(bytestream_get_le32(&blue_channel_buffer)); + }

Not that this must used code suitable for unaligned access, thus e.g. on Sparc *ptr_x++ = exr_flt2uint(le2me_32(*(uint32_t *)red_channel_buffer)); will be faster.

On 2009-09-08 19:57, Reimar D?ffinger wrote:

On Tue, Sep 08, 2009 at 03:30:26PM +0200, Jimmy Christensen wrote:

...

+static inline uint16_t exr_flt2uint(uint32_t v) +{ + int exp = v>> 23; + if (v& 0x80000000) + return 0; + if (exp<= 127+7-24) // we would shift out all bits anyway + return 0; + if (exp>= 127) + return 0xffff; + v&= 0x007fffff; + return (v+(1<<23))>> (127+7-exp);

Indentation is off.

Thanks. Fixed in rev17.

Also, if you use int32_t as type for v, you can remove the (v& 0x80000000) case, since exp will then be< 0 for negative numbers (should be documented with a comment of course).

I would love to remove the line, but I have no idea on how to document this since you are alot more in to this. Perhaps you could write what I should put in? Or perhaps send in a patch after it's been approved? Anyway here's the new patch Again, thanks for the review.

On 2009-09-13 19:14, Jimmy Christensen wrote:

Hmm... mail still not utf-8. Will try again for rev19.

Ehhmm.. nevermind seems to be fixed now? -- Best Regards Jimmy Christensen Developer Ghost A/S

On 2009-09-13 20:35, Reimar D?ffinger wrote:

On Sun, Sep 13, 2009 at 05:58:48PM +0200, Jimmy Christensen wrote: Replace

...

+static inline uint16_t exr_flt2uint(uint32_t v) +{ + int exp = v>> 23; + if (v& 0x80000000) + return 0; + if (exp<= 127+7-24) // we would shift out all bits anyway + return 0;

by e.g.

...

+static inline uint16_t exr_flt2uint(int32_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. + if (exp<= 127+7-24) // we would shift out all bits anyway + return 0;

Thanks, will change to that in rev19.

...

+ * @return zero if variable is invalid

That is what had me confused in my previous comments: It is 0 both when it is invalid (due to type mismatch) and when the name just does not match.

...

+ if (buf_end - buf>= minimum_length&& !strncmp(buf, value_name, strlen(value_name))) { + buf += strlen(value_name)+1;

Hmm.. Are you sure that strncmp is correct and you shouldn't actually be using strcmp? It looks to me like the string in buf must be 0-terminated, then strcmp would be correct.

Yes, it needs to be 0-terminated. Will change it to strcmp in rev19. Same goes for the variable type string. Changed for that aswell.

...

+ unsigned int variable_buffer_data_size = bytestream_get_le32(buf); + if (buf_end - *buf<= variable_buffer_data_size) { + av_log(NULL, AV_LOG_ERROR, "Incomplete header\n"); + return 0; + }

Why is == an error? At least the variable size data still fits in in the == case...

True. Will remove the == case. Although if this was the case there would be no data to have in the actual image in which case == would also be wrong :)

...

+ const uint8_t bytes_per_color_table[3] = { + 1, 2, 4 + }; + unsigned int current_bits_per_color_id = 0; + + if (!strcmp(*buf, "R")) + s->red_channel = channel_iter; + if (!strcmp(*buf, "G")) + s->green_channel = channel_iter; + if (!strcmp(*buf, "B")) + s->blue_channel = channel_iter; + + while (bytestream_get_byte(buf)&& *buf< channel_list_end) + continue; /* skip */ + + current_bits_per_color_id = bytestream_get_le32(buf); + if (current_bits_per_color_id> 2) + return -1; + + if (channel_iter == s->red_channel || + channel_iter == s->green_channel || + channel_iter == s->blue_channel) { + if (channel_iter == s->red_channel) { + s->bits_per_color_id = current_bits_per_color_id; + s->channel_offsets[0] = *current_channel_offset; + } + if (channel_iter == s->green_channel) + s->channel_offsets[1] = *current_channel_offset; + if (channel_iter == s->blue_channel) + s->channel_offsets[2] = *current_channel_offset; + } + *current_channel_offset += bytes_per_color_table[current_bits_per_color_id]; + *buf += 12; + return 1;

You misunderstood my comment on that. All those comparisons against channel_iter are completely obfuscating things. I guess you could do something like:

int channel_index = -1; if (!buf[1]) { // only handle 1-character channel strings so far switch (buf[0]) { case 'R': channel_index++; case 'G': channel_index++; case 'B': channel_index++; } } [... while an current_bits_per_color_id as above ...] if (channel_index>= 0) { s->bits_per_color_id = current_bits_per_color_id; s->channel_offsets[channel_index] = *current_channel_offset; } *current_channel_offset[... rest identical...]

You do not need channel_iter, nor do you need s->red_channel etc., though you might want to know if a channel is missing. You can either do that with a bit mask, or you could initialize channel_offsets to something certainly invalid like -1.

IMHO that's optimizing a bit too much. With the current implementation it would be much easier to have the decoder use channel names other than just R, G and B. If eg. I wanted to expand the decoder to also decoder left eye of a OpenSXR file (OpenEXR stereo file). Their names are left.R, left.G and left.B. Optimizing for only 1 character channel names seems a bit too much, especially since it's only in the header parser and is that much speed important. You're right that I might not need channel_iter since I added current_channel_offset. Removed channel_iter and redid the code similar to yours.

...

+ if (buf_end - buf< 10) { + while (channel_list_end>= buf + 19) { + if (buf_end - buf> 9) { + if (buf_end - buf>= 5) {

Does this clarify the inconsistency I pointed out before? The checks generally are "end - position> ..." but that one is of the kind "end> position + ..."

Found it now and fixed in rev19.

...

+ const uint64_t line_offset = bytestream_get_le64(&buf) + 8; + // Check if the buffer has the required bytes needed from the offset + if ((buf_end - avpkt->data)< line_offset + xdelta * current_channel_offset) {

Pointless ().

Fixed.

But somewhere along the line you changed get_le32 to get_le64 and now the right hand side can overflow. Another problem is that line_offset + xdelta * current_channel_offset is not even the right term, given that a malicious file could give r, g and b different bit depths. In that case the correct term would have to be line_offset + xdelta * (FFMAX3(s->channel_offsets[0], s->channel_offsets[1], s->channel_offsets[2]) + (s->bits_per_color_id == 2 ? 4 : 2)).

You're right. No file should have different channel types anyway, so will check for that instead and error out if there is a mismatch. Also added better error output.

I'd suggest to just verify that all bits_per_color_id are identical at the point where you read that value from the file. Also it seems to me that buf_end - avpkt->data would be simpler written as avpkt->size?

Fixed.

I'd then extend the verification of xdelta to include if (xdelta> avptk->size / current_channel_offset) error; and here do

Good point. Added.

if (line_offset> avpkt->size - xdelta * current_channel_offset) error;

Simplifications may be possible though.

On 2009-09-14 14:21, Reimar D?ffinger wrote:

On Mon, Sep 14, 2009 at 01:59:57PM +0200, Jimmy Christensen wrote:

...

...

...

+ if (!strcmp(*buf, "R")) + s->red_channel = channel_iter; + if (!strcmp(*buf, "G")) + s->green_channel = channel_iter; + if (!strcmp(*buf, "B")) + s->blue_channel = channel_iter; + + while (bytestream_get_byte(buf)&& *buf< channel_list_end) + continue; /* skip */ + + current_bits_per_color_id = bytestream_get_le32(buf); + if (current_bits_per_color_id> 2) + return -1; + + if (channel_iter == s->red_channel || + channel_iter == s->green_channel || + channel_iter == s->blue_channel) { + if (channel_iter == s->red_channel) { + s->bits_per_color_id = current_bits_per_color_id; + s->channel_offsets[0] = *current_channel_offset; + } + if (channel_iter == s->green_channel) + s->channel_offsets[1] = *current_channel_offset; + if (channel_iter == s->blue_channel) + s->channel_offsets[2] = *current_channel_offset; + } + *current_channel_offset += bytes_per_color_table[current_bits_per_color_id]; + *buf += 12; + return 1;

You misunderstood my comment on that. All those comparisons against channel_iter are completely obfuscating things. I guess you could do something like:

int channel_index = -1; if (!buf[1]) { // only handle 1-character channel strings so far switch (buf[0]) { case 'R': channel_index++; case 'G': channel_index++; case 'B': channel_index++; } } [... while an current_bits_per_color_id as above ...] if (channel_index>= 0) { s->bits_per_color_id = current_bits_per_color_id; s->channel_offsets[channel_index] = *current_channel_offset; } *current_channel_offset[... rest identical...]

You do not need channel_iter, nor do you need s->red_channel etc., though you might want to know if a channel is missing. You can either do that with a bit mask, or you could initialize channel_offsets to something certainly invalid like -1.

IMHO that's optimizing a bit too much. With the current implementation it would be much easier to have the decoder use channel names other than just R, G and B. If eg. I wanted to expand the decoder to also decoder left eye of a OpenSXR file (OpenEXR stereo file). Their names are left.R, left.G and left.B. Optimizing for only 1 character channel names seems a bit too much, especially since it's only in the header parser and is that much speed important.

Well you still got the basic idea. Except that you are suddenly using strncmp again instead of strcmp.

I wasn't sure if it was safe to use only strcmp. Will change it for next revision if more changes are needed. (otherwise just posting a new one with that change).

Ping? Seems it should be almost done? -- Best Regards Jimmy Christensen Developer Ghost A/S

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.

On Sat, Apr 24, 2010 at 05:59:46PM +0200, Jimmy Christensen wrote: [...]

...

[...]

...

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

they are partly redundant, remove the redundant checks. also the new patch now is containing things that can be exploited that i think where not in the previous

...

...

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

its half a year since i reviewed this, you dont expect me to remember do you? anyway, ive looked again and the first line i looked at looks exploitable this code needs a complete review by the author against security issues not fixing just the few i found. also make sure you understand which sub expressions have which type in C and at which point they overflow [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Awnsering whenever a program halts or runs forever is On a turing machine, in general impossible (turings halting problem). On any real computer, always possible as a real computer has a finite number of states N, and will either halt in less than N cycles or never halt.

Argl, I should read my mails one time more often before sending... On Fri, Jul 03, 2009 at 02:16:43PM +0200, Reimar D?ffinger wrote:

What? variable_buffer_name is exactly 64 bit large and your code will

64 bytes of course.

And all that is no excuse to make a copy of it just to compare it. Or do you always when you read a book first copy it, then read the copy while having the copy lying next to you and then burn the copy you made?

"while having the original book lying next to you" was what I meant

On Fri, Jul 03, 2009 at 03:39:04PM +0200, Jimmy Christensen wrote:

...

What? variable_buffer_name is exactly 64 bit large and your code will happily try to write any amount of data into it when there is no 0 termination. In addition it is on the stack, so this is _trivial_ to exploit, anyone using this code could just make his passwords public and he'd probably be just as safe. Have you never heard of functions like strncpy (or the more useful strlcpy, since it is not portable in FFmpeg as av_strlcpy?). And all that is no excuse to make a copy of it just to compare it. Or do you always when you read a book first copy it, then read the copy while having the copy lying next to you and then burn the copy you made? Because that is more or less what your code does... Or to put in code what would make sense to do: if (buf_end - buf>= 9&& !strncmp(buf, "channels", 9)) { buf += 9; skip_buffertype(); ...

As long as you are using strcpy, strcmp, strlen or anything like that on external data, unverified data you are doing it wrong.

No I haven't heard about strncpy or strlcpy. But thanks for pointing me in the direction.

The problem is that I don't know what the next variable is called and how long it is. Also I don't know what all of them are called, since the creator have the option of making their own.

I wrote above how you can handle those you recognize. Those you don't you just have to skip.

I suppose this should work?

...

av_strlcpy(variable_buffer_name, buf, (buf_end - buf)); buf += strlen(variable_buffer_name)+1; av_strlcpy(variable_buffer_type, buf, (buf_end - buf)); buf += strlen(variable_buffer_type)+1;

Now you have added a length that avoids that you overread the on-heap input buffer but it is still possible to overwrite the on-stack variable variable_buffer_name. This has improved security not in any relevant way.

...

...

Keeping the variable_buffer_type is to be able to use it for later in the interpretation of the different variables. And it also serves for the purpose of knowing how many bytes to skip to get to the actual data.

Huh? It is _not_ used anywhere, not one bit and nowhere at all. Copying data into it is a complete waste of time.

I actually need it for 1 purpose. To get the length of the variable.

And you need to _copy_ it for that? Ok, some questions: Have you ever worked with C strings? Do you know what they are? Do you know how to find out the length of one (not! using strlen, strlen is unsafe)? If you know the answer to the last one, why do you think that finding the length requires that you make a copy?

Hi, On Thu, Jul 2, 2009 at 11:28 AM, Vitor Sessak<vitor1001 at gmail.com> wrote:

Jimmy Christensen wrote:

...

+ ? ?switch (bits_per_color_table[bits_per_color_id]) { + ? ? ? ?case 32: + ? ? ? ? ? ?avctx->pix_fmt = PIX_FMT_RGB48LE; + ? ? ? ? ? ?break; + ? ? ? ?case 16: + ? ? ? ? ? ?avctx->pix_fmt = PIX_FMT_RGB48LE; + ? ? ? ? ? ?break; + ? ? ? ?default: + ? ? ? ? ? ?av_log(avctx, AV_LOG_ERROR, "Unkown color format : %d\n", bits_per_color_id);

Same [av_log_missing_feature()]

"Invalid" != "Not implemented", right? This looks like "invalid" to me. Ronald

On 2009-07-02 17:35, Vitor Sessak wrote:

Ronald S. Bultje wrote:

...

Hi,

On Thu, Jul 2, 2009 at 11:28 AM, Vitor Sessak<vitor1001 at gmail.com> wrote:

...

Jimmy Christensen wrote:

...

+ switch (bits_per_color_table[bits_per_color_id]) { + case 32: + avctx->pix_fmt = PIX_FMT_RGB48LE; + break; + case 16: + avctx->pix_fmt = PIX_FMT_RGB48LE; + break; + default: + av_log(avctx, AV_LOG_ERROR, "Unkown color format : %d\n", bits_per_color_id); Same [av_log_missing_feature()]

"Invalid" != "Not implemented", right? This looks like "invalid" to me.

Quoting the first message in this thread:

...

8-bit integer is in theory supported aswell by the format, but I have yet to find software which supports writing it

So it is more a "sample welcome" case.

I think a better way of doing it is making an av_log_missing_feature for 8-bit and an invalid for anything above 0x2 which is 32-bit. 0x0 - 0x2 is the only ones mentioned in the documentation of the format. -- Best Regards Jimmy Christensen Developer Ghost A/S