commit e4a0a67c425eec93c924d83edb446572f9ab4277
author Alexei Frolov <frolv@google.com> Mon Jan 10 18:09:42 2022 -0800
committer CQ Bot Account <pigweed-scoped@luci-project-accounts.iam.gserviceaccount.com> Tue Jan 11 02:41:17 2022 +0000
tree d2cbae9e5a167836ca395e6575f64d39d599e1e7
parent 3ce31ccbd3cad5b72fdf103c7dcd5969487b14ae

pw_rpc: Don't hold the RPC lock when calling AcquireBuffer

This updates RPC calls to release their buffer before calling
Channel::AcquireBuffer to prevent potential deadlocks if the
ChannelOutput has its own locking.

Change-Id: Ia9ee156ba3ddf48e8e5bb445549e7a89f45e5885
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/77960
Pigweed-Auto-Submit: Alexei Frolov <frolv@google.com>
Reviewed-by: Wyatt Hepler <hepler@google.com>
Commit-Queue: Auto-Submit <auto-submit@pigweed.google.com.iam.gserviceaccount.com>

pw_rpc/call.cc

diff --git a/pw_rpc/call.cc b/pw_rpc/call.cc
index 13e3dea..b49e900 100644
--- a/pw_rpc/call.cc
+++ b/pw_rpc/call.cc
@@ -112,16 +112,30 @@
   return SendPacket(type, response, status);
 }
 
-ByteSpan Call::PayloadBufferLocked() {
+ByteSpan Call::PayloadBuffer() {
+  rpc_lock().lock();
+
   // Only allow having one active buffer at a time.
   if (response_.empty()) {
-    response_ = channel().AcquireBuffer();
+    Channel& c = channel();
+    rpc_lock().unlock();
+
+    // Don't call AcquireBuffer with rpc_lock() held, as this may cause deadlock
+    // if the channel is also protected by a mutex.
+    Channel::OutputBuffer buffer = c.AcquireBuffer();
+
+    rpc_lock().lock();
+    response_ = std::move(buffer);
   }
 
   // The packet type is only used to size the payload buffer.
   // TODO(pwrev/506): Replace the packet header calculation with a constant
   //     rather than creating a packet.
-  return response_.payload(MakePacket(PacketType::CLIENT_STREAM, {}));
+  ByteSpan buffer =
+      response_.payload(MakePacket(PacketType::CLIENT_STREAM, {}));
+  rpc_lock().unlock();
+
+  return buffer;
 }
 
 Status Call::Write(ConstByteSpan payload) {
@@ -139,7 +153,9 @@
   const Packet packet = MakePacket(type, payload, status);
 
   if (!buffer().Contains(payload)) {
-    ByteSpan buffer = PayloadBufferLocked();
+    rpc_lock().unlock();
+    ByteSpan buffer = PayloadBuffer();
+    rpc_lock().lock();
 
     if (payload.size() > buffer.size()) {
       ReleasePayloadBufferLocked();

pw_rpc/public/pw_rpc/internal/call.h

diff --git a/pw_rpc/public/pw_rpc/internal/call.h b/pw_rpc/public/pw_rpc/internal/call.h
index 1cdaab8..263bf47 100644
--- a/pw_rpc/public/pw_rpc/internal/call.h
+++ b/pw_rpc/public/pw_rpc/internal/call.h
@@ -160,10 +160,7 @@
 
   // Acquires a buffer into which to write a payload or returns a previously
   // acquired buffer. The Call MUST be active when this is called!
-  [[nodiscard]] ByteSpan PayloadBuffer() PW_LOCKS_EXCLUDED(rpc_lock()) {
-    LockGuard lock(rpc_lock());
-    return PayloadBufferLocked();
-  }
+  [[nodiscard]] ByteSpan PayloadBuffer() PW_LOCKS_EXCLUDED(rpc_lock());
 
   // Releases the buffer without sending a packet.
   void ReleasePayloadBuffer() PW_LOCKS_EXCLUDED(rpc_lock()) {
@@ -280,8 +277,6 @@
        MethodType type,
        CallType call_type);
 
-  [[nodiscard]] ByteSpan PayloadBufferLocked()
-      PW_EXCLUSIVE_LOCKS_REQUIRED(rpc_lock());
   void ReleasePayloadBufferLocked() PW_EXCLUSIVE_LOCKS_REQUIRED(rpc_lock());
 
   Packet MakePacket(PacketType type,