ambiguity of signed types leads to bad read in psdinput.cpp on windows

[ghost]

trafficstars

Hi OIIO,

I was having trouble in a VS2010 build of OIIO loading RLE encoded PSD files. I tracked down the issue to an unsigned type usage in psdinput.cpp::decompress_packbits. Specifically the assignment of const char* src (which is itself a pointer referencing the std::string m_rle_buffer) to int16_t header as follows:

header = *src++;

In my project's environment this allowed values in access of 128 and did not wrap around to -127 and correctly decode the packets. If I changed the header to a "signed char" everything worked as expected. I was hoping to see if you could help shed some light on this particular part of the code. What is the best way to achieve this conversion safely?

Thanks, Jon

[lgritz]

Taking another look at this...

I definitely see something weird. The function looks like this (I'm leaving out the unimportant parts):

bool
PSDInput::decompress_packbits (const char *src, char *dst,
                 uint16_t packed_length, uint16_t unpacked_length)
{
    int32_t src_remaining = packed_length;
    int32_t dst_remaining = unpacked_length;
    int16_t header;
    int length;

    while (src_remaining > 0 && dst_remaining > 0) {
        header = *src++;
        src_remaining--;

        if (header == 128)    // <---- what???? can't happen
            continue;
        else if (header >= 0) {
            // (1 + n) literal bytes
            ...
        } else {
            // repeat byte (1 - n) times
            ...
        }
    }
    return true;
}

The header = *src++ is perfectly legal (assigning signed char to signed int16, but will result in a value on [-128,127]. Thus, I find the if (header == 128) clause very confusing, since it can never happen. This of course makes me doubt the rest of the logic.

The final else clause (when the value < 0) certainly implies that the characters are supposed to be signed.

So is that supposed to read if (header == -128)?

I didn't write this code, so I am really not sure what the correct fix is.

I don't really understand your comment, "In my project's environment this allowed values in access of 128 and did not wrap around to -127." Assigning a signed char to an int16_t should fully preserve the values. Can you clarify?

[ghost]

Hi lgritz, I am sorry I did not reply to this sooner. Evidently my github settings do not send notification emails.

I am not sure what the history of that code segment is, however I agree that the header check for 128 is disconcerting.

I don't think I was clear enough in my original write up of the issue. The way I "fixed" our copy of psdinput.cpp was to change the type for header from int16_t to signed char. Then opening RLE files worked again. I could not find documentation to back my findings, but with Windows 7 and Visual Studio 2010 that section of psdinput.cpp allowed header to reach values in excess of 127.

"Assigning a signed char to an int16_t should fully preserve the values".

This is not what I observed with VS2010.

Thanks, Jon

[lgritz]

I think #3589 addresses this issue. Since this has gone several years without comment, let's assume that is the fix and close it, and we can re-open if we discover another issue lurking here.