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

[Reimar Döffinger]

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;


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

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

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


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

> +            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 ().
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)).
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?
I'd then extend the verification of xdelta to include
if (xdelta > avptk->size / current_channel_offset) error;
and here do
if (line_offset > avpkt->size - xdelta * current_channel_offset) error;

Simplifications may be possible though.