Fix USB transport io_service lifetime UAF and handshake stability

CMakeLists.txt

@@ -262,11 +262,19 @@ else()
             ${source_files}
             ${include_files})
 endif()
-target_include_directories(aasdk PUBLIC 
+if(SKIP_BUILD_PROTOBUF)
+    target_include_directories(aasdk PUBLIC
         $<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}/protobuf>
-    $<BUILD_INTERFACE:${protobuf_SOURCE_DIR}/src>
+        $<BUILD_INTERFACE:${Protobuf_INCLUDE_DIRS}>
         $<INSTALL_INTERFACE:include/aap_protobuf>
-)
+    )
+else()
+    target_include_directories(aasdk PUBLIC
+        $<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}/protobuf>
+        $<BUILD_INTERFACE:${protobuf_SOURCE_DIR}/src>
+        $<INSTALL_INTERFACE:include/aap_protobuf>
+    )
+endif()
 
 # Link libraries - aap_protobuf is always built as subdirectory
 target_link_libraries(aasdk PUBLIC
@@ -354,10 +362,26 @@ install(FILES
 install(FILES
     "${CMAKE_CURRENT_SOURCE_DIR}/cert/headunit.key"
     DESTINATION /etc/aasdk
-    PERMISSIONS OWNER_READ OWNER_WRITE
+    PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ
     COMPONENT runtime
 )
 
+# Align direct "make install" behavior with package postinst:
+# create/use aasdk group and assign cert/key group ownership + key group-read.
+install(CODE [[
+    set(_cert_dir "$ENV{DESTDIR}/etc/aasdk")
+    set(_cert_file "${_cert_dir}/headunit.crt")
+    set(_key_file "${_cert_dir}/headunit.key")
+
+    execute_process(
+        COMMAND /bin/sh -c "if ! getent group aasdk >/dev/null 2>&1; then if command -v addgroup >/dev/null 2>&1; then addgroup --system aasdk || true; elif command -v groupadd >/dev/null 2>&1; then groupadd --system aasdk || true; fi; fi"
+    )
+
+    execute_process(
+        COMMAND /bin/sh -c "if getent group aasdk >/dev/null 2>&1; then if [ -f '${_cert_file}' ]; then chown root:aasdk '${_cert_file}' || true; chmod 644 '${_cert_file}' || true; fi; if [ -f '${_key_file}' ]; then chown root:aasdk '${_key_file}' || true; chmod 640 '${_key_file}' || true; fi; fi"
+    )
+]] COMPONENT runtime)
+
 # Export the targets to a script
 install(EXPORT aasdkTargets
         FILE aasdkTargets.cmake

TROUBLESHOOTING.md

@@ -294,6 +294,47 @@ lsusb
 sudo chmod 666 /dev/bus/usb/001/002  # Replace with your device
 ```
 
+### USB Receive Retry Corruption
+
+#### Issue: transport retries eventually produce zero-length frames or channel storms
+```
+Repeated receive retries are followed by malformed frame parsing,
+zero-byte payload loops, or channel receive queues being rejected.
+```
+
+**Typical Symptoms:**
+
+1. `LIBUSB_ERROR_INTERRUPTED` (`native=-4` / `4294967292`) or transient `LIBUSB_ERROR_NO_DEVICE` during bulk IN receives.
+2. A later burst of bogus protocol data, often seen as zero-sized frames or repeated channel wakeups.
+3. Session teardown even though the original USB error looked transient.
+
+**Root Cause:**
+
+`DataSink::fill()` reserves a 16 KB receive slot before libusb writes into it. If the transfer fails and no matching `commit()` happens, that slot remains in the circular buffer unless it is explicitly rolled back. On the next receive cycle, the stale zero-filled slot can be exposed to `distributeReceivedData()` and parsed as real protocol bytes.
+
+**Fix:**
+
+1. Track whether a `fill()` is still pending.
+2. Call `rollback()` before re-arming a failed receive.
+3. Keep `fill()` self-healing so any missed rollback still discards the dangling slot.
+
+Relevant implementation points:
+
+1. `include/aasdk/Transport/DataSink.hpp`
+2. `src/Transport/DataSink.cpp`
+3. `src/Transport/USBTransport.cpp`
+
+**Validation:**
+```bash
+# Enable verbose transport logs and watch for receive retries.
+export AASDK_LOG_LEVEL=DEBUG
+export AASDK_VERBOSE_USB=1
+
+# The important invariant is that every failed receive path either calls
+# rollback() explicitly or reaches a later fill() that auto-discards the
+# uncommitted slot before allocating another 16 KB chunk.
+```
+
 ### SSL/TLS Certificate Issues
 
 #### Issue: SSL handshake failures

debian/postinst

@@ -11,6 +11,15 @@ case "$1" in
         cert_file="$cert_dir/headunit.crt"
         key_file="$cert_dir/headunit.key"
 
+        # Ensure dedicated runtime group exists.
+        if ! getent group aasdk >/dev/null 2>&1; then
+            if command -v addgroup >/dev/null 2>&1; then
+                addgroup --system aasdk || true
+            elif command -v groupadd >/dev/null 2>&1; then
+                groupadd --system aasdk || true
+            fi
+        fi
+
         # Ensure target cert directory exists.
         mkdir -p "$cert_dir"
         chmod 755 "$cert_dir" || true
@@ -36,14 +45,8 @@ case "$1" in
         fi
 
         if [ -f "$key_file" ]; then
-            chown root:root "$key_file" || true
-            chmod 600 "$key_file" || true
-
-            # Optional compatibility mode for non-root runtimes in the aasdk group.
-            if [ "$cert_group" = "aasdk" ]; then
-                chown root:aasdk "$key_file" || true
-                chmod 640 "$key_file" || true
-            fi
+            chown root:"$cert_group" "$key_file" || true
+            chmod 640 "$key_file" || true
         fi
 
         # Update the dynamic linker cache

include/aasdk/Transport/DataSink.hpp

@@ -29,16 +29,30 @@ namespace aasdk::transport {
     public:
       DataSink();
 
+      /// Allocate a cChunkSize slot at the tail of the buffer for the next USB
+      /// receive transfer to write into.  If a previous fill() was never
+      /// followed by commit() (e.g. the USB transfer returned an error), the
+      /// dangling uncommitted slot is automatically rolled back first so that
+      /// stale zero-bytes never appear in the protocol stream.
       common::DataBuffer fill();
 
+      /// Finalise the most recent fill(): trim the unused tail of the slot to
+      /// the actual number of bytes transferred and clear the pending flag.
       void commit(common::Data::size_type size);
 
+      /// Discard the uncommitted slot from the most recent fill() that was
+      /// never followed by a successful commit().  Safe to call even when no
+      /// fill is pending (no-op in that case).
+      void rollback();
+
       common::Data::size_type getAvailableSize();
 
       common::Data consume(common::Data::size_type size);
 
     private:
       boost::circular_buffer<common::Data::value_type> data_;
+      /// True between a fill() call and its matching commit() or rollback().
+      bool pendingFill_{false};
       static constexpr common::Data::size_type cChunkSize = 16384;
     };
 

include/aasdk/Transport/TCPTransport.hpp

@@ -79,9 +79,10 @@ namespace aasdk::transport {
        * @param ioService Boost ASIO io_service for async socket operations
        * @param tcpEndpoint Connected TCP socket endpoint (phone or test client)
        *
-       * Note: ioService and tcpEndpoint must remain valid until TCPTransport is destroyed.
+      * The shared_ptr keeps the io_service alive as long as the TCPTransport
+      * (or any pending handler with a shared_from_this() capture) is alive.
        */
-      TCPTransport(boost::asio::io_service &ioService, tcp::ITCPEndpoint::Pointer tcpEndpoint);
+      TCPTransport(std::shared_ptr<boost::asio::io_service> ioService, tcp::ITCPEndpoint::Pointer tcpEndpoint);
 
       /**
        * @brief Stop TCP transport and close socket.

include/aasdk/Transport/Transport.hpp

@@ -30,7 +30,9 @@ namespace aasdk::transport {
 
     class Transport : public ITransport, public std::enable_shared_from_this<Transport> {
     public:
-      Transport(boost::asio::io_service &ioService);
+      // ioService is held by shared_ptr so the io_context outlives any
+      // pending handlers that carry a shared_from_this() capture.
+      explicit Transport(std::shared_ptr<boost::asio::io_service> ioService);
 
       // Deleted copy operations
       Transport(const Transport &) = delete;
@@ -58,6 +60,9 @@ namespace aasdk::transport {
 
       DataSink receivedDataSink_;
 
+      // Must be declared before strands so it is initialised first.
+      std::shared_ptr<boost::asio::io_service> ioServicePtr_;
+
       boost::asio::io_service::strand receiveStrand_;
       ReceiveQueue receiveQueue_;
 

include/aasdk/Transport/USBTransport.hpp

@@ -73,9 +73,10 @@ namespace aasdk::transport {
        * @param ioService Boost ASIO io_service for async operations
        * @param aoapDevice AOAP device handle (from IUSBHub discovery)
        *
-       * Note: ioService must outlive the USBTransport instance.
+      * The shared_ptr keeps the io_service alive as long as the USBTransport
+      * (or any pending handler with a shared_from_this() capture) is alive.
        */
-      USBTransport(boost::asio::io_service &ioService, usb::IAOAPDevice::Pointer aoapDevice);
+      USBTransport(std::shared_ptr<boost::asio::io_service> ioService, usb::IAOAPDevice::Pointer aoapDevice);
 
       /**
        * @brief Stop USB transport and reject pending operations.
@@ -98,14 +99,35 @@ namespace aasdk::transport {
       /// Internal: handle USB OUT transfer completion; check for errors, resolve promise
       void sendHandler(SendQueue::iterator queueElement, common::Data::size_type offset, size_t bytesTransferred);
 
+      /// Internal: re-queue a receive after a short recovery delay (strand-safe)
+      void scheduleReceiveRetry(uint32_t delayMs);
+
       /// AOAP device handle (provides bulk IN/OUT endpoint access)
       usb::IAOAPDevice::Pointer aoapDevice_;
 
+      /// How many times we have retried a LIBUSB_ERROR_NO_DEVICE on the IN endpoint
+      uint32_t receiveNoDeviceRetryCount_{0};
+
+      /// How many times we have retried a LIBUSB_ERROR_INTERRUPTED on the IN endpoint
+      uint32_t receiveInterruptedRetryCount_{0};
+
       /// Timeout for send operations (10 seconds)
       static constexpr uint32_t cSendTimeoutMs = 10000;
 
       /// Timeout for receive operations (infinite: 0)
       static constexpr uint32_t cReceiveTimeoutMs = 0;
+
+      /// Max bounded retries for a post-handshake no-device transient error
+      static constexpr uint32_t cReceiveNoDeviceRetryMax = 2;
+
+      /// Delay (ms) between no-device IN endpoint retries
+      static constexpr uint32_t cReceiveNoDeviceRetryDelayMs = 200;
+
+      /// Max bounded retries for LIBUSB_ERROR_INTERRUPTED (-4); resets on success
+      static constexpr uint32_t cReceiveInterruptedRetryMax = 60;
+
+      /// Delay (ms) between interrupted IN endpoint retries (short — just reschedule)
+      static constexpr uint32_t cReceiveInterruptedRetryDelayMs = 10;
     };
 
 }

include/aasdk/USB/AOAPDevice.hpp

@@ -30,7 +30,8 @@ namespace aasdk::usb {
     class AOAPDevice : public IAOAPDevice {
     public:
       AOAPDevice(IUSBWrapper &usbWrapper, boost::asio::io_service &ioService, DeviceHandle handle,
-                 const libusb_interface_descriptor *interfaceDescriptor);
+                 uint8_t interfaceNumber, unsigned char inEndpointAddress,
+                 unsigned char outEndpointAddress);
 
       ~AOAPDevice() override;
 
@@ -52,9 +53,12 @@ namespace aasdk::usb {
 
       static const libusb_interface_descriptor *getInterfaceDescriptor(const libusb_interface *interface);
 
+      static std::tuple<uint8_t, unsigned char, unsigned char>
+      extractEndpointDetails(const libusb_interface_descriptor *interfaceDescriptor);
+
       IUSBWrapper &usbWrapper_;
       DeviceHandle handle_;
-      const libusb_interface_descriptor *interfaceDescriptor_;
+      uint8_t interfaceNumber_;
       IUSBEndpoint::Pointer inEndpoint_;
       IUSBEndpoint::Pointer outEndpoint_;
 

src/Transport/DataSink.cpp

@@ -25,10 +25,24 @@ namespace aasdk {
   namespace transport {
 
     DataSink::DataSink()
-        : data_(common::cStaticDataSize) {
+        : data_(common::cStaticDataSize), pendingFill_(false) {
     }
 
     common::DataBuffer DataSink::fill() {
+      if (pendingFill_) {
+        // Safety net: a previous fill() never received a matching commit() — most
+        // likely a USB transfer retry path where the transfer returned an error
+        // before writing any data (e.g. EINTR, LIBUSB_ERROR_NO_DEVICE).  Roll back
+        // that dangling allocation now so it can never be mistaken for real protocol
+        // data by distributeReceivedData().  Callers on the retry path should also
+        // call rollback() explicitly before fill() to document intent, but this
+        // auto-rollback acts as a hard safety net for any path that misses that.
+        if (data_.size() >= cChunkSize) {
+          data_.erase_end(cChunkSize);
+        }
+      }
+      pendingFill_ = true;
+
       const auto offset = data_.size();
       data_.resize(data_.size() + cChunkSize);
 
@@ -41,9 +55,22 @@ namespace aasdk {
         throw error::Error(error::ErrorCode::DATA_SINK_COMMIT_OVERFLOW);
       }
 
+      pendingFill_ = false;
       data_.erase_end((cChunkSize - size));
     }
 
+    void DataSink::rollback() {
+      if (!pendingFill_) {
+        return;
+      }
+      // Only erase if there's actually a pending chunk to roll back.
+      // Safety check: circular_buffer::erase_end(n) requires size() >= n.
+      if (data_.size() >= cChunkSize) {
+        data_.erase_end(cChunkSize);
+      }
+      pendingFill_ = false;
+    }
+
     common::Data::size_type DataSink::getAvailableSize() {
       return data_.size();
     }

src/Transport/SSLWrapper.cpp

@@ -20,6 +20,7 @@
 #include <string>
 #include <cerrno>
 #include <cstring>
+#include <mutex>
 #if defined(__has_include) && __has_include(<openssl/engine.h>) && !defined(OPENSSL_NO_ENGINE)
 #include <openssl/engine.h>
 #define HAVE_ENGINE_H
@@ -34,6 +35,10 @@
 namespace aasdk {
   namespace transport {
 
+    namespace {
+      std::once_flag gOpenSslInitOnce;
+    }
+
     static auto sslErrorToString(int sslErrorCode) -> const char* {
       switch (sslErrorCode) {
         case SSL_ERROR_NONE:
@@ -64,25 +69,20 @@ namespace aasdk {
     }
 
     SSLWrapper::SSLWrapper() {
-      SSL_library_init();
-      SSL_load_error_strings(); // Optional: Can also be removed if not needed.
-      OpenSSL_add_all_algorithms();
+      std::call_once(gOpenSslInitOnce, []() {
+    #if (OPENSSL_VERSION_NUMBER < 0x10100000L)
+        SSL_library_init();
+        SSL_load_error_strings();
+        OpenSSL_add_all_algorithms();
+    #else
+        OPENSSL_init_ssl(OPENSSL_INIT_LOAD_SSL_STRINGS | OPENSSL_INIT_LOAD_CRYPTO_STRINGS,
+             nullptr);
+    #endif
+      });
     }
 
     SSLWrapper::~SSLWrapper() {
-#ifdef FIPS_mode_set
-      FIPS_mode_set(0); // FIPS_mode_set removed in later versions of OpenSSL.
-#endif
-#ifdef HAVE_ENGINE_H
-      ENGINE_cleanup();
-#endif
-      CONF_modules_unload(1);
-      EVP_cleanup();
-      CRYPTO_cleanup_all_ex_data();
-#if (OPENSSL_VERSION_NUMBER < 0x10100000L)
-      ERR_remove_state(0);
-#endif
-      ERR_free_strings();
+      // OpenSSL state is process-global. Do not tear it down per wrapper instance.
     }
 
     X509 *SSLWrapper::readCertificate(const std::string &certificate) {

src/Transport/TCPTransport.cpp

@@ -22,10 +22,8 @@
 namespace aasdk {
   namespace transport {
 
-    TCPTransport::TCPTransport(boost::asio::io_service &ioService, tcp::ITCPEndpoint::Pointer tcpEndpoint)
-        : Transport(ioService), tcpEndpoint_(std::move(tcpEndpoint)) {
-
-    }
+    TCPTransport::TCPTransport(std::shared_ptr<boost::asio::io_service> ioService, tcp::ITCPEndpoint::Pointer tcpEndpoint)
+        : Transport(std::move(ioService)), tcpEndpoint_(std::move(tcpEndpoint)) {}
 
     void TCPTransport::enqueueReceive(common::DataBuffer buffer) {
       auto receivePromise = tcp::ITCPEndpoint::Promise::defer(receiveStrand_);