pw_bluetooth: Remove scratch buffer from snoop log stack

Rather than creating a large buffer on the stack, which could overflow the stack create a member buffer for creating records. Bug: 375198513 Test: presubmit (unit tests) Change-Id: Ib87b5601ece07da14f3d56ccb6b5c4ebc1fb448e Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/265832 Reviewed-by: David Rees <[email protected]> Commit-Queue: Alan Rosenthal <[email protected]> Docs-Not-Needed: Alan Rosenthal <[email protected]> Lint: Lint 🤖 <[email protected]> Reviewed-by: Ben Lawson <[email protected]>
google · Feb 19, 2025 · febc8c2 · febc8c2
1 parent cf933ea
commit febc8c2
Show file tree

Hide file tree

Showing 5 changed files with 85 additions and 42 deletions.
diff --git a/pw_bluetooth/BUILD.bazel b/pw_bluetooth/BUILD.bazel
@@ -139,6 +139,7 @@ cc_library(
         "//pw_result",
         "//pw_span",
         "//pw_sync:mutex",
+        "//pw_sync:virtual_basic_lockable",
     ],
 )
 

diff --git a/pw_bluetooth/BUILD.gn b/pw_bluetooth/BUILD.gn
@@ -158,10 +158,12 @@ pw_source_set("snoop") {
     "$dir_pw_chrono:system_clock",
     "$dir_pw_containers:inline_var_len_entry_queue",
     "$dir_pw_sync:mutex",
+    "$dir_pw_sync:virtual_basic_lockable",
     dir_pw_assert,
     dir_pw_bluetooth_proxy,
     dir_pw_hex_dump,
     dir_pw_log,
+    dir_pw_result,
   ]
 }
 
@@ -302,6 +304,7 @@ pw_test_group("tests") {
     ":uuid_test",
     ":emboss_test",
     ":emboss_util_test",
+    ":snoop_test",
   ]
 }
 
@@ -408,5 +411,8 @@ pw_test("snoop_test") {
       dir_pw_third_party_emboss != "" && pw_chrono_SYSTEM_CLOCK_BACKEND != ""
   sources = [ "snoop_test.cc" ]
   include_dirs = [ "." ]
-  deps = [ ":snoop" ]
+  deps = [
+    ":snoop",
+    "$dir_pw_chrono:simulated_system_clock",
+  ]
 }
