coroutines: miscompilation when using ternary operator and co_await (use after free)

[adrianimboden]

https://godbolt.org/z/avrxq5zb9

res ok(bool cond) {
    if (cond) {
        co_return res{co_await foo_error()};
    } else {
        co_return res{5};
    }
}

res notok(bool cond) {
    co_return cond ? res{co_await foo_error()} : res{5};
}

int main() {
    ok(false);    //ok
    ok(true);     //ok
    notok(false); //ok
    notok(true);  //crash
}

results in:

=================================================================
==1==ERROR: AddressSanitizer: heap-use-after-free on address 0x6070000001ae at pc 0x5555f4f0091e bp 0x7fff81013350 sp 0x7fff81013348
READ of size 1 at 0x6070000001ae thread T0
    #0 0x5555f4f0091d in notok(bool) /app/example.cpp:104:26
    #1 0x5555f4f013e5 in main /app/example.cpp:111:5
    #2 0x7f9da9a28082 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x24082) (BuildId: 1878e6b475720c7c51969e69ab2d276fae6d1dee)
    #3 0x5555f4e2739d in _start (/app/output.s+0x1f39d)

0x6070000001ae is located 62 bytes inside of 72-byte region [0x607000000170,0x6070000001b8)
freed by thread T0 here:
    #0 0x5555f4efd30d in operator delete(void*) /root/llvm-project/compiler-rt/lib/asan/asan_new_delete.cpp:152:3
    #1 0x5555f4f05442 in notok(bool) (.destroy) /app/example.cpp:103:5
    #2 0x5555f4f0a8f4 in std::__1::coroutine_handle<void>::destroy[abi:v160000]() const /opt/compiler-explorer/clang-trunk-20221123/bin/../include/c++/v1/__coroutine/coroutine_handle.h:84:9
    #3 0x5555f4f07554 in res_promise_type::await_transform(res)::Suspension::await_suspend(std::__1::coroutine_handle<void>) /app/example.cpp:73:22
    #4 0x5555f4f008da in notok(bool) /app/example.cpp:104:26
    #5 0x5555f4f013e5 in main /app/example.cpp:111:5
    #6 0x7f9da9a28082 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x24082) (BuildId: 1878e6b475720c7c51969e69ab2d276fae6d1dee)

previously allocated by thread T0 here:
    #0 0x5555f4efcaad in operator new(unsigned long) /root/llvm-project/compiler-rt/lib/asan/asan_new_delete.cpp:95:3
    #1 0x5555f4f00017 in notok(bool) /app/example.cpp:103:5
    #2 0x5555f4f013e5 in main /app/example.cpp:111:5
    #3 0x7f9da9a28082 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x24082) (BuildId: 1878e6b475720c7c51969e69ab2d276fae6d1dee)

SUMMARY: AddressSanitizer: heap-use-after-free /app/example.cpp:104:26 in notok(bool)
Shadow bytes around the buggy address:
  0x606fffffff00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x606fffffff80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x607000000000: fa fa fa fa fd fd fd fd fd fd fd fd fd fa fa fa
  0x607000000080: fa fa fd fd fd fd fd fd fd fd fd fa fa fa fa fa
  0x607000000100: fd fd fd fd fd fd fd fd fd fa fa fa fa fa fd fd
=>0x607000000180: fd fd fd fd fd[fd]fd fa fa fa fa fa fa fa fa fa
  0x607000000200: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x607000000280: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x607000000300: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x607000000380: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x607000000400: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==1==ABORTING

[llvmbot]

@llvm/issue-subscribers-coroutines

[apolloww]

It seems that "heap-use-after-free" only happens in asan build.

Looking at the unoptimized IR straight from clang https://godbolt.org/z/zv1cKP345, one of the markers %cleanup.cond26 that conditionally cleans up %ref.tmp25 (the coroutine_handler as the parameter of await_suspend call) is accessed after coro frame is destroyed inside res_promise_type::await_transform(res)::Suspension::await_suspend(std::coroutine_handle<>)

cleanup.cont:                                     ; preds = %cleanup
  %16 = load i8, ptr %cond1, align 1, !dbg !3222
  %tobool12 = trunc i8 %16 to i1, !dbg !3222
  store i1 false, ptr %cleanup.cond, align 1
  store i1 false, ptr %cleanup.cond17, align 1
  store i1 false, ptr %cleanup.cond20, align 1
  store i1 false, ptr %cleanup.cond26, align 1
  store i1 false, ptr %cleanup.cond40, align 1
  store i1 false, ptr %cleanup.cond43, align 1
  br i1 %tobool12, label %cond.true, label %cond.false, !dbg !3222

cond.true:                                        ; preds = %cleanup.cont
  call void @llvm.lifetime.start.p0(i64 16, ptr %ref.tmp13) #6, !dbg !3224
  store i1 true, ptr %cleanup.cond, align 1, !dbg !3224
  invoke void @_Z9foo_errorv(ptr sret(%struct.res) align 8 %agg.tmp14)
          to label %invoke.cont16 unwind label %lpad15, !dbg !3225

