Fix ASAN problems under Clang 15.

kentonv · kentonv · commit c4eef80a13e8 · 2022-06-29T11:38:06.000-05:00
It appears ASAN now by default tries to detect stack-use-after-return. This breaks our assumptions in requireOnStack() and totally breaks fibers.

For requireOnStack() we can just skip the check in this case.

For fibers, we need to implement the ASAN hints to tell it when we're switching fibers.
diff --git a/c++/src/kj/async-test.c++ b/c++/src/kj/async-test.c++
@@ -1162,7 +1162,12 @@ KJ_TEST("fiber pool") {
         if (i1_local == nullptr) {
           i1_local = &i;
         } else {
+#if !KJ_HAS_COMPILER_FEATURE(address_sanitizer)
+          // Verify that the stack variable is in the exact same spot as before.
+          // May not work under ASAN as the instrumentation to detect stack-use-after-return can
+          // change the address.
           KJ_ASSERT(i1_local == &i);
+#endif
         }
         return i;
       });
@@ -1173,7 +1178,9 @@ KJ_TEST("fiber pool") {
           if (i2_local == nullptr) {
             i2_local = &i;
           } else {
+#if !KJ_HAS_COMPILER_FEATURE(address_sanitizer)
             KJ_ASSERT(i2_local == &i);
+#endif
           }
           return i;
         });
