Fix reading EXR images on 32bit Windows with OpenEXR 3.3

[ChristopherSchwartzXRITE]

This PR fixes an issue with atomic_compare_exchange_strong in chunk.c due to inappropriate type on 32 bit platforms.

Context:

When switching from OpenEXR 3.2.4 to OpenEXR 3.3.2, I noticed that reading image content from file (via ImfInputFile::readPixels(int, int)) failed most of the time (but not always) in 32 bit Windows.

I found that OpenEXR 3.3 apparently changed the reading routine to use extract_chunk_table from OpenEXRCore's chunk.c.

Here, two variables eptr and nptr are declared to be of type uintptr_t in

openexr/src/lib/OpenEXRCore/chunk.c

Line 568 in 8bc3fae

uintptr_t eptr = 0, nptr = 0;

However, in

openexr/src/lib/OpenEXRCore/chunk.c

Lines 644 to 647 in 8bc3fae

	if (!atomic_compare_exchange_strong (
	EXR_CONST_CAST (atomic_uintptr_t*, &(part->chunk_table)),
	&eptr,
	nptr))

, they are used in an atomic_compare_exchange_strong call as uint64_t* and uint64_t respectively.

See

openexr/src/lib/OpenEXRCore/chunk.c

Lines 38 to 47 in 8bc3fae

	static inline int
	atomic_compare_exchange_strong (
	uint64_t volatile* object, uint64_t* expected, uint64_t desired)
	{
	uint64_t prev =
	(uint64_t) InterlockedCompareExchange64 (object, desired, *expected);
	if (prev == *expected) return 1;
	*expected = prev;
	return 0;
	}

While the latter isn't a problem per-se, the usage of &eptr as uint64_t* lets atomic_compare_exchange_strong interpret whatever is at the address of eptr as an 8 byte long number.
However, the actual type uintptr_t is only 4 bytes long.

So, whatever is in the next four bytes will garble the value originally stored in eptr.

As a result in the comparison of prev (which holds "0" if the exchange worked as expected) does not compare to the expected and declare value of uintptr_t eptr = 0 from line 568 but any number that is in the respective full 8 bytes of memory will fail and set the full 8 bytes to 0.

openexr/src/lib/OpenEXRCore/chunk.c

Lines 44 to 46 in 8bc3fae

	if (prev == *expected) return 1;
	*expected = prev;
	return 0;

This not only misleads the calling code to assume that it failed to thread-safe exchange the pointers but also overwrites the ctable pointer with the (now 8 bytes of zeros) addresss and eventually (falsely) report "out of memory".

openexr/src/lib/OpenEXRCore/chunk.c

Lines 649 to 652 in 8bc3fae

	if (nptr != UINTPTR_MAX) ctxt->free_fn (ctable);
	ctable = (uint64_t*) eptr;
	if (ctable == NULL)
	return ctxt->standard_error (ctxt, EXR_ERR_OUT_OF_MEMORY);

My proposed minimal fix simply changes the type of eptr (and nptr) to always be an unsigned 64 bit value, which should work for both, 32 and 64 bit platforms.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: cary-ilm / name: Cary Phillips (f21b064)
• ✅ login: ChristopherSchwartzXRITE / name: Christopher Schwartz (a04cb6b, 6e8e640, 0abfce1)

[meshula]

Thanks for finding this!

Would you mind doing the CLA business noted in the message above?

[meshula]

This looks good to me, but I'll defer to @kdt3rd for a second approval.

[ChristopherSchwartzXRITE]

Would you mind doing the CLA business noted in the message above?

I reached out to my company's legal department but won't be able to sign off the CLA before monday.
I hope that's soon enough.

[meshula]

That's perfectly fine :) Thanks for looking into it.

[kdt3rd]

