Merge "SnoopLog: make sure mode is initialized" into main

5 files changed, 33 insertions, 49 deletions

diff --git a/system/gd/hal/snoop_logger.cc b/system/gd/hal/snoop_logger.cc
index 6b9fc8a2d1..0fc1939d7c 100644
--- a/system/gd/hal/snoop_logger.cc
+++ b/system/gd/hal/snoop_logger.cc
@@ -23,7 +23,7 @@
 #include <com_android_bluetooth_flags.h>
 #ifdef __ANDROID__
 #include <cutils/trace.h>
-#endif // __ANDROID__
+#endif  // __ANDROID__
 #include <sys/stat.h>
 
 #include <algorithm>
@@ -469,8 +469,6 @@ const std::string SnoopLogger::kBtSnoopLogFilterProfileModeMagic = "magic";
 // PBAP, MAP and HFP packets filter mode - disabled
 const std::string SnoopLogger::kBtSnoopLogFilterProfileModeDisabled = "disabled";
 
-std::string SnoopLogger::btsnoop_mode_;
-
 // Consts accessible in unit tests
 const size_t SnoopLogger::PACKET_TYPE_LENGTH = 1;
 const size_t SnoopLogger::MAX_HCI_ACL_LEN = 14;
@@ -482,7 +480,8 @@ SnoopLogger::SnoopLogger(std::string snoop_log_path, std::string snooz_log_path,
                          const std::chrono::milliseconds snooz_log_life_time,
                          const std::chrono::milliseconds snooz_log_delete_alarm_interval,
                          bool snoop_log_persists)
-    : snoop_log_path_(std::move(snoop_log_path)),
+    : btsnoop_mode_(btsnoop_mode),
+      snoop_log_path_(std::move(snoop_log_path)),
       snooz_log_path_(std::move(snooz_log_path)),
       max_packets_per_file_(max_packets_per_file),
       btsnooz_buffer_(max_packets_per_buffer),