@@ -1210,9 +1217,24 @@ KJ_TEST("fiber pool") {
 bool onOurStack(char* p) {
   // If p points less than 64k away from a random stack variable, then it must be on the same
   // stack, since we never allocate stacks smaller than 64k.
+#if KJ_HAS_COMPILER_FEATURE(address_sanitizer)
+  // The stack-use-after-return detection mechanism breaks our ability to check this, so don't.
+  return true;
+#else
   char c;
   ptrdiff_t diff = p - &c;
   return diff < 65536 && diff > -65536;
+#endif
+}
+
+bool notOnOurStack(char* p) {
+  // Opposite of onOurStack(), except returns true if the check can't be performed.
+#if KJ_HAS_COMPILER_FEATURE(address_sanitizer)
+  // The stack-use-after-return detection mechanism breaks our ability to check this, so don't.
+  return true;
+#else
+  return !onOurStack(p);
+#endif
 }
 
 KJ_TEST("fiber pool runSynchronously()") {
@@ -1240,11 +1262,14 @@ KJ_TEST("fiber pool runSynchronously()") {
   });
   KJ_ASSERT(ptr2 != nullptr);
 
+#if !KJ_HAS_COMPILER_FEATURE(address_sanitizer)
   // Should have used the same stack both times, so local var would be in the same place.
+  // Under ASAN, the stack-use-after-return detection correctly fires on this, so we skip the check.
   KJ_EXPECT(ptr1 == ptr2);
+#endif
 
   // Should have been on a different stack from the main stack.
-  KJ_EXPECT(!onOurStack(ptr1));
+  KJ_EXPECT(notOnOurStack(ptr1));
 
   KJ_EXPECT_THROW_MESSAGE("test exception",
       pool.runSynchronously([&]() { KJ_FAIL_ASSERT("test exception"); }));
@@ -1298,7 +1323,7 @@ KJ_TEST("fiber pool limit") {
   // is the one from the thread.
   pool.runSynchronously([&]() {
     KJ_EXPECT(onOurStack(ptr2));
-    KJ_EXPECT(!onOurStack(ptr1));
+    KJ_EXPECT(notOnOurStack(ptr1));
 
     KJ_EXPECT(pool.getFreelistSize() == 0);
   });
@@ -1369,15 +1394,15 @@ KJ_TEST("run event loop on freelisted stacks") {
 
     // The event callbacks should have run on a different stack, but the wait should have been on
     // the main stack.
-    KJ_EXPECT(!onOurStack(ptr1));
-    KJ_EXPECT(!onOurStack(ptr2));
+    KJ_EXPECT(notOnOurStack(ptr1));
+    KJ_EXPECT(notOnOurStack(ptr2));
     KJ_EXPECT(onOurStack(port.waitStack));
 
     pool.runSynchronously([&]() {
       // This should run on the same stack where the event callbacks ran.
       KJ_EXPECT(onOurStack(ptr1));
       KJ_EXPECT(onOurStack(ptr2));
-      KJ_EXPECT(!onOurStack(port.waitStack));
+      KJ_EXPECT(notOnOurStack(port.waitStack));
     });
   }
 
@@ -1410,8 +1435,8 @@ KJ_TEST("run event loop on freelisted stacks") {
 
     // The event callback should have run on a different stack, and poll() should have run on
     // a separate stack too.
-    KJ_EXPECT(!onOurStack(ptr1));
-    KJ_EXPECT(!onOurStack(port.pollStack));
+    KJ_EXPECT(notOnOurStack(ptr1));
+    KJ_EXPECT(notOnOurStack(port.pollStack));
 
     pool.runSynchronously([&]() {
       // This should run on the same stack where the event callbacks ran.
diff --git a/c++/src/kj/async.c++ b/c++/src/kj/async.c++
@@ -78,6 +78,19 @@
 
 #include <stdlib.h>
 
+#if KJ_HAS_COMPILER_FEATURE(address_sanitizer)
+// Clang's address sanitizer requires special hints when switching fibers, especially in order for
+// stack-use-after-return handling to work right.
+//
+// TODO(someday): Does GCC's sanitizer, flagged by __SANITIZE_ADDRESS__, have these hints too? I
+//   don't know and am not in a position to test, so I'm assuming not for now.
+#include <sanitizer/common_interface_defs.h>
+#else
+// Nop the hints so that we don't have to put #ifdefs around every use.
+#define __sanitizer_start_switch_fiber(...)
+#define __sanitizer_finish_switch_fiber(...)
+#endif
+
 #if _MSC_VER && !__clang__
 // MSVC's atomic intrinsics are weird and different, whereas the C++ standard atomics match the GCC
 // builtins -- except for requiring the obnoxious std::atomic<T> wrapper. So, on MSVC let's just
@@ -1330,6 +1343,23 @@ struct FiberStack::Impl {
   jmp_buf fiberJmpBuf;
   jmp_buf originalJmpBuf;
 
+#if KJ_HAS_COMPILER_FEATURE(address_sanitizer)
+  // Stuff that we need to pass to __sanitizer_start_switch_fiber() /
+  // __sanitizer_finish_switch_fiber() when using ASAN.
+
+  void* originalFakeStack = nullptr;
+  void* fiberFakeStack = nullptr;
+  // Pointer to ASAN "fake stack" associated with the fiber and its calling stack. Filled in by
+  // __sanitizer_start_switch_fiber() before switching away, consumed by
+  // __sanitizer_finish_switch_fiber() upon switching back.
+
+  void const* originalBottom;
+  size_t originalSize;
+  // Size and location of the original stack before switching fibers. These are filled in by
+  // __sanitizer_finish_switch_fiber() after the switch, and must be passed to
+  // __sanitizer_start_switch_fiber() when switching back later.
+#endif
+
   static Impl* alloc(size_t stackSize, ucontext_t* context) {
 #ifndef MAP_ANONYMOUS
 #define MAP_ANONYMOUS MAP_ANON
@@ -1423,6 +1453,9 @@ struct FiberStack::StartRoutine {
 
     auto& stack = *reinterpret_cast<FiberStack*>(ptr);
 
+    __sanitizer_finish_switch_fiber(nullptr,
+        &stack.impl->originalBottom, &stack.impl->originalSize);
+
     // We first switch to the fiber inside of the FiberStack constructor. This is just for
     // initialization purposes, and we're expected to switch back immediately.
     stack.switchToMain();
@@ -1478,9 +1511,11 @@ FiberStack::FiberStack(size_t stackSizeParam)
 
   makecontext(&context, reinterpret_cast<void(*)()>(&StartRoutine::run), 2, arg1, arg2);
 
+  __sanitizer_start_switch_fiber(&impl->originalFakeStack, impl, stackSize - sizeof(Impl));
   if (_setjmp(impl->originalJmpBuf) == 0) {
     setcontext(&context);
   }
+  __sanitizer_finish_switch_fiber(impl->originalFakeStack, nullptr, nullptr);
 #endif
 #else
 #if KJ_NO_EXCEPTIONS
@@ -1577,9 +1612,11 @@ void FiberStack::switchToFiber() {
 #if _WIN32 || __CYGWIN__
   SwitchToFiber(osFiber);
 #else
+  __sanitizer_start_switch_fiber(&impl->originalFakeStack, impl, stackSize - sizeof(Impl));
   if (_setjmp(impl->originalJmpBuf) == 0) {
     _longjmp(impl->fiberJmpBuf, 1);
   }
+  __sanitizer_finish_switch_fiber(impl->originalFakeStack, nullptr, nullptr);
 #endif
 #endif
 }
@@ -1590,9 +1627,21 @@ void FiberStack::switchToMain() {
 #if _WIN32 || __CYGWIN__
   SwitchToFiber(getMainWin32Fiber());
 #else
+  // TODO(someady): In theory, the last time we switch away from the fiber, we should pass `nullptr`
+  //   for the first argument here, so that ASAN destroys the fake stack. However, as currently
+  //   designed, we don't actually know if we're switching away for the last time. It's understood
+  //   that when we call switchToMain() in FiberStack::run(), then the main stack is allowed to
+  //   destroy the fiber, or reuse it. I don't want to develop a mechanism to switch back to the
+  //   fiber on final destruction just to get the hints right, so instead we leak the fake stack.
+  //   This doesn't seem to cause any problems -- it's not even detected by ASAN as a memory leak.
+  //   But if we wanted to run ASAN builds in production or something, it might be an issue.
+  __sanitizer_start_switch_fiber(&impl->fiberFakeStack,
+                                 impl->originalBottom, impl->originalSize);
   if (_setjmp(impl->fiberJmpBuf) == 0) {
     _longjmp(impl->originalJmpBuf, 1);
   }
+  __sanitizer_finish_switch_fiber(impl->fiberFakeStack,
+                                  &impl->originalBottom, &impl->originalSize);
 #endif
 #endif
 }
diff --git a/c++/src/kj/exception-test.c++ b/c++/src/kj/exception-test.c++
@@ -132,10 +132,16 @@ TEST(Exception, UnwindDetector) {
 }
 #endif
 
+#if defined(FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION) || \
+    KJ_HAS_COMPILER_FEATURE(address_sanitizer) || \
+    defined(__SANITIZE_ADDRESS__)
+// The implementation skips this check in these cases.
+#else
 #if !__MINGW32__  // Inexplicably crashes when exception is thrown from constructor.
 TEST(Exception, ExceptionCallbackMustBeOnStack) {
   KJ_EXPECT_THROW_MESSAGE("must be allocated on the stack", new ExceptionCallback);
 }
+#endif
 #endif  // !__MINGW32__
 
 #if !KJ_NO_EXCEPTIONS
diff --git a/c++/src/kj/exception.c++ b/c++/src/kj/exception.c++
@@ -971,7 +971,11 @@ KJ_THREADLOCAL_PTR(ExceptionCallback) threadLocalCallback = nullptr;
 
 ExceptionCallback::ExceptionCallback(): next(getExceptionCallback()) {
   char stackVar;
-#ifndef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
+#if defined(FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION) || \
+    KJ_HAS_COMPILER_FEATURE(address_sanitizer) || \
+    defined(__SANITIZE_ADDRESS__)
+  // When using libfuzzer or ASAN, this sanity check may spurriously fail, so skip it.
+#else
   ptrdiff_t offset = reinterpret_cast<char*>(this) - &stackVar;
   KJ_ASSERT(offset < 65536 && offset > -65536,
             "ExceptionCallback must be allocated on the stack.");