invoke.cont16:                                    ; preds = %cond.true
  store i1 true, ptr %cleanup.cond17, align 1, !dbg !3225
  invoke void @_ZN16res_promise_type15await_transformE3res(ptr sret(%struct.Suspension) align 8 %ref.tmp13, ptr noundef nonnull align 8 dereferenceable(8) %__promise, ptr noundef %agg.tmp14)
          to label %invoke.cont19 unwind label %lpad18, !dbg !3224

invoke.cont19:                                    ; preds = %invoke.cont16
  store i1 true, ptr %cleanup.cond20, align 1, !dbg !3224
  %call23 = invoke noundef zeroext i1 @_ZZN16res_promise_type15await_transformE3resEN10Suspension11await_readyEv(ptr noundef nonnull align 8 dereferenceable(16) %ref.tmp13)
          to label %invoke.cont22 unwind label %lpad21, !dbg !3224

invoke.cont22:                                    ; preds = %invoke.cont19
  br i1 %call23, label %await.ready, label %await.suspend, !dbg !3224

await.suspend:                                    ; preds = %invoke.cont22
  %17 = call token @llvm.coro.save(ptr null), !dbg !3224
  call void @llvm.lifetime.start.p0(i64 8, ptr %ref.tmp25) #6, !dbg !3224
  store i1 true, ptr %cleanup.cond26, align 1, !dbg !3224
  %call27 = call ptr @_ZNSt3__116coroutine_handleI16res_promise_typeE12from_addressB7v170000EPv(ptr noundef %5) #6, !dbg !3224
  %coerce.dive28 = getelementptr inbounds %"struct.std::__1::coroutine_handle.6", ptr %ref.tmp25, i32 0, i32 0, !dbg !3224
  store ptr %call27, ptr %coerce.dive28, align 8, !dbg !3224
  %call29 = call ptr @_ZNKSt3__116coroutine_handleI16res_promise_typeEcvNS0_IvEEB7v170000Ev(ptr noundef nonnull align 8 dereferenceable(8) %ref.tmp25) #6, !dbg !3224
  %coerce.dive30 = getelementptr inbounds %"struct.std::__1::coroutine_handle", ptr %agg.tmp24, i32 0, i32 0, !dbg !3224
  store ptr %call29, ptr %coerce.dive30, align 8, !dbg !3224
  %coerce.dive31 = getelementptr inbounds %"struct.std::__1::coroutine_handle", ptr %agg.tmp24, i32 0, i32 0, !dbg !3224
  %18 = load ptr, ptr %coerce.dive31, align 8, !dbg !3224
  invoke void @_ZZN16res_promise_type15await_transformE3resEN10Suspension13await_suspendENSt3__116coroutine_handleIvEE(ptr noundef nonnull align 8 dereferenceable(16) %ref.tmp13, ptr %18)
          to label %invoke.cont33 unwind label %lpad32, !dbg !3224

invoke.cont33:                                    ; preds = %await.suspend
  %cleanup.is_active = load i1, ptr %cleanup.cond26, align 1, !dbg !3224
  br i1 %cleanup.is_active, label %cleanup.action, label %cleanup.done, !dbg !3224

cleanup.action:                                   ; preds = %invoke.cont33
  call void @llvm.lifetime.end.p0(i64 8, ptr %ref.tmp25) #6, !dbg !3224
  br label %cleanup.done, !dbg !3224

cleanup.done:                                     ; preds = %cleanup.action, %invoke.cont33
  %19 = call i8 @llvm.coro.suspend(token %17, i1 false), !dbg !3224
  switch i8 %19, label %coro.ret [
    i8 0, label %await.ready
    i8 1, label %await.cleanup
  ], !dbg !3224

While for the non-asan build https://godbolt.org/z/GKMc5hv43, the same marker is not there, instead the lifetime of %20 is hoisted outside the conditional branch.

64:                                               ; preds = %62
  %65 = load i8, ptr %5, align 1, !dbg !3220
  %66 = trunc i8 %65 to i1, !dbg !3220
  store i1 false, ptr %15, align 1
  store i1 false, ptr %17, align 1
  store i1 false, ptr %18, align 1
  call void @llvm.lifetime.start.p0(i64 8, ptr %20) #6, !dbg !3220
  store i1 false, ptr %21, align 1
  store i1 false, ptr %22, align 1
  br i1 %66, label %67, label %103, !dbg !3220

67:                                               ; preds = %64
  call void @llvm.lifetime.start.p0(i64 16, ptr %14) #6, !dbg !3222
  store i1 true, ptr %15, align 1, !dbg !3222
  invoke void @_Z9foo_errorv(ptr sret(%struct.res) align 8 %16)
          to label %68 unwind label %83, !dbg !3223

68:                                               ; preds = %67
  store i1 true, ptr %17, align 1, !dbg !3223
  invoke void @_ZN16res_promise_type15await_transformE3res(ptr sret(%struct.Suspension) align 8 %14, ptr noundef nonnull align 8 dereferenceable(8) %6, ptr noundef %16)
          to label %69 unwind label %87, !dbg !3222

