rename WiFiUdpStrem.h to UDPStream.h & SignalHandler corrections

Author: pschatzmann

.gitignore

@@ -1,3 +1,4 @@
 build/
 .vscode
+.ipynb_checkpoints/
 

ArduinoCore-Linux/cores/arduino/Arduino.h

@@ -44,6 +44,7 @@
 #include "api/ArduinoAPI.h"
 #include "RemoteSerial.h"
 #include "HardwareSetup.h"
+#include "FileStream.h"
 
 using namespace arduino;
 

ArduinoCore-Linux/cores/arduino/FileStream.h

@@ -24,7 +24,7 @@
 namespace arduino {
 
 /**
- * @brief We use the FileStream class to be able to provide Serail, Serial1 and
+ * @brief We use the FileStream class to be able to provide Serial, Serial1 and
  * Serial2 outside of the Arduino environment;
  */
 class FileStream : public Stream {
@@ -48,6 +48,10 @@ class FileStream : public Stream {
     // nothing to be done
   }
 
+  virtual void end() {
+    // nothing to be done
+  }
+
   virtual void print(const char* str) {
     out << str;
     out.flush();

ArduinoCore-Linux/cores/arduino/HardwareSetupRemote.h

@@ -26,7 +26,7 @@
 #include "RemoteI2C.h"
 #include "RemoteSPI.h"
 #include "SPIWrapper.h"
-#include "WiFiUdpStream.h"
+#include "UDPStream.h"
 
 namespace arduino {
 
@@ -127,7 +127,7 @@ class HardwareSetupRemote : public I2CSource,
   HardwareSPI* getSPI() { return &spi; }
 
  protected:
-  WiFiUDPStream default_stream;
+  arduino::UDPStream default_stream;
   Stream* p_stream = nullptr;
   RemoteI2C i2c;
   RemoteSPI spi;
@@ -145,15 +145,15 @@ class HardwareSetupRemote : public I2CSource,
           int len = s->readBytes(buffer, 18);
           buffer[len] = 0;
           if (strncmp(buffer, "Arduino-Emulator", 16)==0) {
-            Logger.info("WiFiUDPStream", "device found!");
+            Logger.info("UDPStream", "device found!");
             break;
           } else {
-            Logger.info("WiFiUDPStream", "unknown command", buffer);
+            Logger.info("UDPStream", "unknown command", buffer);
           }
         }
         delay(10000);
       } catch (const std::exception& ex) {
-        Logger.error("WiFiUDPStream", ex.what());
+        Logger.error("UDPStream", ex.what());
       }
     }
   }

ArduinoCore-Linux/cores/arduino/SignalHandler.h

@@ -24,10 +24,43 @@
 #include <vector>
 #include <map>
 #include <algorithm>
+#include <mutex>
+#include <thread>
+#include <unistd.h>
 
 #undef INADDR_NONE
 
 // Generic signal handler utility