hrm, it should have been uintptr_t in the msvc block at lines 35-48, this is more likely my poor win32 programming substituting in for not having stdatomic.h properly implemented in msvc at the time I did it - there should be a 32 vs 64 bit switch depending on what size uintptr_t is - it is always pointing to a uint64 as you note (it's an offset into the file), but might be a 32-bit pointer (4-byte pointer) pointing to that array of 8-byte values, so what is there seems not quite right to me, although will work, but not as efficiently as it could on a 32-bit machine, the implementations of the atomic function for msvc should instead check #if sizeof(void *) == 4 or so and it should deal with uintptr_t...

[kdt3rd]

concretely, those two functions at line 35 should likely be #ifdef _WIN64 then use Interlocked64 or use Interlocked32 and take uintptr_t values... thanks for finding that

[ChristopherSchwartzXRITE]

concretely, those two functions at line 35 should likely be #ifdef _WIN64 then use Interlocked64 or use Interlocked32 and take uintptr_t values... thanks for finding that

I'll take a look at implementing it more efficent using the proposed #ifdef

[ChristopherSchwartzXRITE]

I did take another look and tried the following:

static inline int
atomic_compare_exchange_strong (
    atomic_uintptr_t volatile* object, uintptr_t* expected, uintptr_t desired)
{
    uintptr_t prev =
#    ifdef _WIN64
        (uintptr_t) InterlockedCompareExchange64 (object, desired, *expected);
#    else
        (uintptr_t) InterlockedCompareExchange ((uintptr_t volatile*) object, desired, *expected);
#    endif
    if (prev == *expected) return 1;
    *expected = prev;
    return 0;
}

This seemlingly worked on 32 and 64 bits and allowed me to revert eptr and nptr to be uintptr_t.

However, please note that this code now has a conversion of pointer to a 64-bit value atomic_uintptr_t volatile* object to a pointer to a 32-bit value via (uintptr_t volatile*) object.

The reason is the define for atomic_uintptr_t here:

openexr/src/lib/OpenEXRCore/internal_structs.h

Lines 57 to 59 in 8bc3fae

	/* yeah, yeah, might be a 32-bit pointer, but if we make it the same, we
	* can write less since we know support is coming (eventually) */
	typedef uint64_t atomic_uintptr_t;

This would introduce a similar error as the one I wanted to fix. However, instead of falsely reading additional bytes, here the four additional bytes wouldn't be correclty overwritten in the scope of atomic_compare_exchange_strong and could thus result in a different value when interpreted as a 64-bit value on the outside scope again.

I also briefly tried changing the atomic_uintptr_t typedef to be a 32 bit value on 32 bit systems, but this did give me incompatible type warnings all over the place.

TL;DR

I would rather keep the proposed solution to use 64 bit values for eptr and nptr instead of revising types all over the OpenEXRCore library and potentially introducing new bugs.

[ChristopherSchwartzXRITE]

Would you mind doing the CLA business noted in the message above?

It took a few more days, but now it's signed.

[kdt3rd]

Oh, sorry, yes - for that definition of atomic_uintptr_t i bodged in to internal structs, switching to uint64 would likely be a good solution. However, the uintptr_t values in chunk.c you've changed to uint64_t are then the wrong thing for a 32 bit system where the atomic_uintptr_t type is appropriately defined (and is the natural representation so you don't want to do a double cas if you don't need to). Or if you use clang / gcc under windows. I continue to think I shouldn't have been lazy, and it should be properly represented in internal_structs an ifdef on sizeof(void*) and then with the ifdefs in the compare and exchange as well... Thanks again for tracking this down and getting the cla stuff done!

[ChristopherSchwartzXRITE]

Sounds like the PR is good to merge then?

[ChristopherSchwartzXRITE]

Is there anything I can do to help get this PR merged?

[cary-ilm]

Sorry this has dragged on so long. @kdt3rd is the authority, but I did notice that the assignment to nptr on line 642 still casts to uintptr_t, which now seems incorrect.

And the assignment to ctable on line 650 is unnecessary, since both sides are now the same type.

(Why won't GitHub allow you to leave a comment on a line with no edit??)

[ChristopherSchwartzXRITE]

I did notice that the assignment to nptr on line 642 still casts to uintptr_t, which now seems incorrect.

Thanks for having another look and pointing these two casts out! I didn't see them myself initially.

I think the cast technically doesn't do any harm, as it won't do anything on a 64 bit system anyways.
And on a 32 bit system it should slice the value of ctable (which is a 32 bit address on this system) into a 32 bit type before assigning it to a 64 bit wide variable.
However, you are right that this looks odd and I'll remove the cast change the cast to cast to the target type uint64_t instead.

And the assignment to ctable on line 650 is unnecessary, since both sides are now the same type.

I'll also remove this (now) unnecessary cast.

The cast is not unnecessary after all.
ctable is an uint64_t* pointer while eptr is an uint64 value (now to be interpreted as the address pointed to by the ctable).
I'll leave it as is.

[cary-ilm]

Apologies for letting this go stale, it's fine as is. Note that now nptr and ctable are both of the same type )uint64_t), but the cast is harmless.

[cary-ilm]

It turns out this doesn't even compile on MINGW32, see #2009, since there's a mismatch in the types of the arguments to atomic_compare_exchange_strong. The first argument is of type atomic_uintptr_t from part->chunk_table, while eptr and nptr are now uint64_t.

@ChristopherSchwartzXRITE and @kdt3rd, could one of you take a look at a fresh solution? Thanks!

[ChristopherSchwartzXRITE]

It turns out this doesn't even compile on MINGW32

I agree that this is annoying and the build needs to be fixed.

I suppose the reason why the failed build went unnoticed on MSVC but is now causing an issue with MINGW32 is the special define of atomic_uintptr_t to always be an uint64_t for MSVC compilers, while other compilers (such as gcc on MINGW) will use the proper type for the platform from <stdatomic.h>:

openexr/src/lib/OpenEXRCore/internal_structs.h

Lines 41 to 62 in 75b70c6

	/* msvc, from version 19.31, evaluate __has_include(<stdatomic.h>) to true but
	* doesn't actually support it yet. Ignoring msvc for now, once we know minimal
	* version supporting it, we can compare against _MSC_VER. */
	# if !defined(_MSC_VER)
	# if defined __has_include
	# if __has_include(<stdatomic.h>)
	# define EXR_HAS_STD_ATOMICS 1
	# endif
	# endif
	# endif
	# ifdef EXR_HAS_STD_ATOMICS
	# include <stdatomic.h>
	# elif defined(_MSC_VER)
	/* msvc w/ c11 support is only very new, until we know what the preprocessor checks are, provide defaults */
	# include <windows.h>
	/* yeah, yeah, might be a 32-bit pointer, but if we make it the same, we
	* can write less since we know support is coming (eventually) */
	typedef uint64_t atomic_uintptr_t;
	# else
	# error OS unimplemented support for atomics
	# endif

Similarly, atomic_compare_exchange_strong is specifically defined for MSVC and handled differently on other platforms:

openexr/src/lib/OpenEXRCore/chunk.c

Lines 29 to 51 in 75b70c6

	#ifdef EXR_HAS_STD_ATOMICS
	# include <stdatomic.h>
	#elif defined(_MSC_VER)
	/* msvc w/ c11 support is only very new, until we know what the preprocessor checks are, provide defaults */
	# include <windows.h>
	# define atomic_load(object) InterlockedOr64 ((int64_t volatile*) object, 0)
	static inline int
	atomic_compare_exchange_strong (
	uint64_t volatile* object, uint64_t* expected, uint64_t desired)
	{
	uint64_t prev =
	(uint64_t) InterlockedCompareExchange64 (object, desired, *expected);
	if (prev == *expected) return 1;
	*expected = prev;
	return 0;
	}
	#else
	# error OS unimplemented support for atomics
	#endif

I think that means that my original problem (see this PR's description) is also just a problem for MSVC due to the special handling.

So I suppose this could be quick-fixed by making the type change introduced in this PR depend on a similar #ifdef EXR_HAS_STD_ATOMICS and #elif defined(_MSC_VER) preprocessor define distinction:

---uint64_t eptr = 0, nptr = 0;
+++#ifdef EXR_HAS_STD_ATOMICS
+++    uintptr_t eptr = 0, nptr = 0;
+++#elif defined(_MSC_VER)
+++    uint64_t eptr = 0, nptr = 0;
+++#else
+++#    error OS unimplemented support for atomics
+++#endif

@kdt3rd and @cary-ilm: Do you agree with this assessment?

[ChristopherSchwartzXRITE]

I opened PR #2009 with the changes proposed above.
Let's have any further discussion in that PR.