[PATCH] D20913: [asan] add primitives that allow coroutine implementations

[Filipe Cabecinhas via llvm-commits]

filcab added a comment.

What ends up happening seems to be a silent bug (and possible corruption afterwards):

- Create two fibers: Leader and Worker
- Leader allocates stuff on the stack (needing an `asan_stack_malloc` call), creating a `FakeFrame`
- Switch to Worker
- Worker does the same, then throws something, catching it too. (This sets the `gc_needed_` flag on the `StackFrame`, since throwing calls `handle_no_return`)
- Worker calls another function that sets up a `FakeFrame`
  - This triggers a GC on `FakeStack` and we collect the Leader's `FakeFrame`!!
- Switch to Leader
- We can still use the array we allocated (because we didn't poison the shadow when GCing), but the `FakeStack` might have reallocated that `FakeFrame` and we would be using invalid frames.

Here's output from a test program I have using our fibers. I added prints to ASan to help show off the bug. And added comments

  Initializing jobs...
  Starting jobs...
  stack_malloc_6(size=d20)
  Allocated FakeFrame 0x0002129b5800                   <- Leader fake frame
  Started Leader
  Created array
  stack_malloc_6(size=d20)
  Allocated FakeFrame 0x0002129b6800                   <- Worker fake frame
  Started Worker
  Created array
  stack_malloc_6(size=d20)                             <- Setting up dummy function's fake frame
  GCing
  Checking FakeFrame idx 0 (flags: 1): 0x0002129b5800
  Collecting!                                          <- Just collected Leader's fake frame!!
  Checking FakeFrame idx 1 (flags: 1): 0x0002129b6800
  Allocated FakeFrame 0x0002129b7800
  dummyFunction
  Created array
  stack_free_6(ptr=0x0002129b7800, size=d20)
  Deallocating FakeFrame 0x0002129b7800
  stack_free_6(ptr=0x0002129b6800, size=d20)
  Deallocating FakeFrame 0x0002129b6800
  stack_free_6(ptr=0x0002129b5800, size=d20)
  Deallocating FakeFrame 0x0002129b5800                <- Finally deallocating the Leader's fake frame. Proper place to "collect" it.
  Exiting.

Here's some pseudo-code for the functions (sorry about the uglyness. Compile with -O0 so ASan actually inserts the `asan_stack_{malloc,free}_*` calls). Since I don't have easy access to a machine with Linux, I can't give you a full test-case. This should be adaptable, though.
Start by creating contexts for the fibers. Then setup a fiber that runs doLeader, and one that does doWorker. Switch to Leader.
If you want to just try swapcontext, I think we should be able to replace the `switchToFiber` with `swapcontext` (assuming we created the appropriate contexts) and it should almost work?

  #include <stdio.h>
  #include <assert.h>
  #include <inttypes.h>
  
  struct { void* nextFiber; } Leader, Worker;
  
  void switchToFiber(void *, uint64_t, uint64_t*);  // Fiber, arg to "return", ptr for "returned" arg
  void returnToThread(uint64_t, uint64_t*);
  
  struct Stuff { void *S1, *S2; };
  __attribute__((noinline,noreturn)) void doLeader(uint64_t argOnInitialize, uint64_t argOnRun) {
    fprintf(stderr, "Started Leader\n");
    // Sets up a FakeStack
    Stuff aaa[200] = {nullptr, nullptr};
    fprintf(stderr, "Created array\n");
    
    switchToFiber(Leader.nextFiber, 0, nullptr);
    returnToThread((uint64_t)aaa->S1, nullptr);
  }
  
  __attribute__((noinline)) int dummyFunction(int *p) {
    fprintf(stderr, "dummyFunction\n");
    Stuff bbb[200] = {nullptr, nullptr};
    fprintf(stderr, "Created array\n");
    return p[1];
  }
  
  __attribute__((noinline,noreturn)) void doWorker(uint64_t argOnInitialize, uint64_t argOnRun) {
    fprintf(stderr, "Started Worker\n");
    Stuff ccc[200] = {nullptr, nullptr};
    fprintf(stderr, "Created array\n");
    try {
      // Calls handle_no_return (which sets FakeStack::needs_gc_)
      throw 3;
    } catch (int e) {
    }
    // Will call asan_stack_malloc and GC the fake stack
    dummyFunction((int*)ccc);
    switchToFiber(Worker.nextFiber, 0, nullptr);
  }

Here's the diff for `lib/asan/asan_fake_stack.cc`:

  diff --git a/lib/asan/asan_fake_stack.cc b/lib/asan/asan_fake_stack.cc
  index 91fdf0a..87f7674 100644
  --- a/lib/asan/asan_fake_stack.cc
  +++ b/lib/asan/asan_fake_stack.cc
  @@ -140,6 +140,7 @@ void FakeStack::HandleNoReturn() {
   // We do it based on their 'real_stack' values -- everything that is lower
   // than the current real_stack is garbage.
   NOINLINE void FakeStack::GC(uptr real_stack) {
  +  Printf("GCing\n");
     uptr collected = 0;
     for (uptr class_id = 0; class_id < kNumberOfSizeClasses; class_id++) {
       u8 *flags = GetFlags(stack_size_log(), class_id);
  @@ -148,7 +149,9 @@ NOINLINE void FakeStack::GC(uptr real_stack) {
         if (flags[i] == 0) continue;  // not allocated.
         FakeFrame *ff = reinterpret_cast<FakeFrame *>(
             GetFrame(stack_size_log(), class_id, i));
  +      Printf("Checking FakeFrame idx %lld (flags: %x): %p\n", i, flags[i], ff);
         if (ff->real_stack < real_stack) {
  +        Printf("Collecting!\n");
           flags[i] = 0;
           collected++;
         }
  @@ -206,12 +209,14 @@ ALWAYS_INLINE uptr OnMalloc(uptr class_id, uptr size) {
     uptr real_stack = reinterpret_cast<uptr>(&local_stack);
     FakeFrame *ff = fs->Allocate(fs->stack_size_log(), class_id, real_stack);
     if (!ff) return 0;  // Out of fake stack.
  +  Printf("Allocated FakeFrame %p\n", ff);
     uptr ptr = reinterpret_cast<uptr>(ff);
     SetShadow(ptr, size, class_id, 0);
     return ptr;
   }
   
   ALWAYS_INLINE void OnFree(uptr ptr, uptr class_id, uptr size) {
  +  Printf("Deallocating FakeFrame %p\n", ptr);
     FakeStack::Deallocate(ptr, class_id);
     SetShadow(ptr, size, class_id, kMagic8);
   }
  @@ -220,14 +225,16 @@ ALWAYS_INLINE void OnFree(uptr ptr, uptr class_id, uptr size) {
   
   // ---------------------- Interface ---------------- {{{1
   using namespace __asan;
  -#define DEFINE_STACK_MALLOC_FREE_WITH_CLASS_ID(class_id)                       \
  -  extern "C" SANITIZER_INTERFACE_ATTRIBUTE uptr                                \
  -      __asan_stack_malloc_##class_id(uptr size) {                              \
  -    return OnMalloc(class_id, size);                                           \
  -  }                                                                            \
  -  extern "C" SANITIZER_INTERFACE_ATTRIBUTE void __asan_stack_free_##class_id(  \
  -      uptr ptr, uptr size) {                                                   \
  -    OnFree(ptr, class_id, size);                                               \
  +#define DEFINE_STACK_MALLOC_FREE_WITH_CLASS_ID(class_id)                      \
  +  extern "C" SANITIZER_INTERFACE_ATTRIBUTE uptr                               \
  +      __asan_stack_malloc_##class_id(uptr size) {                             \
  +    Printf("stack_malloc_" #class_id "(size=%llx)\n", size);                  \
  +    return OnMalloc(class_id, size);                                          \
  +  }                                                                           \
  +  extern "C" SANITIZER_INTERFACE_ATTRIBUTE void __asan_stack_free_##class_id( \
  +      uptr ptr, uptr size) {                                                  \
  +    Printf("stack_free_" #class_id "(ptr=%p, size=%llx)\n", ptr, size);       \
  +    OnFree(ptr, class_id, size);                                              \
     }
   
   DEFINE_STACK_MALLOC_FREE_WITH_CLASS_ID(0)


http://reviews.llvm.org/D20913