+//
+// dispatch() used to run the registered handlers (std::map lookup,
+// std::function calls) and call exit(0) directly from inside the real OS
+// signal handler. None of that is async-signal-safe: exit() in particular
+// is explicitly documented as unsafe to call from a signal handler, since
+// it runs atexit()/static-destructor cleanup - including destroying the
+// very std::map dispatch() itself was reading. In a multi-threaded process
+// (any sketch using audio/video/network libraries alongside this) a second
+// signal delivered to another thread while the first is mid-exit() re-enters
+// dispatch() and reads/frees that map concurrently - a genuine
+// heap-use-after-free, reproducible under AddressSanitizer.
+//
+// Fixed with the standard self-pipe trick: the real signal handler only
+// does the one thing that's actually async-signal-safe here - write() one
+// byte to a pipe - and a dedicated background thread, running in ordinary
+// (non-signal) context, blocks reading that pipe and does the unsafe work
+// (map lookup, invoking handlers, exit()) with no signal-safety
+// restrictions and no reentrancy risk.
+//
+// That move alone isn't enough, though: any sketch with other threads
+// still running at signal time (e.g. an audio callback thread, a decoder
+// thread) still races exit()'s normal C++ shutdown, which runs every
+// static/global object's destructor on the reaper thread while those
+// other threads may still be mid-call on the very same objects - observed
+// as "pure virtual method called" (a virtual call landing on an object
+// whose vtable was already torn down mid-destruction). The registered
+// HandlerFunc callbacks above are the sketch's actual cleanup and still
+// run first, in full; what follows uses _exit(), which ends the process
+// immediately without invoking any other static destructor or atexit
+// handler, so it can't race with them.
 class SignalHandler {
  public:
   using HandlerFunc = std::function<void(int)>;
@@ -36,21 +69,58 @@ class SignalHandler {
     auto& vec = getHandlers()[signum];
     vec.push_back(handler);
     std::signal(signum, SignalHandler::dispatch);
+    ensureReaperThread();
   }
 
  private:
   static std::map<int, std::vector<HandlerFunc>>& getHandlers() {
     static std::map<int, std::vector<HandlerFunc>> handlers;
     return handlers;
   }
+
+  // Async-signal-safe: writes one byte and returns. No map access, no
+  // std::function calls, no exit() - all of that is deferred to the
+  // reaper thread, well outside signal-handler context.
   static void dispatch(int signum) {
-    auto& handlers = getHandlers();
-    auto it = handlers.find(signum);
-    if (it != handlers.end()) {
-      for (auto& func : it->second) {
-        func(signum);
-      }
-    }
-    exit(0);
+    char sig = (char)signum;
+    ssize_t n = write(pipeWriteFd(), &sig, 1);
+    (void)n;  // nothing safe to do with a failed write() from a handler
+  }
+
+  static int& pipeWriteFd() {
+    static int fd = -1;
+    return fd;
+  }
+  static int& pipeReadFd() {
+    static int fd = -1;
+    return fd;
+  }
+
+  // Starts the reaper thread at most once, the first time any signal is
+  // registered - safe to call from registerHandler() every time.
+  static void ensureReaperThread() {
+    static std::once_flag started;
+    std::call_once(started, [] {
+      int fds[2];
+      pipe(fds);
+      pipeReadFd() = fds[0];
+      pipeWriteFd() = fds[1];
+      std::thread([] {
+        char sig;
+        while (read(pipeReadFd(), &sig, 1) == 1) {
+          int signum = (int)(unsigned char)sig;
+          auto& handlers = getHandlers();
+          auto it = handlers.find(signum);
+          if (it != handlers.end()) {
+            for (auto& func : it->second) {
+              func(signum);
+            }
+          }
+          // not exit(): see the class comment - avoids racing other still
+          // -running threads against this thread's C++ static destructors.
+          _exit(0);
+        }
+      }).detach();
+    });
   }
 };

ArduinoCore-Linux/cores/arduino/UDPStream.h