@@ -490,8 +489,6 @@ SnoopLogger::SnoopLogger(std::string snoop_log_path, std::string snooz_log_path,
       snooz_log_life_time_(snooz_log_life_time),
       snooz_log_delete_alarm_interval_(snooz_log_delete_alarm_interval),
       snoop_log_persists(snoop_log_persists) {
-  btsnoop_mode_ = btsnoop_mode;
-
   if (btsnoop_mode_ == kBtSnoopLogModeFiltered) {
     log::info("Snoop Logs filtered mode enabled");
     EnableFilters();
@@ -566,9 +563,6 @@ void SnoopLogger::OpenNextSnoopLogFile() {
 }
 
 void SnoopLogger::EnableFilters() {
-  if (btsnoop_mode_ != kBtSnoopLogModeFiltered) {
-    return;
-  }
   std::lock_guard<std::mutex> lock(snoop_log_filters_mutex);
   for (auto itr = kBtSnoopLogFilterState.begin(); itr != kBtSnoopLogFilterState.end(); itr++) {
     auto filter_enabled_property = os::GetSystemProperty(itr->first);
@@ -1135,7 +1129,7 @@ void SnoopLogger::Capture(const HciPacket& immutable_packet, Direction direction
   if (com::android::bluetooth::flags::snoop_logger_tracing()) {
     LogTracePoint(packet, direction, type);
   }
-  #endif // __ANDROID__
+#endif  // __ANDROID__
 
   uint64_t timestamp_us = std::chrono::duration_cast<std::chrono::microseconds>(
                                   std::chrono::system_clock::now().time_since_epoch())
@@ -1305,7 +1299,7 @@ void SnoopLogger::Stop() {
     socket_ = nullptr;
   }
 
-  btsnoop_mode_.clear();
+  btsnoop_mode_ = kBtSnoopLogModeDisabled;
   // Disable all filters
   DisableFilters();
 
@@ -1350,36 +1344,26 @@ size_t SnoopLogger::GetMaxPacketsPerBuffer() {
   return btsnooz_max_memory_usage_bytes / kDefaultBtSnoozMaxBytesPerPacket;
 }
 
-std::string SnoopLogger::GetBtSnoopMode() {
+std::string SnoopLogger::GetCurrentSnoopMode() { return btsnoop_mode_; }
+
+static std::string GetBtSnoopMode() {
   // Default mode is FILTERED on userdebug/eng build, DISABLED on user build.
   // In userdebug/eng build, it can also be overwritten by modifying the global setting
-  std::string default_mode = kBtSnoopLogModeDisabled;
-  {
-    auto is_debuggable = os::GetSystemPropertyBool(kIsDebuggableProperty, false);
-    if (is_debuggable) {
-      auto default_mode_property = os::GetSystemProperty(kBtSnoopDefaultLogModeProperty);
-      if (default_mode_property) {
-        default_mode = std::move(default_mode_property.value());
-      } else {
-        default_mode = kBtSnoopLogModeFiltered;
-      }
-    }
+  std::string btsnoop_mode = SnoopLogger::kBtSnoopLogModeDisabled;
+  if (os::GetSystemPropertyBool(SnoopLogger::kIsDebuggableProperty, false)) {
+    btsnoop_mode = os::GetSystemProperty(SnoopLogger::kBtSnoopDefaultLogModeProperty)
+                           .value_or(SnoopLogger::kBtSnoopLogModeFiltered);
   }
 
-  // Get the actual mode if exist
-  std::string btsnoop_mode = default_mode;
-  {
-    auto btsnoop_mode_prop = os::GetSystemProperty(kBtSnoopLogModeProperty);
-    if (btsnoop_mode_prop) {
-      btsnoop_mode = std::move(btsnoop_mode_prop.value());
-    }
-  }
+  btsnoop_mode = os::GetSystemProperty(SnoopLogger::kBtSnoopLogModeProperty).value_or(btsnoop_mode);
 
-  // If Snoop Logger already set up, return current mode
-  bool btsnoop_mode_empty = btsnoop_mode_.empty();
-  log::info("btsnoop_mode_empty: {}", btsnoop_mode_empty);
-  if (!btsnoop_mode_empty) {
-    return btsnoop_mode_;
+  // Only allow a subset of values:
+  if (!(btsnoop_mode == SnoopLogger::kBtSnoopLogModeDisabled ||
+        btsnoop_mode == SnoopLogger::kBtSnoopLogModeFull ||
+        btsnoop_mode == SnoopLogger::kBtSnoopLogModeFiltered ||
+        btsnoop_mode == SnoopLogger::kBtSnoopLogModeKernel)) {
+    log::warn("{}: Invalid btsnoop value, default back to disabled", btsnoop_mode);
+    return SnoopLogger::kBtSnoopLogModeDisabled;
   }
 
   return btsnoop_mode;
@@ -1463,7 +1447,7 @@ void SnoopLogger::LogTracePoint(const HciPacket& packet, Direction direction, Pa
     } break;
   }
 }
-#endif // __ANDROID__
+#endif  // __ANDROID__
 
 }  // namespace hal
 }  // namespace bluetooth
diff --git a/system/gd/hal/snoop_logger.h b/system/gd/hal/snoop_logger.h
index 3566630e70..bcd406cc9d 100644
--- a/system/gd/hal/snoop_logger.h
+++ b/system/gd/hal/snoop_logger.h
@@ -199,9 +199,8 @@ public:
 
   static size_t GetMaxPacketsPerBuffer();
 
-  // Get snoop logger mode based on current system setup
-  // Changes to this values is only effective after restarting Bluetooth
-  static std::string GetBtSnoopMode();
+  // Get current snoop logger mode
+  std::string GetCurrentSnoopMode();
 
   // Returns whether the soc manufacturer is Qualcomm
   // Changes to this value is only effective after restarting Bluetooth
@@ -232,7 +231,7 @@ public:
 
   // Set a RFCOMM dlci as acceptlisted, allowing packets with that RFCOMM CID
   // to show up in the snoop logs. The local_cid is used to associate it with
-  // its corrisponding ACL connection. The dlci is the channel with direction
+  // its corresponding ACL connection. The dlci is the channel with direction
   // so there is no chance of a collision if two services are using the same
   // channel but in different directions.
   void AcceptlistRfcommDlci(uint16_t conn_handle, uint16_t local_cid, uint8_t dlci);
@@ -305,7 +304,7 @@ protected:
   uint32_t FilterProfiles(bool is_received, uint8_t* packet);
   // Check if packet is A2DP media packet (a2dppktsfiltered mode)
   bool IsA2dpMediaPacket(bool is_received, uint8_t* packet);
-  // Chec if channel is cached in snoop logger for filtering (a2dppktsfiltered mode)
+  // Check if channel is cached in snoop logger for filtering (a2dppktsfiltered mode)
   bool IsA2dpMediaChannel(uint16_t conn_handle, uint16_t cid, bool is_local_cid);
   // Handle HFP filtering while profilesfiltered enabled
   uint32_t FilterProfilesHandleHfp(uint8_t* packet, uint32_t length, uint32_t totlen,
@@ -319,12 +318,12 @@ protected:
 
   std::unique_ptr<SnoopLoggerSocketThread> snoop_logger_socket_thread_;
 
-  #ifdef __ANDROID__
+#ifdef __ANDROID__
   void LogTracePoint(const HciPacket& packet, Direction direction, PacketType type);
-  #endif // __ANDROID__
+#endif  // __ANDROID__
 
 private:
-  static std::string btsnoop_mode_;
+  std::string btsnoop_mode_;
   std::string snoop_log_path_;
   std::string snooz_log_path_;
   std::ofstream btsnoop_ostream_;
diff --git a/system/stack/fuzzers/l2cap_fuzzer.cc b/system/stack/fuzzers/l2cap_fuzzer.cc
index 2c5c3d2cde..1963983aef 100644
--- a/system/stack/fuzzers/l2cap_fuzzer.cc
+++ b/system/stack/fuzzers/l2cap_fuzzer.cc
@@ -85,7 +85,7 @@ class SnoopLogger;
 
 const std::string SnoopLogger::kBtSnoopLogModeFiltered = "filtered";
 
-std::string SnoopLogger::GetBtSnoopMode() { return "filtered"; }
+std::string SnoopLogger::GetCurrentSnoopMode() { return "filtered"; }
 void SnoopLogger::AcceptlistL2capChannel(uint16_t, uint16_t, uint16_t) {}
 void SnoopLogger::AddA2dpMediaChannel(uint16_t, uint16_t, uint16_t) {}
 void SnoopLogger::AddRfcommL2capChannel(uint16_t, uint16_t, uint16_t) {}
diff --git a/system/stack/l2cap/internal/l2c_api.h b/system/stack/l2cap/internal/l2c_api.h
index 6f65f81d71..5ed3403cff 100644
--- a/system/stack/l2cap/internal/l2c_api.h
+++ b/system/stack/l2cap/internal/l2c_api.h
@@ -75,7 +75,7 @@ typedef uint8_t tL2CAP_CHNL_DATA_RATE;
 #define L2CAP_FCR_CHAN_OPT_ALL_MASK (L2CAP_FCR_CHAN_OPT_BASIC | L2CAP_FCR_CHAN_OPT_ERTM)
 
 /* Validity check for PSM.  PSM values must be odd.  Also, all PSM values must
- * be assigned such that the least significant bit of the most sigificant
+ * be assigned such that the least significant bit of the most significant
  * octet equals zero.
  */
 #define L2C_INVALID_PSM(psm) (((psm) & 0x0101) != 0x0001)
diff --git a/system/stack/l2cap/l2c_api.cc b/system/stack/l2cap/l2c_api.cc
index a233046a74..b9587454f1 100644
--- a/system/stack/l2cap/l2c_api.cc
+++ b/system/stack/l2cap/l2c_api.cc
@@ -24,7 +24,7 @@
 
 #define LOG_TAG "bt_l2cap"
 
-#include "stack/l2cap/internal/l2c_api.h"
+#include "stack/l2cap/l2c_api.h"
 
 #include <base/location.h>
 #include <base/strings/stringprintf.h>
@@ -47,6 +47,7 @@
 #include "stack/include/btm_client_interface.h"
 #include "stack/include/l2cap_module.h"
 #include "stack/include/main_thread.h"
+#include "stack/l2cap/internal/l2c_api.h"
 #include "stack/l2cap/l2c_int.h"
 #include "types/raw_address.h"
 
@@ -1639,7 +1640,7 @@ void L2CA_SetMediaStreamChannel(uint16_t local_media_cid, bool status) {
     return;
   }
 
-  if (snoop_logger->GetBtSnoopMode() != snoop_logger->kBtSnoopLogModeFiltered) {
+  if (snoop_logger->GetCurrentSnoopMode() != snoop_logger->kBtSnoopLogModeFiltered) {
     return;
   }