diff --git a/pw_bluetooth/CMakeLists.txt b/pw_bluetooth/CMakeLists.txt
@@ -273,12 +273,14 @@ pw_add_library(pw_bluetooth.snoop STATIC
   PUBLIC_DEPS
     pw_assert.check
     pw_bluetooth_proxy
+    pw_bluetooth.emboss_snoop
     pw_chrono.system_clock
     pw_containers.inline_var_len_entry_queue
     pw_hex_dump
     pw_log
     pw_sync.mutex
-    pw_bluetooth.emboss_snoop
+    pw_sync.virtual_basic_lockable
+    pw_result
 )
 
 pw_target_link_targets(pw_bluetooth

diff --git a/pw_bluetooth/public/pw_bluetooth/snoop.h b/pw_bluetooth/public/pw_bluetooth/snoop.h
@@ -22,19 +22,43 @@
 #include "pw_bytes/span.h"
 #include "pw_chrono/system_clock.h"
 #include "pw_containers/inline_var_len_entry_queue.h"
+#include "pw_result/result.h"
+#include "pw_span/span.h"
 #include "pw_status/status.h"
 #include "pw_sync/lock_annotations.h"
 #include "pw_sync/mutex.h"
+#include "pw_sync/virtual_basic_lockable.h"
 
 namespace pw::bluetooth {
 
 /// Snoop will record Rx & Tx transactions in a circular buffer. The most
 /// recent transactions are saved when the buffer is full.
+///
+/// @param system_clock system clock to use
+/// @param queue queue to hold all records
+/// @param lock lock to hold while accessing queue
+/// @param scratch_buffer buffer used for generation of each record. If a record
+/// is larger than the scratch buffer, the record will be truncated.
 class Snoop {
  public:
-  Snoop(chrono::VirtualSystemClock& system_clock,
-        InlineVarLenEntryQueue<>& queue)
-      : system_clock_(system_clock), queue_(queue) {}
+  static pw::Result<Snoop> Create(chrono::VirtualSystemClock& system_clock,
+                                  InlineVarLenEntryQueue<>& queue,
+                                  pw::sync::VirtualBasicLockable& queue_lock,
+                                  span<uint8_t> scratch_buffer) {
+    if (scratch_buffer.size() <
+        emboss::snoop_log::EntryHeader::MaxSizeInBytes()) {
+      return Status::FailedPrecondition();
+    }
+    return Snoop(system_clock, queue, queue_lock, scratch_buffer);
+  }
+
+  // Calculate the size of the scratch buffer.
+  ///
+  /// @param hci_payload_size The number of bytes of the hci packet to save
+  static constexpr size_t NeededScratchBufferSize(size_t hci_payload_size) {
+    return emboss::snoop_log::EntryHeader::MaxSizeInBytes() + /* hci type */ 1 +
+           hci_payload_size;
+  }
 
   // Dump the snoop log to the log as a hex string
   Status DumpToLog();
@@ -48,7 +72,7 @@ class Snoop {
   ///
   /// @param callback callback to invoke
   Status Dump(const Function<Status(ConstByteSpan data)>& callback) {
-    std::lock_guard lock(queue_mutex_);
+    std::lock_guard lock(queue_lock_);
     return DumpUnlocked(callback);
   }
 
@@ -65,16 +89,44 @@ class Snoop {
   Status DumpUnlocked(const Function<Status(ConstByteSpan data)>& callback)
       PW_NO_LOCK_SAFETY_ANALYSIS;
 
+  /// Add a Tx transaction
+  ///
+  /// @param packet Packet to save to snoop log
+  void AddTx(proxy::H4PacketInterface& packet) {
+    AddEntry(emboss::snoop_log::PacketFlags::SENT, packet);
+  }
+
+  // Add an Rx transaction
+  ///
+  /// @param packet Packet to save to snoop log
+  void AddRx(proxy::H4PacketInterface& packet) {
+    AddEntry(emboss::snoop_log::PacketFlags::RECEIVED, packet);
+  }
+
+ protected:
+  /// @param system_clock system clock to use
+  /// @param queue queue to hold all records
+  /// @param lock lock to hold while accessing queue
+  /// @param scratch_buffer buffer used for generation of each record. If a
+  /// record is larger than the scratch buffer, the record will be truncated.
+  Snoop(chrono::VirtualSystemClock& system_clock,
+        InlineVarLenEntryQueue<>& queue,
+        pw::sync::VirtualBasicLockable& queue_lock,
+        span<uint8_t> scratch_buffer)
+      : system_clock_(system_clock),
+        queue_(queue),
+        scratch_buffer_(scratch_buffer),
+        queue_lock_(queue_lock) {}
+
+ private:
   /// Add an entry to the snoop log
   ///
   /// @param packet_flag Packet flags (rx/tx)
   /// @param packet Packet to save to snoop log
   /// @param scratch_entry scratch buffer used to assemble the entry
   void AddEntry(emboss::snoop_log::PacketFlags emboss_packet_flag,
-                proxy::H4PacketInterface& hci_packet,
-                span<uint8_t> scratch_entry);
+                proxy::H4PacketInterface& hci_packet);
 
- private:
   /// Generates the snoop log file header and sends it to the callback
   ///
   /// @param callback callback to invoke
@@ -83,8 +135,9 @@ class Snoop {
 
   constexpr static uint32_t kEmbossFileVersion = 1;
   chrono::VirtualSystemClock& system_clock_;
-  InlineVarLenEntryQueue<>& queue_ PW_GUARDED_BY(queue_mutex_);
-  sync::Mutex queue_mutex_;
+  InlineVarLenEntryQueue<>& queue_ PW_GUARDED_BY(queue_lock_);
+  span<uint8_t> scratch_buffer_ PW_GUARDED_BY(queue_lock_);
+  pw::sync::VirtualBasicLockable& queue_lock_;
 };
 
 /// SnoopBuffer is a buffer backed snoop log.
@@ -95,30 +148,13 @@ template <size_t kTotalSize, size_t kMaxHciPacketSize>
 class SnoopBuffer : public Snoop {
  public:
   SnoopBuffer(chrono::VirtualSystemClock& system_clock)
-      : Snoop(system_clock, queue_buffer_) {}
-
-  /// Add a Tx transaction
-  ///
-  /// @param packet Packet to save to snoop log
-  void AddTx(proxy::H4PacketInterface& packet) {
-    std::array<uint8_t, kScratchEntrySize> entry{};
-    AddEntry(emboss::snoop_log::PacketFlags::SENT, packet, entry);
-  }
-
-  // Add an Rx transaction
-  ///
-  /// @param packet Packet to save to snoop log
-  void AddRx(proxy::H4PacketInterface& packet) {
-    std::array<uint8_t, kScratchEntrySize> entry{};
-    AddEntry(emboss::snoop_log::PacketFlags::RECEIVED, packet, entry);
-  }
+      : Snoop(system_clock, queue_buffer_, queue_mutex_, scratch_buffer_) {}
 
  private:
-  // Entry max size
-  constexpr static size_t kScratchEntrySize =
-      emboss::snoop_log::EntryHeader::MaxSizeInBytes() + /* hci type */ 1 +
-      kMaxHciPacketSize;
+  std::array<uint8_t, Snoop::NeededScratchBufferSize(kMaxHciPacketSize)>
+      scratch_buffer_{};
   InlineVarLenEntryQueue<kTotalSize> queue_buffer_;
+  sync::VirtualMutex queue_mutex_;
 };
 
 }  // namespace pw::bluetooth
diff --git a/pw_bluetooth/snoop.cc b/pw_bluetooth/snoop.cc
@@ -98,25 +98,22 @@ Status Snoop::DumpUnlocked(
 }
 
 void Snoop::AddEntry(emboss::snoop_log::PacketFlags emboss_packet_flag,
-                     proxy::H4PacketInterface& hci_packet,
-                     span<uint8_t> scratch_entry) {
-  std::lock_guard lock(queue_mutex_);
+                     proxy::H4PacketInterface& hci_packet) {
+  std::lock_guard lock(queue_lock_);
 
-  // Ensure scratch_entry can fit the entire header
-  PW_CHECK_INT_GT(scratch_entry.size(),
-                  emboss::snoop_log::EntryHeader::MaxSizeInBytes());
   size_t hci_packet_length_to_include = std::min(
       hci_packet.GetHciSpan().size(),
-      static_cast<size_t>(scratch_entry.size() -
+      static_cast<size_t>(scratch_buffer_.size() -
                           emboss::snoop_log::EntryHeader::MaxSizeInBytes() -
                           /* hci type*/ 1));
   size_t total_entry_size = hci_packet_length_to_include + /* hci type*/ 1 +
                             emboss::snoop_log::EntryHeader::MaxSizeInBytes();
   // Ensure the scratch buffer can fit the entire entry
-  PW_CHECK_INT_GE(scratch_entry.size(), total_entry_size);
+  PW_CHECK_INT_GE(scratch_buffer_.size(), total_entry_size);
 
   pw::Result<emboss::snoop_log::EntryWriter> result =
-      MakeEmbossWriter<emboss::snoop_log::EntryWriter>(scratch_entry);
+      MakeEmbossWriter<emboss::snoop_log::EntryWriter>(scratch_buffer_);
+  PW_CHECK_OK(result);
   emboss::snoop_log::EntryWriter writer = result.value();
   writer.header().original_length().Write(hci_packet.GetHciSpan().size() +
                                           /* hci type*/ 1);
@@ -140,7 +137,8 @@ void Snoop::AddEntry(emboss::snoop_log::PacketFlags emboss_packet_flag,
                                    /*src=*/hci_packet_trimmed));
 
   // save the entry!
-  queue_.push_overwrite(as_bytes(span{scratch_entry.data(), total_entry_size}));
+  queue_.push_overwrite(
+      as_bytes(span{scratch_buffer_.data(), total_entry_size}));
 }
 
 /// Generates the snoop log file header