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

Destructor Tombstones #5318

Merged
merged 18 commits into from
Mar 3, 2025
Merged

Conversation

StephanTLavavej
Copy link
Member

🪦 Overview

Resolves #5315. See that issue for the design and rationale.

This feature is currently disabled by default. When disabled, it is completely preprocessed away and has no effect whatsoever.

💀 Commits

  • Add control and value macros.
  • Implement tombstones for containers.
    • ~_Vector_val() for vector<T> demonstrates the most complete pattern:
      • We guard everything with #if _MSVC_STL_DESTRUCTOR_TOMBSTONES, so it completely vanishes when disabled.
      • We guard against fancy pointers.
      • We guard against constant evaluation.
      • We use a named _Tombstone because _MSVC_STL_UINTPTR_TOMBSTONE_VALUE might expand to a function call.
    • ~_Vb_val() for vector<bool> already exists.
      • _Vectype _Myvec provides the storage, which we just tombstoned.
      • Zeroing out _Mysize ensures that vector<bool>::operator[] hardening will detect that _Off < this->_Mysize is false.
      • We don't need to reinterpret_cast here, so we don't need to guard against constant evaluation.
      • The general pattern is that we use the tombstone value for pointers, and default construction values (typically zero) for other members. This results in the safest state of "I look like I contain nothing after default construction, except that my pointers are radioactive".
    • Note that different containers use different pointer types with is_pointer_v and reinterpret_cast. For example, _Deque_val uses _Mapptr while _List_val uses _Nodeptr.
    • ~_Hash() for the unordered associative containers:
      • Doesn't need to deal with _Mylist _List, which is a normal list.
      • Doesn't need to deal with _Hash_vec<_Aliter> _Vec. It's not a normal vector, but it stores a _Vector_val which we've tombstoned.
      • Setting _Mask and _Maxidx back to their default construction values doesn't provide the same benefit as zeroing out size fields, but it's consistent with the pattern and seems worth doing.
  • Implement tombstones for basic_string.
    • This is special because of the Small String Optimization. In small mode, the string would be usable, so we need large mode for the tombstone to have any effect.
    • I'm setting _Mysize to zero, to work with hardened preconditions as usual. For the capacity _Myres, I'm using the first value that basic_string would use when entering large mode, as it has a "roundup mask".
    • _Large_mode_engaged() returns _Myres > _Small_string_capacity, so _Small_string_capacity + 1 would be sufficient, but it seems safer to use the value that basic_string would ordinarily use, as this is less likely to confuse basic_string logic now and in the future. Only the tombstone pointer needs to be impossible.
  • Implement tombstones for unique_ptr (scalar and array).
  • Implement tombstones for shared_ptr and weak_ptr.
  • Implement tombstones for function.
    • The Small Functor Optimization is much easier to deal with than basic_string. _Local() returns _Getimpl() == static_cast<const void*>(&_Mystorage), so the tombstone pointer will be detected as large.
  • Implement tombstones for basic_regex.
  • Implement tombstones for valarray.
    • Comment that _Tidy_deallocate() already sets _Mysize = 0;.
  • Implement tombstones for any.
    • With a comment explaining why we're using _Small mode.
  • Implement tombstones for nontrivial optional.
    • This makes the destructor behave like reset(). Setting it to empty will work with precondition hardening to prevent access to the object.
    • We should not attempt to scribble over T's bytes.
    • Add a comment explaining why we can't tombstone trivial optionals (which are lower risk anyways, so no big loss).
  • Implement tombstones for nontrivial variant.
    • Same kind of comment for trivial variant.
  • Implement tombstones for move_only_function.
    • Everything goes through this pseudo-vtable.
  • Implement tombstones for exception_ptr.
    • __ExceptionPtrDestroy() calls ~shared_ptr<const _EXCEPTION_RECORD>(), but it's separately compiled.
  • Implement tombstones for polymorphic_allocator.
  • Update VSO_0000000_wcfb01_idempotent_container_destructors.
    • Add an #error for the future when we enable destructor tombstones by default.
    • Update the comments to clarify how we feel about this usage.
    • Add missing alignas to the buffers - we were being very bad kitties. 😼
  • Add GH_005315_destructor_tombstones.
    • I've manually confirmed that in release mode without optimizations, each of these tests "gets away" with UB. Of course, this is neither guaranteed nor something we desire by itself, which is why this isn't part of the automated testing. As the comment explains, having each test get away with UB without tombstones, only to be reliably stopped by tombstones, is the best way to confirm that the tombstones are effective. (It would be less compelling for the test to do something complicated that would have crashed even without tombstones.)
    • So, I've carefully written each test to take a default-constructed object (except for optional and variant which are special because we simply set them to empty/valueless, so they have to start in a value-holding state), then call a simple preconditionless operation, chosen to have the highest chance of succeeding without tombstones (if in unchecked release mode), but to encounter the tombstone if enabled. Again, this test is always built with tombstones enabled, so it should be perfectly reliable.
    • In all cases except for the dynamically allocated sentinel nodes (list, set, and unordered_set), not even ASan can detect the UB without tombstones (which is ideal for the test, as the comment explains). ASan with tombstones prints nice messages about using the bogus address.
    • In this test's debug and ASan configurations, debug/ASan checks may terminate the tests before the tombstones are encountered. That's fine.
    • I am specifically testing 1-byte, 2-byte, and 4-byte characters for basic_string because of the capacity calculation. We don't need to test all character types here.
    • Note that basic_string is special and allows its null terminator to be accessed by operator[] and even overwritten as long as it's with an identical null character value, so str[0] = '\0'; is actually valid on any modifiable string (just not a destroyed one!).
    • Finally, because we aren't yet enabling tombstones anywhere else in the test suite, I've included a small compile-only test to verify that we aren't damaging constexpr.
    • I haven't added automated testing for tombstones + fancy pointers (which would be a lot of effort for little benefit), but I manually verified that the entire test suite passes with tombstones temporarily enabled (with the exception of VSO_0000000_wcfb01_idempotent_container_destructors), which does provide good fancy pointer coverage.

