Fix AddressSanitizer: heap-buffer-overflow from embree buffer check

[jamie-pate]

Fixes #94548

Embree code seems to do some incorrect confusing pointer math when checking the padding, This evaluates to extra 12 bytes dereferencing an additional 4 byte int.

When the address sanitizer is enabled it triggers an assertion

    /*! checks padding to 16 byte check, fails hard */
    __forceinline void checkPadding16() const
    {
      if (ptr_ofs && num)
volatile int MAYBE_UNUSED w = *((int*)getPtr(size()-1)+3); // FIXME: is failing hard avoidable?
    }

[jamie-pate]

Looks like the vector 'grows exponentially' so it would have to be exactly sized to hit near a power of 2 to hit this dereference check and possibly cause a crash

Edit:
Unfortunately all powers of 2 are not multiples of 12...
e.g.

for LocalVector<Vector3>:

42 x 12 = 504b (42 x sizeof(Vector3))
getptr(size -1) == byte 500 (last float* address is the last 4 bytes of the buffer)
resize(42) will set capacity to 512 (next larger power of 2)
500 + 16 = 516 (4 bytes past the end of the 512 byte/2^9byte capacity)

[jamie-pate]

It looks like we should use reserve() instead as well as resize() here..
reserve() with the padding, after resize() without padding.

Also, upon further inspection I suspect this buffer may cause this error:
throw_RTCError(RTC_ERROR_INVALID_OPERATION, "data must be 4 bytes aligned");
on 32 bit systems that don't align their malloc to 16 bytes!!!

Even worse: this would only fail on release builds since debug builds automatically add padding to align to sizeof(uint64_t) (it's fine, see my next comment)

[jamie-pate]

It looks like we should use reserve() instead of resize() here.. Also, upon further inspection I suspect this buffer may cause this error: throw_RTCError(RTC_ERROR_INVALID_OPERATION, "data must be 4 bytes aligned"); on 32 bit systems that don't align their malloc to 16 bytes!!!

Even worse: this would only fail on release builds since debug builds automatically add padding to align to sizeof(uint64_t)

After a more careful reading I realize the byte alignment requirement for the start of the buffer is 4 bytes (not 16).

x86_32 malloc implementations seem to align to 8 bytes so I can stop worrying :D

on 32 bit GNU/Linux for example, where realloc/malloc specification says

The address of a block returned by malloc or realloc in GNU systems is always a multiple of eight (or sixteen on 64-bit systems).

or msvcrt for 32 bit windows

In Visual C++, the fundamental alignment is the alignment that's required for a double, or 8 bytes. In code that targets 64-bit platforms, it's 16 bytes.

[clayjohn]

This seems fine and I trust your judgement. But at this point it feels a bit risky to merge for RC1, so I'm bumping to 4.4.

[jamie-pate]

The chances of this actually causing a segfault/access violation are pretty slim I don't see it happening in my project without turning on ASAN. Early 4.4/4.3.1/4.2.x etc sounds fine to me :D

[jamie-pate]

Fixed by #98770