search

real-logic / aeron

Efficient reliable UDP unicast, UDP multicast, and IPC message transport
Apache License 2.0
7.37k stars 888 forks source link

AtomicBuffer strict-aliasing violations (undefined behavior) #1627

Closed gsauthof closed 2 months ago

gsauthof commented 3 months ago

Some methods in the AtomicBuffer class such as overlayStruct(), putInt64() and getInt32() violate C++ strict-aliasing rules and thus yield undefined behavior (UB) because an underlying uint8_t array is accessed through unrelated and incompatible pointer types:

https://github.com/real-logic/aeron/blob/dd302d5a243870657f8fa1917d0d157605e4ab0f/aeron-client/src/main/cpp/concurrent/AtomicBuffer.h#L189-L220

Also, defining the field m_buffer as std::uint8_t*, cf.

https://github.com/real-logic/aeron/blob/dd302d5a243870657f8fa1917d0d157605e4ab0f/aeron-client/src/main/cpp/concurrent/AtomicBuffer.h#L475

in such a low level buffer class invites more UB because the C++ standard doesn't allow extra aliasing for uint8_t pointers. This contrasts unsigned char, char or std::byte pointers for which the C++ specifies special aliasing rules.

Thus, for such a buffer class it's usually advantageous to use - say - unsigned char pointers internally to eliminate UB.

Sure, currently, in most environments uint8_t is simply typedeffed to unsigned char. However, one can't rely on that this is everywhere the case and that no implementation ever changes that typedef. For example, there are people who use uin8_t for intended use cases and who complain to compiler developers about missed optimization opportunities in their uint8_t array code, when uint8_t is typedeffed to unsigned char, because of possible aliasing to all other types that is then allowed.

I'm thus wondering:

  1. Does aeron require users to compile everything with -fno-strict-aliasing?
  2. Is the aeron team interested in eliminiting such undefined behavior?
mikeb01 commented 2 months ago

On all of the platforms that we test on uint8_t is currently typedef'd unsigned char. As you say that can be relied upon, so I've added a static_assert to verify that these are the same times. Therefore the compilation will break on any platform on which this is not supported. While it would be safer to change the internal representation to unsigned char this would mean that the passed in parameters would also need to change or perform some sort of byte by byte copy. As this is designed so that is does not own the underlying buffer this creates some awkward ownership issues or would require cascading this change out to all use of this class, which would potentially break a lot of customer code.