[libcxx] Poison memory in variant destroy

[ccotter]

In the version of __destroy that does not destruct any part of __data, MSAN is unable to see that __data is logically uninitialized. This allows false negatives such as the following to run without any MSAN diagnostic.

std::variant<double, int> v;
v.emplace<double>();
double& d = std::get<double>(v);
v.emplace<int>();
if (d) ...

With these changes, MSAN reports uninitialized memory:

==32202==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x5557b64820aa in main /home/ccotter/variant_msan.cpp:19:9
    #1 0x7fbfacd88554 in __libc_start_main /usr/src/debug/glibc-2.17-c758a686/csu/../csu/libc-start.c:266
    #2 0x5557b63ed40d in _start (/home/ccotter/a.out+0x3140d)

I wasn't sure how to add a test for this just yet, but wanted to get the changes up first to review whether this change would make sense in libc++.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Chris Cotter (ccotter)

Changes

In the version of __destroy that does not destruct any part of __data, MSAN is unable to see that __data is logically uninitialized. This allows false negatives such as the following to run without any MSAN diagnostic.

std::variant<double, int> v;
v.emplace<double>();
double& d = std::get<double>(v);
v.emplace<int>();
if (d) ...

With these changes, MSAN reports uninitialized memory:

==32202==WARNING: MemorySanitizer: use-of-uninitialized-value
    #<!-- -->0 0x5557b64820aa in main /home/ccotter/variant_msan.cpp:19:9
    #<!-- -->1 0x7fbfacd88554 in __libc_start_main /usr/src/debug/glibc-2.17-c758a686/csu/../csu/libc-start.c:266
    #<!-- -->2 0x5557b63ed40d in _start (/home/ccotter/a.out+0x3140d)

I wasn't sure how to add a test for this just yet, but wanted to get the changes up first to review whether this change would make sense in libc++.

---

Full diff: https://github.com/llvm/llvm-project/pull/101048.diff

1 Files Affected:

• (modified) libcxx/include/variant (+7)

diff --git a/libcxx/include/variant b/libcxx/include/variant
index 631ffceab5f68..41c946fc96727 100644
--- a/libcxx/include/variant
+++ b/libcxx/include/variant
@@ -261,6 +261,10 @@ namespace std {
 #include <new>
 #include <version>
 
+#if __has_feature(memory_sanitizer)
+#include <sanitizer/msan_interface.h>
+#endif
+
 // standard-mandated includes
 
 // [variant.syn]
@@ -781,6 +785,9 @@ _LIBCPP_VARIANT_DESTRUCTOR(
     _Trait::_TriviallyAvailable,
     _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 ~__dtor() = default,
     _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void __destroy() noexcept {
+#if __has_feature(memory_sanitizer)
+      __sanitizer_dtor_callback(&this->__data, sizeof(this->__data));
+#endif
       this->__index = __variant_npos<__index_t>;
     } _LIBCPP_EAT_SEMICOLON);
 

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[ccotter]

@ldionne could you help me find a reviewer for this? I've not contributed to libcxx before. Thanks!

[philnik777]

Couldn't you just call the destructor?

[ccotter]

I don't think there is a destructor to call in this case as the type is trivial, so I didn't think that it was semantically correct to call the dtor in this case, unless you think it's appropriate even for the trivial case (although I'd be happy to try that if that's your preference). The destructor for the _Trait::_Available case below does call the underlying object's destructor, and MSAN understands that.

[philnik777]

I don't think there is a destructor to call in this case as the type is trivial, so I didn't think that it was semantically correct to call the dtor in this case, unless you think it's appropriate even for the trivial case (although I'd be happy to try that if that's your preference). The destructor for the _Trait::_Available case below does call the underlying object's destructor, and MSAN understands that.

There is no destructor to call, but it's perfectly valid to call a destructor on such types: https://godbolt.org/z/qWjzvPYjW. It's called a pseudo-destructor for these cases. I think this would be a better alternative, since it avoids any MSan-specific code and makes it clear to any tooling that the lifetime of the object has ended.

[ldionne]

I agree with @philnik777 here, I think this improvement makes a lot of sense, but we'd want to implement it without referring to explicit sanitizer APIs.

[ccotter]

In my testing this will need an enhancement on the MSAN side too to understand that t.~T() where T is a trivial type like double would emit __sanitizer_dtor_callback (or equivalent) to mark the bytes as poisoned. @vitalybuka could you chime in too on the MSAN side? Just want to make sure libcxx and MSAN folks are on the same page before I proceed.

[ldionne]

Gentle ping @vitalybuka. IMO the desired outcome for libc++ is pretty clear, it would be nice if MSAN's behavior could be aligned with it.

Said differently, our code would also be valid if we already called the destructor inside variant::__destroy() today, but MSAN would still have these false negatives. In that sense, I view this as a MSAN bug and not something we should work around in libc++ by calling the MSAN API directly.

[vitalybuka]

Gentle ping @vitalybuka. IMO the desired outcome for libc++ is pretty clear, it would be nice if MSAN's behavior could be aligned with it.

MSAN bug

I guess is intentional, but msan can be improved. As-is __sanitizer_dtor_callback is a function call, so it can be too expensive. Maybe this can be done reasonably efficiently if we inline the call.

BTW. libc++ uses __sanitizer_annotate_contiguous_container, and this can probably have a custom Asan support.

[ldionne]

I think we're on the same page, then. @ccotter Could you turn this patch into one that makes a call to the destructor instead? The remaining part of the underlying issue should be fixed from the MSAN side instead.

[ccotter]

Yes, that makes sense. thanks!

[philnik777]

@ccotter do you plan to continue working on this? If not, let's close this PR for now and file an issue about this instead.