* `~_Vector_val()` for `vector<T>` demonstrates the most complete pattern:
  + We guard everything with `#if _MSVC_STL_DESTRUCTOR_TOMBSTONES`, so it completely vanishes when disabled.
  + We guard against fancy pointers.
  + We guard against constant evaluation.
  + We use a named `_Tombstone` because `_MSVC_STL_UINTPTR_TOMBSTONE_VALUE` might expand to a function call.
* `~_Vb_val()` for `vector<bool>` already exists.
  + `_Vectype _Myvec` provides the storage, which we just tombstoned.
  + Zeroing out `_Mysize` ensures that `vector<bool>::operator[]` hardening will detect that `_Off < this->_Mysize` is false.
  + We don't need to `reinterpret_cast` here, so we don't need to guard against constant evaluation.
  + The general pattern is that we use the tombstone value for pointers, and default construction values (typically zero) for other members. This results in the safest state of "I look like I contain nothing after default construction, except that my pointers are radioactive".
* Note that different containers use different pointer types with `is_pointer_v` and `reinterpret_cast`. For example, `_Deque_val` uses `_Mapptr` while `_List_val` uses `_Nodeptr`.
* `~_Hash()` for the unordered associative containers:
  + Doesn't need to deal with `_Mylist _List`, which is a normal `list`.
  + Doesn't need to deal with `_Hash_vec<_Aliter> _Vec`. It's not a normal `vector`, but it stores a `_Vector_val` which we've tombstoned.
  + Setting `_Mask` and `_Maxidx` back to their default construction values doesn't provide the same benefit as zeroing out size fields, but it's consistent with the pattern and seems worth doing.
This is special because of the Small String Optimization. In small mode, the string would be usable,
so we need large mode for the tombstone to have any effect.

I'm setting `_Mysize` to zero, to work with hardened preconditions as usual. For the capacity `_Myres`,
I'm using the first value that `basic_string` would use when entering large mode,
as it has a "roundup mask".

`_Large_mode_engaged()` returns `_Myres > _Small_string_capacity`, so `_Small_string_capacity + 1` would be sufficient,
but it seems safer to use the value that `basic_string` would ordinarily use,
as this is less likely to confuse `basic_string` logic now and in the future.
Only the tombstone pointer needs to be impossible.
The Small Functor Optimization is much easier to deal with than `basic_string`.
`_Local()` returns `_Getimpl() == static_cast<const void*>(&_Mystorage)`,
so the tombstone pointer will be detected as large.
Comment that `_Tidy_deallocate()` already sets `_Mysize = 0;`.
With a comment explaining why we're using `_Small` mode.
This makes the destructor behave like `reset()`.

Setting it to empty will work with precondition hardening to prevent access to the object.

We should not attempt to scribble over `T`'s bytes.
Everything goes through this pseudo-vtable.
`__ExceptionPtrDestroy()` calls `~shared_ptr<const _EXCEPTION_RECORD>()`, but it's separately compiled.
Add an `#error` for the future when we enable destructor tombstones by default.

Update the comments to clarify how we feel about this usage.

Add missing `alignas` to the buffers - we were being very bad kitties.
@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Feb 27, 2025
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner February 27, 2025 16:09
@AlexGuteniev

This comment was marked as resolved.

@StephanTLavavej

This comment was marked as resolved.

@AlexGuteniev

This comment was marked as resolved.

@StephanTLavavej
Copy link
Member Author

I've pushed an additional commit to skip the test under /clr. It passes locally when run repeatedly, but there was a sporadic hang in the internal PR system. This doesn't indicate anything wrong with the feature - it just looks like /clr occasionally reacts to tombstone violations in a way that our "death test" machinery doesn't handle.

Copy link
Member

@davidmrdavid davidmrdavid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

☠️ 🦇 CasSTLvania, symphony of the tombstone ☠️ 🦇

Thanks for this PR! Left some comments, mostly nits and some questions. Very close to approval.

@StephanTLavavej StephanTLavavej added the high priority Important! label Mar 1, 2025
Copy link
Member

@davidmrdavid davidmrdavid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appreciate the follow ups and incorporating the detailed comments, it helps me a lot and I think it makes the code more accessible. LGTM

@StephanTLavavej StephanTLavavej merged commit 0d8f517 into microsoft:main Mar 3, 2025
39 checks passed
@StephanTLavavej StephanTLavavej deleted the tombstone-pizza branch March 3, 2025 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved high priority Important!
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Implement destructor tombstones
5 participants