Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix UB of potentially wrong compile-time assumption of transparently replaceable #81

Merged
merged 1 commit into from
Apr 6, 2024

Conversation

mingxwa
Copy link
Collaborator

@mingxwa mingxwa commented Apr 4, 2024

Added necessary calls to std::launder() to avoid causing undefined behavior. No functional changes.

There is a discussion of std::launder() on stackoverflow. libc++ is also using std::launder() in its implementation of small buffer optimization.

Closes #80

@tian-lt-personal
Copy link

tian-lt-personal commented Apr 4, 2024

Suggest adding test cases to prevent from regressions.

@mingxwa
Copy link
Collaborator Author

mingxwa commented Apr 5, 2024

Suggest adding test cases to prevent from regressions.

It does not seem to be easy to come up with a case that can break without std::launder() on GCC, clang or MSVC.

I checked the transmogrify() example on (cppreference)[https://en.cppreference.com/w/cpp/utility/launder]. It breaks on GCC and clang, but it is not how proxy works. I believe the behavior is generally undefined if a "transmogrify-able" type is used in an STL container or proxy.

Another way to attack is const fields. I tried many ways to trick the compilers, but none of the 3 compromises.

I also checked the unit tests of libstdc++. They have a case for std::launder(), but unfortunately the case passes even without std::launder() on recent GCC releases (I checked several versions with O2 and O3). Same as Microsoft implementation of STL.

Search for std::launder() in GitHub, I found a comment in abseil:

// A typed accessor for the object in `TypeErasedState` storage
template <class T>
T& ObjectInLocalStorage(TypeErasedState* const state) {
  // We launder here because the storage may be reused with the same type.
#if defined(__cpp_lib_launder) && __cpp_lib_launder >= 201606L
  return *std::launder(reinterpret_cast<T*>(&state->storage));
#elif ABSL_HAVE_BUILTIN(__builtin_launder)
  return *__builtin_launder(reinterpret_cast<T*>(&state->storage));
#else

  // When `std::launder` or equivalent are not available, we rely on undefined
  // behavior, which works as intended on Abseil's officially supported
  // platforms as of Q2 2022.
#if !defined(__clang__) && defined(__GNUC__)
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wstrict-aliasing"
#endif
  return *reinterpret_cast<T*>(&state->storage);
#if !defined(__clang__) && defined(__GNUC__)
#pragma GCC diagnostic pop
#endif

#endif
}

After research for a whole day, I do not think we should spend more time try to come up with a quality unit test case for this change.

@mingxwa mingxwa closed this Apr 5, 2024
@mingxwa mingxwa reopened this Apr 5, 2024
@mingxwa mingxwa merged commit d6846aa into microsoft:main Apr 6, 2024
7 checks passed
@mingxwa mingxwa deleted the user/mingxwa/lifetime-ub branch April 6, 2024 06:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Undefined behavior: Field ptr_ of proxy is accessed with potentially wrong compile-time assumption
3 participants