From d1259628d4084573c27c772e5d3b99384e960337 Mon Sep 17 00:00:00 2001
From: Dmitry Prokoptsev <dmitryp@hudson-trading.com>
Date: Wed, 7 Aug 2024 10:17:23 -0400
Subject: [PATCH] Fix use-after-free in CBPortal.

There has been a heuristics to detect user bugs where a reference-like object
passed through CBPortal was not consumed timely. This turned out not to work
nicely, as in the most straightforward use case:

    const string& s = co_await untilCBCalled(
        [&](Callback<void(const string&)> cb) { /*...*/ };

-- the involved `CBPortal`, being a temporary, will get destroyed before
`CBPortal::wakeUp()` returns, leaving no opportunity to check whether the value
has been consumed.

The new test, `cb-reflike`, readily triggers a use-after-free when run with
`-fsanitize=address` (I, however, had to move `CBPortal` in heap in that test
to have adress sanitizer reliably detect the use-after-free).

Fix that by removing the heuristics.
---
 corral/CBPortal.h  | 17 +----------------
 test/basic_test.cc | 19 +++++++++++++++++++
 2 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/corral/CBPortal.h b/corral/CBPortal.h
index 63ab7f4..5b16c5c 100644
--- a/corral/CBPortal.h
+++ b/corral/CBPortal.h
@@ -48,15 +48,6 @@ template <> struct MaybeTuple<> {
     static void fromTuple(std::tuple<>&&) {}
 };
 
-// Heuristics for identifying arguments to the CBPortal bridge callback
-// that might be invalid to use after the callback has returned.
-template <class T> constexpr bool LooksLikeReference = false;
-template <class T> constexpr bool LooksLikeReference<T&> = true;
-template <class T> constexpr bool LooksLikeReference<T&&> = true;
-template <class T> constexpr bool LooksLikeReference<T*> = true;
-template <class T> constexpr bool LooksLikeReference<std::span<T>> = true;
-template <> constexpr inline bool LooksLikeReference<std::string_view> = true;
-
 template <class InitiateFn, class CancelFn, class... CBPortals>
 class CBPortalProxy;
 enum class CBPortalProxyStatus : uint8_t;
@@ -356,13 +347,7 @@ template <class... Ts>
 void CBPortal<Ts...>::Callback::operator()(Ts... values) const {
     CORRAL_ASSERT(!portal_->value_.has_value() && "consumer not keeping up");
     portal_->value_.emplace(std::forward<Ts>(values)...);
-    portal_->wakeUp();
-    if constexpr ((detail::LooksLikeReference<Ts> || ...)) {
-        CORRAL_ASSERT(!portal_->value_.has_value() &&
-                      "reference arguments will dangle if not consumed "
-                      "immediately; are you combining CBPortals using "
-                      "something other than anyOf() with only CBPortal args?");
-    }
+    portal_->wakeUp(); // may destroy `portal_`
 }
 
 template <class... Ts> void CBPortal<Ts...>::wakeUp() {
diff --git a/test/basic_test.cc b/test/basic_test.cc
index a2bdfbb..762c166 100644
--- a/test/basic_test.cc
+++ b/test/basic_test.cc
@@ -824,6 +824,25 @@ CORRAL_TEST_CASE("cb-return-type") {
     CATCH_CHECK(&rr == &x);
 }
 
+CORRAL_TEST_CASE("cb-reflike") {
+    std::function<void(const std::string&)> cb;
+    co_await allOf(
+            [&]() -> Task<> {
+                auto portal = std::make_unique<CBPortal<const std::string&>>();
+                std::string s = co_await untilCBCalled(
+                        [&](std::function<void(const std::string&)> c) {
+                            cb = c;
+                        },
+                        *portal);
+                CATCH_CHECK(s == "hello");
+                portal.reset();
+            },
+            [&]() -> Task<> {
+                co_await t.sleep(1ms);
+                cb("hello");
+            });
+}
+
 template <class T> class MyLightweightFn;
 template <class... Args> class MyLightweightFn<void(Args...)> {
     void (*fn_)(size_t, Args...);