@@ -0,0 +1,152 @@
+#pragma once
+#include <WiFi.h>
+#include <WiFiUdp.h>
+#if defined(ESP32)
+#include <esp_wifi.h>
+#endif
+#include "ArduinoLogger.h"
+
+namespace arduino {
+
+/**
+ * @brief A UDP class which makes sure that we can use UDP as
+ * simple Stream w/o additional complexity.
+ * When we did not specify a target IP address we send the data
+ * to the address we received the data.
+ * Single character writes will be buffered up to the defined write
+ * buffer size (default 256 bytes)
+ * @author Phil Schatzmann
+ * @copyright GPLv3
+ */
+
+class UDPStream : public Stream {
+ public:
+  /// Default Constructor
+  UDPStream() = default;
+
+  /// Short hand constructor to start sending data to the indicated address /
+  /// port
+  UDPStream(IPAddress a, uint16_t port) { begin(a, port); }
+
+  /// @brief Defines an alternative UDP object. By default we use WiFiUDP
+  /// @param udp
+  void setUDP(UDP& udp) { p_udp = &udp; };
+
+  /// Always return 1492 (MTU 1500 - 8 byte header) as UDP packet available to
+  /// write
+  int availableForWrite() override { return 1492; }
+
+  /**
+   * Provides the available size of the current package and if this is used up
+   * of the next package
+   */
+  int available() override {
+    int size = p_udp->available();
+    // if the curren package is used up we prvide the info for the next
+    if (size == 0) {
+      size = p_udp->parsePacket();
+    }
+    return size;
+  }
+
+  /// Starts to send data to the indicated address / port
+  bool begin(IPAddress a, uint16_t port) {
+    remote_address_ext = a;
+    remote_port_ext = port;
+    return p_udp->begin(port);
+  }
+
+  /// Starts to receive data from/with the indicated port
+  bool begin(uint16_t port, uint16_t port_ext = 0) {
+    remote_address_ext = IPAddress((uint32_t)0);
+    remote_port_ext = port_ext != 0 ? port_ext : port;
+    return p_udp->begin(port);
+  }
+
+  /// Starts to receive data in multicast from/with the indicated address / port
+  bool beginMulticast(IPAddress address, uint16_t port) {
+    return p_udp->beginMulticast(address, port);
+  }
+
+  /// Alternative to begin(IPAddress a, uint16_t port)
+  bool setTarget(IPAddress a, uint16_t port) { return begin(a, port); }
+
+  /// We use the same remote port as defined in begin for write
+  uint16_t remotePort() {
+    uint16_t result = p_udp->remotePort();
+    return result != 0 ? result : remote_port_ext;
+  }
+
+  /// We use the same remote ip as defined in begin for write
+  IPAddress remoteIP() {
+    // Determine address if it has not been specified
+    if ((uint32_t)remote_address_ext == 0) {
+      remote_address_ext = p_udp->remoteIP();
+    }
+    // EmulatorLogger.log(EmulatorLogger::INFO, "ip: %u", remote_address_ext);
+    return remote_address_ext;
+  }
+
+  /// Replys will be sent to the initial remote caller
+  size_t write(const uint8_t* data, size_t len) override {
+    IPAddress remote = remoteIP();
+    if ((uint32_t)remote == 0) {
+      Logger.error("UDPStream", "no remote address defined");
+      return 0;
+    }
+    p_udp->beginPacket(remote, remotePort());
+    size_t result = p_udp->write(data, len);
+    p_udp->endPacket();
+    return result;
+  }
+
+  /// Reads bytes using WiFi::readBytes
+  size_t readBytes(uint8_t* data, size_t len) {
+    size_t avail = available();
+    size_t bytes_read = 0;
+    if (avail > 0) {
+      // get the data now
+      bytes_read = p_udp->readBytes((uint8_t*)data, len);
+    }
+    return bytes_read;
+  }
+
+  void stop() { p_udp->stop(); }
+
+  int peek() override { return p_udp->peek(); }
+
+  int read() override { return p_udp->read(); }
+
+  size_t write(uint8_t data) override {
+    if (write_buffer.capacity() < write_buffer_size) {
+      write_buffer.reserve(write_buffer_size);
+    }
+    write_buffer.push_back(data);
+    if (write_buffer.size() >= write_buffer_size) {
+      flush();
+    }
+    return 1;
+  }
+
+  void flush() override {
+    if (write_buffer.empty()) {
+      return;
+    }
+    write(write_buffer.data(), write_buffer.size());
+    write_buffer.clear();
+  }
+
+  void setWriteBufferSize(size_t size) { write_buffer_size = size; }
+
+ protected:
+  WiFiUDP default_udp;
+  UDP* p_udp = &default_udp;
+  uint16_t remote_port_ext = 0;
+  IPAddress remote_address_ext;
+  int write_buffer_size = 256;
+  std::vector<uint8_t> write_buffer;
+};
+
+}  // namespace arduino
+
+using WiFiUDPStream = arduino::UDPStream;