From 0ad25534177fc8ab9f54c78215502a79d36d0730 Mon Sep 17 00:00:00 2001
From: David Reiss <dreiss@fb.com>
Date: Mon, 9 Oct 2023 11:01:45 -0700
Subject: [PATCH] fix: use-after-return in synchronous calls (#362)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* fix: Use-after-return in synchronous calls

This bug was introduced by c39bc637b8df7791 and can be reproduced by
configuring with

  cmake -S . -B build -DBUILD_TESTS=yes -DCMAKE_CXX_COMPILER=clang++ \
    -DCMAKE_CXX_FLAGS="-Wno-error=deprecated-copy -fno-omit-frame-pointer -fsanitize=address -fsanitize-address-use-after-scope"

and running `cmake --buid build && cmake --build build -t test` or
`build/tests/sdbus-c++-integration-tests --gtest_filter=SdbusTestObject.HandlesCorrectlyABulkOfParallelServerSideAsyncMethods`

The issue is that `sdbus_async_reply_handler` can call `removeCall`, which
writes to `data->finished`, but `data` can point to the stack of
`sendMethodCallMessageAndWaitForReply`, which can return as soon as
`asyncCallData->callback` is called.

As a fix, I restored some of the logic removed in c39bc637b8df7791.
Specifically, in `sdbus_async_reply_handler`, I make a copy of some data
from `asyncCallData` (a new `state` field instead of `slot`), and in the
`SCOPE_GUARD`, I don't call `removeCall` if the call was actually
synchronous.

* refactor: use enum class instead of int

---------

Co-authored-by: David Reiss <dreiss@meta.com>
Co-authored-by: Stanislav Angelovič <stanislav.angelovic@protonmail.com>
---
 src/Proxy.cpp | 10 ++++++----
 src/Proxy.h   | 12 +++++++++---
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/src/Proxy.cpp b/src/Proxy.cpp
index 8a760ca..1b0c1db 100644
--- a/src/Proxy.cpp
+++ b/src/Proxy.cpp
@@ -120,7 +120,7 @@ PendingAsyncCall Proxy::callMethod(const MethodCall& message, async_reply_handle
     SDBUS_THROW_ERROR_IF(!message.isValid(), "Invalid async method call message provided", EINVAL);
 
     auto callback = (void*)&Proxy::sdbus_async_reply_handler;
-    auto callData = std::make_shared<AsyncCalls::CallData>(AsyncCalls::CallData{*this, std::move(asyncReplyCallback), {}});
+    auto callData = std::make_shared<AsyncCalls::CallData>(AsyncCalls::CallData{*this, std::move(asyncReplyCallback), {}, AsyncCalls::CallData::State::RUNNING});
     auto weakData = std::weak_ptr<AsyncCalls::CallData>{callData};
 
     callData->slot = message.send(callback, callData.get(), timeout);
@@ -162,7 +162,7 @@ MethodReply Proxy::sendMethodCallMessageAndWaitForReply(const MethodCall& messag
         syncCallReplyData.sendMethodReplyToWaitingThread(reply, error);
     };
     auto callback = (void*)&Proxy::sdbus_async_reply_handler;
-    AsyncCalls::CallData callData{*this, std::move(asyncReplyCallback), {}};
+    AsyncCalls::CallData callData{*this, std::move(asyncReplyCallback), {}, AsyncCalls::CallData::State::NOT_ASYNC};
 
     message.send(callback, &callData, timeout, floating_slot);
 
@@ -275,14 +275,16 @@ int Proxy::sdbus_async_reply_handler(sd_bus_message *sdbusMessage, void *userDat
     assert(asyncCallData != nullptr);
     assert(asyncCallData->callback);
     auto& proxy = asyncCallData->proxy;
+    auto state = asyncCallData->state;
 
     // We are removing the CallData item at the complete scope exit, after the callback has been invoked.
     // We can't do it earlier (before callback invocation for example), because CallBack data (slot release)
     // is the synchronization point between callback invocation and Proxy::unregister.
     SCOPE_EXIT
     {
-        // Remove call meta-data if it's a real async call (a sync call done in terms of async has slot == nullptr)
-        proxy.pendingAsyncCalls_.removeCall(asyncCallData);
+        // Remove call meta-data if it's a real async call (a sync call done in terms of async has STATE_NOT_ASYNC)
+        if (state != AsyncCalls::CallData::State::NOT_ASYNC)
+            proxy.pendingAsyncCalls_.removeCall(asyncCallData);
     };
 
     auto message = Message::Factory::create<MethodReply>(sdbusMessage, &proxy.connection_->getSdBusInterface());
diff --git a/src/Proxy.h b/src/Proxy.h
index f11f059..3208084 100644
--- a/src/Proxy.h
+++ b/src/Proxy.h
@@ -135,10 +135,16 @@ namespace sdbus::internal {
         public:
             struct CallData
             {
+                enum class State
+                {   NOT_ASYNC
+                ,   RUNNING
+                ,   FINISHED
+                };
+
                 Proxy& proxy;
                 async_reply_handler callback;
                 Slot slot;
-                bool finished { false };
+                State state;
             };
 
             ~AsyncCalls()
@@ -149,14 +155,14 @@ namespace sdbus::internal {
             void addCall(std::shared_ptr<CallData> asyncCallData)
             {
                 std::lock_guard lock(mutex_);
-                if (!asyncCallData->finished) // The call may have finished in the mean time
+                if (asyncCallData->state != CallData::State::FINISHED) // The call may have finished in the meantime
                     calls_.emplace_back(std::move(asyncCallData));
             }
 
             void removeCall(CallData* data)
             {
                 std::unique_lock lock(mutex_);
-                data->finished = true;
+                data->state = CallData::State::FINISHED;
                 if (auto it = std::find_if(calls_.begin(), calls_.end(), [data](auto const& entry){ return entry.get() == data; }); it != calls_.end())
                 {
                     auto callData = std::move(*it);