69:                                               ; preds = %68
  store i1 true, ptr %18, align 1, !dbg !3222
  %70 = invoke noundef zeroext i1 @_ZZN16res_promise_type15await_transformE3resEN10Suspension11await_readyEv(ptr noundef nonnull align 8 dereferenceable(16) %14)
          to label %71 unwind label %91, !dbg !3222

71:                                               ; preds = %69
  br i1 %70, label %99, label %72, !dbg !3222

72:                                               ; preds = %71
  %73 = call token @llvm.coro.save(ptr null), !dbg !3222
  %74 = call ptr @_ZNSt3__116coroutine_handleI16res_promise_typeE12from_addressB7v170000EPv(ptr noundef %36) #6, !dbg !3222
  %75 = getelementptr inbounds %"struct.std::__1::coroutine_handle.6", ptr %20, i32 0, i32 0, !dbg !3222
  store ptr %74, ptr %75, align 8, !dbg !3222
  %76 = call ptr @_ZNKSt3__116coroutine_handleI16res_promise_typeEcvNS0_IvEEB7v170000Ev(ptr noundef nonnull align 8 dereferenceable(8) %20) #6, !dbg !3222
  %77 = getelementptr inbounds %"struct.std::__1::coroutine_handle", ptr %19, i32 0, i32 0, !dbg !3222
  store ptr %76, ptr %77, align 8, !dbg !3222
  %78 = getelementptr inbounds %"struct.std::__1::coroutine_handle", ptr %19, i32 0, i32 0, !dbg !3222
  %79 = load ptr, ptr %78, align 8, !dbg !3222
  invoke void @_ZZN16res_promise_type15await_transformE3resEN10Suspension13await_suspendENSt3__116coroutine_handleIvEE(ptr noundef nonnull align 8 dereferenceable(16) %14, ptr %79)
          to label %80 unwind label %95, !dbg !3222

80:                                               ; preds = %72
  call void @llvm.lifetime.end.p0(i64 8, ptr %20) #6, !dbg !3222
  %81 = call i8 @llvm.coro.suspend(token %73, i1 false), !dbg !3222
  switch i8 %81, label %190 [
    i8 0, label %99
    i8 1, label %82
  ], !dbg !3222

And this all comes down from CodeGenFunction::EmitCoawaitExpr -> emitSuspendExpression -> ... -> CodeGenFunction::EmitMaterializeTemporaryExpr and some special treatment for asan https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CGExpr.cpp#L545

      // Avoid creating a conditional cleanup just to hold an llvm.lifetime.end
      // marker. Instead, start the lifetime of a conditional temporary earlier
      // so that it's unconditional. Don't do this with sanitizers which need
      // more precise lifetime marks.
      ConditionalEvaluation *OldConditional = nullptr;
      CGBuilderTy::InsertPoint OldIP;
      if (isInConditionalBranch() && !E->getType().isDestructedType() &&
          !SanOpts.has(SanitizerKind::HWAddress) &&
          !SanOpts.has(SanitizerKind::Memory) &&
          !CGM.getCodeGenOpts().SanitizeAddressUseAfterScope) {
        OldConditional = OutermostConditional;
        OutermostConditional = nullptr;

        OldIP = Builder.saveIP();
        llvm::BasicBlock *Block = OldConditional->getStartingBlock();
        Builder.restoreIP(CGBuilderTy::InsertPoint(
            Block, llvm::BasicBlock::iterator(Block->back())));
      }

      if (auto *Size = EmitLifetimeStart(
              CGM.getDataLayout().getTypeAllocSize(Alloca.getElementType()),
              Alloca.getPointer())) {
        pushFullExprCleanup<CallLifetimeEnd>(NormalEHLifetimeMarker, Alloca,
                                             Size);
      }
      ... ...

Note that IfStmt does not apply to this case because it is not a conditionally evaluated expression, so it never generates any of these markers.

  void pushFullExprCleanup(CleanupKind kind, As... A) {
    // If we're not in a conditional branch, or if none of the
    // arguments requires saving, then use the unconditional cleanup.
    if (!isInConditionalBranch())
      return EHStack.pushCleanup<T>(kind, A...);
      ... ...

If I remove the check for asan !CGM.getCodeGenOpts().SanitizeAddressUseAfterScope, asan build runs OK. We certainly don't want to do that to every MaterializeTemporaryExpr, I think. I wonder if we can limit the case to awaiter.await_suspendor CoawaitExpr.

cc: @ChuanqiXu9 , @bcardosolopes

[ChuanqiXu9]

Thanks for looking into this!

My primary concern is that how would the change affect the sanitizers. Will the sanitizer be less precise? Or will the sanitizer will report more false positive cases? I guess we can tolerant the first case as we are not able to model the lifetime model of C++ coroutines in the frontend precisely.

Then if it is the first case: my concern is that E may contain too many subexprs and it may be time-consuming to check if it contains CoroutineSuspendExpr in some edge cases. And I think it may not be too bad to avoid the marker directly if we're in a coroutine.

[apolloww]

Thanks for the feedback. I agree that we can't check every subexprs of coawait. I think we can set the condition to be

1. In ConditionalOperator (in this case it is a ternary operator)
2. In await.suspend block.

Made a quick fix: https://reviews.llvm.org/D144680