Use size_t for DWA buffersize calculation

[peterhillman]

Extend #894 to use size_t instead of int for all buffer size calculation in initializeBuffers, and tidy up unnecessary casts
Addresses https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=29653

Signed-off-by: Peter Hillman peterh@wetafx.co.nz

[meshula]

size_t is one of those scary types due to the way it's defined as being "at least 65535"... Thankfully SIZE_MAX on MSVC/64 is 2 ** 64, which is the platform where I would have expected a silly pitfall. I suggest adding a compile time assert that size_t is at least 64 bits on 64 bit platforms to ensure we don't have problems with files over 2**32 bytes in size. I'm sure that's the first thing a fuzzer is going to try :)

[lgritz]

@meshula I think that has to be the case, because size_t is guaranteed to be the biggest size of anything with a size() method. It has to be big enough to represent the biggest addressable memory size.

[meshula]

@lgritz That's what I expect as well, and observe with the three compilers I use. This caveat in the spec always makes me worry. It seems like a poison pill to let one compiler do something bonkers:

• size_t is commonly used for array indexing and loop counting. Programs that use other types, such as unsigned int, for array indexing may fail on, e.g. 64-bit systems when the index exceeds UINT_MAX or if it relies on 32-bit modular arithmetic.

This caveat bugs me so much, I use the sized types, like uint32_t everywhere in my code now except for when I'm dealing with std routines that explicitly return a size_t.

Note also the caveat of "anything with a size() method"... The code in question here is

size_t rleAmount = 2 * pixelCount * OPENEXR_IMF_NAMESPACE::pixelTypeSize (_channelData[chan].type);

I think that the type we want is ptrdiff_t, because that's the type that's guaranteed by spec to represent the biggest addressable memory size

[meshula]

I think ptrdiff_t is the more appropriate type here, per the conversation with @lgritz in the main thread.

[meshula]

Please note that I hit Approve before the conversation with Larry, and don't seem to be able to change Approve back to Comment, hence the discrepancy between comments and approval status...

[lgritz]

When I read that quote, what it appears to say is that size_t is safe for array indexing, but that other types, like regular unsigned int or uint32_t may not be, because they aren't guaranteed to be big enough to index into all possible arrays.

ptrdiff_t is the signed equivalent to size_t -- it's the thing that you know is guaranteed to safely hold the difference between any two addresses on that architecture.

[meshula]

Right --- uintptr_t would be the choice, not ptrdiff_t.

[peterhillman]

Well, size_t "can store the maximum size of a theoretically possible object of any type (including array)" and is the type passed to new, which is what it's used for here.

I switched those variables to size_t because they are ultimately copied to member variables which are of type size_t:

openexr/src/lib/OpenEXR/ImfDwaCompressor.h

Line 173 in dd87e54

size_t _packedAcBufferSize;

It appears those values ultimately get written into the files as Int64, so maybe changing these types to Int64, including in the header, would be safest? That would change the memory layout of the DwaCompressor object on some architectures, which I was trying to avoid.

In the reported fuzz issue, there's a file which caused a signed int overflow in the buffer size calculation. Maybe the answer is to use Int64 for the calculation, then test the value is less than limits<size_t>::max before casting to size_t in the new?

[meshula]

That would work. Going to back to the root issue, I do think we need to use a principled int64 all the way through.

[meshula]

looks solid