Fix two address sanitizer bugs (#2078)

* Fix two address sanitizer bugs

- NetworkSignalHandler attempts to read int64_t from unaligned memory.
- Wrong destructor being called for SignalHandlers.

* Add FIELD_RAW_SAFE

This macro acts the same way FIELD_RAW, except it will use a
std::optional rather than a raw pointer. Values extracted with this new
macro need to be trivially copyable so they can be moved to a variable
with proper alignment.

Author: Molter73

collector/lib/NetworkSignalHandler.cpp

@@ -1,5 +1,6 @@
 #include "NetworkSignalHandler.h"
 
+#include <cstring>
 #include <optional>
 
 #include <libsinsp/sinsp.h>
@@ -88,8 +89,8 @@ std::optional<Connection> NetworkSignalHandler::GetConnection(sinsp_evt* evt) {
     }
   }
 
-  const int64_t* res = event_extractor_->get_event_rawres(evt);
-  if (!res || *res < 0) {
+  auto res = event_extractor_->get_event_rawres(evt);
+  if (!res.has_value() || res.value() < 0) {
     // ignore unsuccessful events for now.
     return std::nullopt;
   }

collector/lib/NetworkSignalHandler.h

@@ -20,7 +20,7 @@ class EventExtractor;
 class NetworkSignalHandler final : public SignalHandler {
  public:
   explicit NetworkSignalHandler(sinsp* inspector, std::shared_ptr<ConnectionTracker> conn_tracker, system_inspector::Stats* stats);
-  ~NetworkSignalHandler();
+  ~NetworkSignalHandler() override;
 
   std::string GetName() override { return "NetworkSignalHandler"; }
   Result HandleSignal(sinsp_evt* evt) override;

collector/lib/ProcessSignalHandler.h

@@ -30,6 +30,12 @@ class ProcessSignalHandler : public SignalHandler {
         stats_(stats),
         config_(config) {}
 
+  ProcessSignalHandler(const ProcessSignalHandler&) = delete;
+  ProcessSignalHandler(ProcessSignalHandler&&) = delete;
+  ProcessSignalHandler& operator=(const ProcessSignalHandler&) = delete;
+  ProcessSignalHandler& operator=(ProcessSignalHandler&&) = delete;
+  ~ProcessSignalHandler() override = default;
+
   bool Start() override;
   bool Stop() override;
   Result HandleSignal(sinsp_evt* evt) override;

collector/lib/SelfCheckHandler.cpp

@@ -56,10 +56,10 @@ SignalHandler::Result SelfCheckNetworkHandler::HandleSignal(sinsp_evt* evt) {
     return IGNORED;
   }
 
-  const uint16_t* server_port = event_extractor_->get_server_port(evt);
-  const uint16_t* client_port = event_extractor_->get_client_port(evt);
+  auto server_port = event_extractor_->get_server_port(evt);
+  auto client_port = event_extractor_->get_client_port(evt);
 
-  if (server_port == nullptr || client_port == nullptr) {
+  if (!server_port.has_value() | !client_port.has_value()) {
     return IGNORED;
   }
 

collector/lib/SignalHandler.h

@@ -25,6 +25,13 @@ class SignalHandler {
     FINISHED,
   };
 
+  SignalHandler() = default;
+  SignalHandler(const SignalHandler&) = default;
+  SignalHandler(SignalHandler&&) = delete;
+  SignalHandler& operator=(const SignalHandler&) = default;
+  SignalHandler& operator=(SignalHandler&&) = delete;
+  virtual ~SignalHandler() = default;
+
   virtual std::string GetName() = 0;
   virtual bool Start() { return true; }
   virtual bool Stop() { return true; }

collector/lib/system-inspector/EventExtractor.h

@@ -57,6 +57,30 @@ class EventExtractor {
  private:                                                                                                  \
   DECLARE_FILTER_CHECK(id, fieldname)
 
+// When using FIELD_RAW_SAFE, type needs to be trivially copyable so we
+// can move it to a properly aligned variable.
+#define FIELD_RAW_SAFE(id, fieldname, type)                                                                \
+ public:                                                                                                   \
+  const std::optional<type> get_##id(sinsp_evt* event) {                                                   \
+    static_assert(std::is_trivially_copyable_v<type>,                                                      \
+                  "Attempted to create FIELD_RAW_SAFE on non trivial type");                               \
+    uint32_t len;                                                                                          \
+    auto buf = filter_check_##id##_->extract_single(event, &len);                                          \
+    if (!buf) return {};                                                                                   \
+    if (len != sizeof(type)) {                                                                             \
+      CLOG_THROTTLED(WARNING, std::chrono::seconds(30))                                                    \
+          << "Failed to extract value for field " << fieldname << ": expected type " << #type << " (size " \
+          << sizeof(type) << "), but returned value has size " << len;                                     \
+      return {};                                                                                           \
+    }                                                                                                      \
+    type val;                                                                                              \
+    std::memcpy(&val, buf, sizeof(type));                                                                  \
+    return {val};                                                                                          \
+  }                                                                                                        \
+                                                                                                           \
+ private:                                                                                                  \
+  DECLARE_FILTER_CHECK(id, fieldname)
+
 #define FIELD_CSTR(id, fieldname)                                 \
  public:                                                          \
   const char* get_##id(sinsp_evt* event) {                        \
@@ -100,6 +124,7 @@ class EventExtractor {
   // - FIELD_CSTR(id, fieldname): exposes the system inspector field <fieldname> via get_<id>(), returning a null-terminated
   //   const char*.
   // - FIELD_RAW(id, fieldname, type): exposes the system inspector field <fieldname> via get_<id>(), returning a const <type>*.
+  // - FIELD_RAW_SAFE(id, fieldname, type): exposes the system inspector field <fieldname> via get_<id>(), returning a std::optional<type>.
   // - EVT_ARG(argname): shorthand for FIELD_CSTR(evt_arg_<argname>, "evt.arg.<argname>")
   // - EVT_ARG_RAW(argname, type): shorthand for FIELD_RAW(evt_arg_<argname>, "evt.rawarg.<argname>", <type>)
   //
@@ -118,11 +143,11 @@ class EventExtractor {
   FIELD_CSTR(proc_args, "proc.args");
 
   // General event information
-  FIELD_RAW(event_rawres, "evt.rawres", int64_t);
+  FIELD_RAW_SAFE(event_rawres, "evt.rawres", int64_t);
 
   // File/network related
-  FIELD_RAW(client_port, "fd.cport", uint16_t);
-  FIELD_RAW(server_port, "fd.sport", uint16_t);
+  FIELD_RAW_SAFE(client_port, "fd.cport", uint16_t);
+  FIELD_RAW_SAFE(server_port, "fd.sport", uint16_t);
 
   // k8s metadata
   FIELD_CSTR(k8s_namespace, "k8s.ns.name");