From e8a769f2c3fbcf07cd0659a3f695fb86df006b30 Mon Sep 17 00:00:00 2001
From: Ryan Zuklie <rzuklie@google.com>
Date: Wed, 18 Sep 2024 07:58:21 -0700
Subject: [PATCH] Fix deadlock from outside Perfetto mutex

Fixes the deadlock described by the below two thread states:

Thread 1:
- ConsumeAll() (takes mBufferMutex)
- perfetto::DataSource::Trace() (takes Perfetto mutex)

Thread 2:
- StartDataSourceImpl() (takes Perfetto mutex)
- NetworkTracePoller::Start() (takes mBufferMutex)

Locks should always be taken in the same order. To achieve this,
ConsumeAll only locks mBufferMutex while accessing the buffer, and
releases it before calling the callback (which under typical use calls
DataSource::Trace() as in "Thread 1").

As a fun side note, Start() always calls ConsumeAll(), so there exists a
lock sequence of perfetto mutex -> mBufferMutex -> perfetto mutex, thus
this order was always "exercised". However, Perfetto uses a recursive
mutex, so the second lock was a no-op, making this hard to catch.

Bug: 320700806
Test: atest libnetworkstats_test
Change-Id: If8c5e0e218666ec53a287ec56333a6d3b8cfe08f
---
 .../libnetworkstats/NetworkTracePoller.cpp    | 24 ++++++++++---------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/service-t/native/libs/libnetworkstats/NetworkTracePoller.cpp b/service-t/native/libs/libnetworkstats/NetworkTracePoller.cpp
index de927d7c05..9cca0785bf 100644
--- a/service-t/native/libs/libnetworkstats/NetworkTracePoller.cpp
+++ b/service-t/native/libs/libnetworkstats/NetworkTracePoller.cpp
@@ -149,18 +149,20 @@ void NetworkTracePoller::TraceIfaces(const std::vector<PacketTrace>& packets) {
 }
 
 bool NetworkTracePoller::ConsumeAll() {
-  std::scoped_lock<std::mutex> lock(mBufferMutex);
-  if (mRingBuffer == nullptr) {
-    ALOGW("Tracing is not active");
-    return false;
-  }
-
   std::vector<PacketTrace> packets;
-  base::Result<int> ret = mRingBuffer->ConsumeAll(
-      [&](const PacketTrace& pkt) { packets.push_back(pkt); });
-  if (!ret.ok()) {
-    ALOGW("Failed to poll ringbuf: %s", ret.error().message().c_str());
-    return false;
+  {
+    std::scoped_lock<std::mutex> lock(mBufferMutex);
+    if (mRingBuffer == nullptr) {
+      ALOGW("Tracing is not active");
+      return false;
+    }
+
+    base::Result<int> ret = mRingBuffer->ConsumeAll(
+        [&](const PacketTrace& pkt) { packets.push_back(pkt); });
+    if (!ret.ok()) {
+      ALOGW("Failed to poll ringbuf: %s", ret.error().message().c_str());
+      return false;
+    }
   }
 
   ATRACE_INT("NetworkTracePackets", packets.size());