Correct atexit(3) support in MSan/NetBSD

[krytarowski]

Summary

The NetBSD specific implementation of cxa_atexit() does not
preserve the 2nd argument if dso is equal to NULL.

Changes:

• Split paths of handling intercepted __cxa_atexit() and atexit(3). This affects all supported Operating Systems.
• Add a local stack-like structure to hold the __cxa_atexit() context. atexit(3) is documented in the C standard as calling callback from the earliest to the oldest entry. This path also fixes potential ABI problem of passing an argument to a function from the atexit(3) callback mechanism.
• Allow usage of global vars with ctors in interceptors. This allows to use Vector without automatic cleaning up the structures.

This code has been modeled after TSan implementation for the same functions.

Sponsored by <The NetBSD Foundation>

[dvyukov]

lib/msan/msan_vector.h

25 ↗

(On Diff #125107)

We should move this code from tsan to sanitizer_common. sanitizer_common is meant exactly to share between sanitizers and avoid duplication of this kind of code.

[krytarowski]

lib/msan/msan_vector.h

25 ↗

(On Diff #125107)

https://reviews.llvm.org/D40726

[krytarowski]

Rebase to trunk.
Reuse __sanitizer::Vector.

[vitalybuka]

lib/msan/msan_interceptors.cc

1136

comment states obvious

1181

comment states obvious

1184

do you need to call REAL(__cxa_atexit) under the lock?

[vitalybuka]

lib/msan/msan_interceptors.cc

1139

this and PushBack comment below also do no add any value

[krytarowski]

lib/msan/msan_interceptors.cc

1184

Yes, as I'm pushing an item to the LIFO container.

[vitalybuka]

lgtm if redundant comments removed
does anyone else have anything to add?

lib/msan/msan_interceptors.cc

1184

I see
there is possibility of MSanAtExitWrapper immediately after REAL(__cxa_atexit) call
so we don't want want MSanAtExitWrapper pass that mutex before we push

[krytarowski]

lib/msan/msan_interceptors.cc

1184

The same algorithm is used in TSan. Blocking mutex is needed for hypothetical scenarios of executing atexit(3) callbacks and registering them concurrently.

[krytarowski]

Remove comments stating obvious.

[krytarowski]

Reverted, as it broke Linux.

[krytarowski]

I'm not reproducing the issues on NetBSD, so for now I will move this to background and keep patches locally.

[vitalybuka]

In D40714#949815, @krytarowski wrote:

I'm not reproducing the issues on NetBSD, so for now I will move this to background and keep patches locally.

Could you please attach move info about Linux issues, just in case some one volunteer to resolve it.

[krytarowski]

http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap-msan/builds/1187

[vitalybuka]

In D40714#950163, @krytarowski wrote:

http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap-msan/builds/1187

https://reviews.llvm.org/P8050 as lifetime of build bot logs is limited

[devnexen]

I was able to reproduce the issue on Linux but I m unsure about the proper solution, the wrappers algorithms are identical between TSan and MSan but atexit has different treatment in Msan since it s seemingly a real call. Locally I ve "disabled" atexit interceptor for Linux.

[krytarowski]

• disable atexit(3) interceptor

[devnexen]

lib/msan/msan_interceptors.cc

1194

Should it be MemorySanitizer ?

[krytarowski]

• add interceptor_scope in atexit interceptor
• correct ThreadSanitizer -> MemorySanitizer

[krytarowski]

Please test. Thank you in advance.

[krytarowski]

• add InterceptorScope interceptor_scope; in __cxa_atexit

[devnexen]

In D40714#1004346, @krytarowski wrote:

Please test. Thank you in advance.

I had updated my compiler-rt tree before this update and it finally worked but with this diff it still failed (I see the new message "unreachable called" for each test). Would be good if a genuine Linux developer could confirm.

[krytarowski]

• set REAL(atexit) in lib/msan/msan.cc

[krytarowski]

In D40714#1004362, @devnexen wrote:

In D40714#1004346, @krytarowski wrote:

Please test. Thank you in advance.

I had updated my compiler-rt tree before this update and it finally worked but with this diff it still failed (I see the new message "unreachable called" for each test). Would be good if a genuine Linux developer could confirm.

Updated diff.

[devnexen]

Problem is since it s in msan.cc it can t compile.

[krytarowski]

• don't set REAL(atexit) to notreachable

[krytarowski]

Intercept atexit.

[krytarowski]

I set InterceptorScope interceptor_scope;, maybe it's enough.

[devnexen]

In D40714#1004415, @krytarowski wrote:

I set InterceptorScope interceptor_scope;, maybe it's enough.

I tested again with and without the diff. Without seems to work.

[krytarowski]

This patch without InterceptorScope interceptor_scope; works, and this patch with InterceptorScope interceptor_scope; doesn't work?

So the original patch should be fine too.

[devnexen]

In D40714#1004417, @krytarowski wrote:

This patch without InterceptorScope interceptor_scope; works, and this patch with InterceptorScope interceptor_scope; doesn't work?

So the original patch should be fine too.

In fact when I mentioned without the diff I meant the original code.

[krytarowski]

I see, with guessing I won't go far. I will reschedule it for later once I will get an access to a Linux host capable to build LLVM.

[krytarowski]

Thanks for testing!

[krytarowski]

If we remove INTERCEPT_FUNCTION(atexit); does it work?

[devnexen]

This comment was removed by devnexen.

[devnexen]

Same failed results in my part.

[krytarowski]

• fix Linux

[krytarowski]

Thanks @mgorny for debugging on Linux.

lib/msan/msan_interceptors.cc

 #include "sanitizer_common/sanitizer_allocator_internal.h"
 #include "sanitizer_common/sanitizer_atomic.h"
 #include "sanitizer_common/sanitizer_common.h"
 #include "sanitizer_common/sanitizer_errno.h"
 #include "sanitizer_common/sanitizer_stackdepot.h"
 #include "sanitizer_common/sanitizer_libc.h"
 #include "sanitizer_common/sanitizer_linux.h"
 #include "sanitizer_common/sanitizer_tls_get_addr.h"
+#include "sanitizer_common/sanitizer_vector.h"
 
 #include <stdarg.h>
 // ACHTUNG! No other system header includes in this file.
 // Ideally, we should get rid of stdarg.h as well.
 
 using namespace __msan;
 
 using __sanitizer::memory_order;
INTERCEPTOR(void, tzset, int fake) {
   return;
 }
 
 struct MSanAtExitRecord {
   void (*func)(void *arg);
   void *arg;
 };
 
-void MSanAtExitWrapper(void *arg) {
+struct InterceptorContext {
+  BlockingMutex atexit_mu;
+  Vector<struct MSanAtExitRecord *> AtExitStack;
+
+  InterceptorContext()
+      : AtExitStack() {
+  }
+};
+
+static ALIGNED(64) char interceptor_placeholder[sizeof(InterceptorContext)];
+InterceptorContext *interceptor_ctx() {
+  return reinterpret_cast<InterceptorContext*>(&interceptor_placeholder[0]);
+}
+
+void MSanAtExitWrapper() {
+  MSanAtExitRecord *r;
+  {
+    BlockingMutexLock l(&interceptor_ctx()->atexit_mu);
+
+    uptr element = interceptor_ctx()->AtExitStack.Size() - 1;
+    r = interceptor_ctx()->AtExitStack[element];
+    interceptor_ctx()->AtExitStack.PopBack();
+  }
+
+  UnpoisonParam(1);
+  ((void(*)())r->func)();
+  InternalFree(r);
+}
+
+void MSanCxaAtExitWrapper(void *arg) {
   UnpoisonParam(1);
   MSanAtExitRecord *r = (MSanAtExitRecord *)arg;
   r->func(r->arg);
   InternalFree(r);
 }
 
+static int setup_at_exit_wrapper(void(*f)(), void *arg, void *dso);
+
+// Unpoison argument shadow for C++ module destructors.
+INTERCEPTOR(int, atexit, void (*func)()) {
+  if (msan_init_is_running) return REAL(atexit)(func);
+  return setup_at_exit_wrapper((void(*)())func, 0, 0);
+}
+
 // Unpoison argument shadow for C++ module destructors.
 INTERCEPTOR(int, __cxa_atexit, void (*func)(void *), void *arg,
             void *dso_handle) {
   if (msan_init_is_running) return REAL(__cxa_atexit)(func, arg, dso_handle);
+  return setup_at_exit_wrapper((void(*)())func, arg, dso_handle);
+}
+
+static int setup_at_exit_wrapper(void(*f)(), void *arg, void *dso) {
   ENSURE_MSAN_INITED();
   MSanAtExitRecord *r =
       (MSanAtExitRecord *)InternalAlloc(sizeof(MSanAtExitRecord));
-  r->func = func;
+  r->func = (void(*)(void *a))f;
   r->arg = arg;
-  return REAL(__cxa_atexit)(MSanAtExitWrapper, r, dso_handle);
+  int res;
+  if (!dso) {
+    // NetBSD does not preserve the 2nd argument if dso is equal to 0
+    // Store ctx in a local stack-like structure
+
+    BlockingMutexLock l(&interceptor_ctx()->atexit_mu);
+
+    res = REAL(__cxa_atexit)((void (*)(void *a))MSanAtExitWrapper, 0, 0);
+    if (!res) {
+      interceptor_ctx()->AtExitStack.PushBack(r);
+    }
+  } else {
+    res = REAL(__cxa_atexit)(MSanCxaAtExitWrapper, r, dso);
+  }
+  return res;
 }
 
 DECLARE_REAL(int, shmctl, int shmid, int cmd, void *buf)
 
 INTERCEPTOR(void *, shmat, int shmid, const void *shmaddr, int shmflg) {
   ENSURE_MSAN_INITED();
   void *p = REAL(shmat)(shmid, shmaddr, shmflg);
   if (p != (void *)-1) {
     __sanitizer_shmid_ds ds;
     int res = REAL(shmctl)(shmid, shmctl_ipc_stat, &ds);
     if (!res) {
       __msan_unpoison(p, ds.shm_segsz);
void __msan_unpoison_string(const char* s) {
   __msan_unpoison(s, REAL(strlen)(s) + 1);
 }
 
 namespace __msan {
 
 void InitializeInterceptors() {
   static int inited = 0;
   CHECK_EQ(inited, 0);
+
+  new(interceptor_ctx()) InterceptorContext();
+
   InitializeCommonInterceptors();
   InitializeSignalInterceptors();
 
   INTERCEPT_FUNCTION(mmap);
   MSAN_MAYBE_INTERCEPT_MMAP64;
   INTERCEPT_FUNCTION(posix_memalign);
   MSAN_MAYBE_INTERCEPT_MEMALIGN;
   MSAN_MAYBE_INTERCEPT___LIBC_MEMALIGN;
#endif
   INTERCEPT_FUNCTION(pthread_key_create);
 
 #if SANITIZER_NETBSD
   INTERCEPT_FUNCTION(__libc_thr_keycreate);
 #endif
 
   INTERCEPT_FUNCTION(pthread_join);
   INTERCEPT_FUNCTION(tzset);
+  INTERCEPT_FUNCTION(atexit);
   INTERCEPT_FUNCTION(__cxa_atexit);
   INTERCEPT_FUNCTION(shmat);
   INTERCEPT_FUNCTION(fork);
   INTERCEPT_FUNCTION(openpty);
   INTERCEPT_FUNCTION(forkpty);
 
   inited = 1;
 }
 } // namespace __msan