netJACK2: reap dead masters, fix KillMaster UAF, dedupe slaves, pin slave multicast

.vscode/settings.json

@@ -0,0 +1,12 @@
+{
+    "workbench.colorCustomizations": {
+        "titleBar.activeBackground": "#282768",
+        "titleBar.activeForeground": "#ffffff",
+        "titleBar.inactiveBackground": "#282768",
+        "titleBar.inactiveForeground": "#ffffff",
+        "activityBar.background": "#1d1c4a",
+        "activityBar.foreground": "#ffffff",
+        "statusBar.background": "#282768",
+        "statusBar.foreground": "#ffffff"
+    }
+}

ChangeLog.rst

@@ -1,7 +1,7 @@
 ChangeLog
 #########
 
-* Unreleased (sastraxi fork)
+* Unreleased (TreeFallSound fork)
 
   * netadapter: clear PI controller integrator on ringbuffer reset.
     ``JackPIControler::OurOfBounds()`` exists for this but had zero call
@@ -27,11 +27,45 @@ ChangeLog
     kernel-route-table behavior.
   * ``build-macos-pkg.sh`` — produces a ``.pkg`` installer that drops
     the fork's binaries, libs, headers, and intclient ``.so``s into
-    ``/usr/local`` on Apple Silicon (the manual-install prefix; Homebrew
+    ``/usr/local`` on Apple Silicon (the manual-instaSSll prefix; Homebrew
     is at ``/opt/homebrew``). Required because ``installer/build-pkg.sh``
     in the JackRouter repo ``check_jack``s a pre-installed jack2 and
     refuses to build without one — without this, fresh JackRouter users
     on Apple Silicon have no way to get the multicast-pin code.
+  * netJACK2 (master): recover from a slave that disappears without
+    sending ``KILL_MASTER`` (yanked cable, hard-killed slave). A master
+    whose RT link hits a fatal recv/send error marks itself dead
+    (``fDead``) and the manager reaps it on its next listen pass
+    (``ReapDeadMasters``) instead of stalling the master cycle ~2 s per
+    period indefinitely. Replaces the "ugly temporary fix" ``ThreadExit``
+    that had been in ``JackNetMasterInterface::FatalRecvError`` since 2008.
+  * netJACK2 (master): fix a use-after-free in ``KillMaster``. The old
+    ``erase(it); delete *it;`` freed through an iterator that ``erase``
+    had already invalidated. This path runs on the *normal* multicast
+    ``KILL_MASTER`` clean-stop, not only on edge cases.
+  * netJACK2 (master): dedupe masters by slave name in ``InitMaster``.
+    A slave that restarts quickly keeps feeding its old master packets,
+    so that master never times out or self-declares dead, and
+    ``FindMaster`` only matches ``fID`` which a re-announcing slave
+    lacks. Without the dedupe each re-announcement spawns another JACK
+    client (``pistomp-01``, ``-02``, ...) all fighting for the same ports.
+  * netJACK2: initialize ``fDead`` in the ``session_params_t``
+    constructor as well, so a master created through that path cannot
+    start life reading an indeterminate dead flag.
+  * netJACK2: fail closed when a requested multicast interface pin cannot
+    be applied. ``NewSocket()`` refuses the socket rather than letting
+    the slave fall back to the default route; the reconnect loop retries.
+  * netJACK2: pin *unicast* egress to an interface, not only the
+    multicast join. On Linux ``JACK_NETJACK_MULTICAST_IF`` set only
+    ``IP_MULTICAST_IF``, so the post-discovery unicast RT stream still
+    followed the default route and could leave over wifi when both the
+    wired and the wifi interface carried a ``169.254`` link-local
+    address. The slave now also applies ``IP_UNICAST_IF``; the master
+    captures the arrival interface of each ``SLAVE_AVAILABLE`` via
+    ``IP_PKTINFO``, latches the first one seen, and pins every spawned
+    master socket to it (``IP_BOUND_IF`` on macOS/BSD, ``IP_UNICAST_IF``
+    on Linux). Env-gated and fail-closed; the latch re-resolves if the
+    pinned interface disappears.
 
 * 1.9.22 (2023-02-02)
 

NETJACK-REAPING.md

@@ -0,0 +1,132 @@
+# The netmanager master is not reaped when the slave goes away
+
+## The symptom
+
+A netJACK2 slave stops. The cause can be a clean stop, a hard kill, or a
+yanked cable. The master side does not recover.
+
+The dead master client stays registered in the server. Its ports stay
+connected. Every audio cycle then waits for a packet that never comes. The
+wait is `PACKET_TIMEOUT * NETWORK_DEFAULT_LATENCY` = 2 seconds. The audio
+budget at 48 kHz / 128 frames is 2.67 milliseconds. The failure is permanent
+and silent.
+
+If the slave restarts, a second master is created. The graph fills with
+`pistomp-01`, `pistomp-02`, and more. Each one fights for the same ports.
+
+## The cause
+
+There are four defects. Each one is on its own path.
+
+### 1. `FatalRecvError` kills the RT thread
+
+`JackNetMasterInterface::FatalRecvError` and `FatalSendError` call
+`ThreadExit()`. That code runs as the JACK process callback, on the real-time
+graph thread (`JackNetMaster::Process`). `ThreadExit()` ends that thread.
+
+The client is still registered. Nothing services it. The server still calls
+its cycle, and the cycle waits out the full `PACKET_TIMEOUT` every time. A
+comment in the source calls this "an UGLY temporary way".
+
+### 2. `KillMaster` is a use-after-free
+
+`JackNetMasterManager::KillMaster` does this:
+
+    fMasterList.erase(master_it);
+    delete (*master_it);
+
+`erase()` makes the iterator not valid. The next line reads it. The `delete`
+then acts on whatever that read produced. So the multicast `KILL_MASTER`
+path — the one clean stop uses — does not reap the master either. It corrupts
+the heap.
+
+### 3. `InitMaster` does not check for an existing slave
+
+`JackNetMasterManager::InitMaster` creates a new `JackNetMaster` for every
+`SLAVE_AVAILABLE` packet. It does not look for a master that already holds
+that slave name. A slave that restarts, or a fast Ethernet Audio toggle on
+the pedal, sends a fresh `SLAVE_AVAILABLE` while the old master is still in
+the list. A duplicate is the result.
+
+The old master often cannot detect this on its own. The restarted slave
+sends to the same multicast group and port, so the old master keeps
+receiving packets and never times out. `FindMaster` only matches `fID`, and
+a re-announcing slave does not carry the old `fID`. So nothing removes the
+stale master.
+
+### 4. The slave does not pin its multicast interface
+
+`JackNetAdapter` calls `fSocket.SetMulticastIF()` before `NewSocket()`. The
+socket does not exist yet, so the `setsockopt` acts on file descriptor 0.
+`NewSocket()` never re-applies the option. The slave's reconnect loop builds
+a new socket on every attempt, and none of them is pinned.
+
+A leaked netadapter then sends its `SLAVE_AVAILABLE` announcements out the
+default route. On a host with the cable on one interface and Wi-Fi on
+another, teardown deletes the cable's route, and the announcements go out
+**over Wi-Fi**.
+
+## The correction
+
+### 1. Reap off the RT thread
+
+`FatalRecvError` and `FatalSendError` set an atomic flag `fDead` and return.
+They do not call `ThreadExit()`. `Exit()` still runs, so `fRunning` becomes
+false and the multicast euthanasia request is still sent.
+
+`JackNetMasterManager::Run` calls `ReapDeadMasters()` at the top of its loop.
+That function walks the master list and destroys every master whose
+`IsDead()` is true. The loop wakes at least every `MANAGER_INIT_TIMEOUT`
+(2 seconds), so a silent network is still handled.
+
+### 2. Capture the pointer before the erase
+
+A new function `RemoveMaster(master_list_it_t)` reads the pointer, then
+erases, then deletes:
+
+    JackNetMaster* master = *master_it;
+    fMasterList.erase(master_it);
+    delete master;
+
+`KillMaster`, `ReapDeadMasters`, and the dedupe path all use it.
+
+### 3. Dedupe by slave name
+
+`InitMaster` walks the master list first. It reaps every master whose
+`fParams.fName` equals the new slave's name. Then it creates the new master.
+
+`ReapDeadMasters()` runs at the top of the same `Run()` pass, so a
+self-declared-dead master is already gone. A name match in `InitMaster` is
+therefore always a live master, deliberately superseded.
+
+The `sleep 3` in the pi-Stomp `jackbridge-pi-up` helper was a workaround for
+this defect. It can be removed once this correction is in the field.
+
+### 4. Re-apply the multicast interface on every socket
+
+`JackNetUnixSocket` stores the interface name in `fMcastIF`. `SetMulticastIF`
+records the name and applies it only if the socket already exists.
+`NewSocket()` re-applies it, through a new private `ApplyMulticastIF()`.
+
+The re-apply is fail closed. If a pin was asked for and cannot be set,
+`NewSocket()` closes the socket and returns `SOCKET_ERROR`. The slave then
+retries in its reconnect loop instead of announcing on the default route. So
+the "over Wi-Fi" case is handled inside the fork, and the pi teardown does
+not need to add or remove kernel routes.
+
+## Note
+
+Defect 1 also returns `SOCKET_ERROR` from the process callback. What the
+server does with it is traced, not guessed: for an internal client like
+netmanager, `JackClient::CycleSignalAux` sees `status != 0` and calls
+`End()`, which clears `fActive` and deactivates the client — the graph
+stops scheduling it at all. That is acceptable, even helpful: `fRunning`
+is already false, any cycle that still runs returns 0 immediately, and
+the manager reaps the master within one loop pass (at most 2 s).
+
+The slave reconnect loop (`JackNetSlaveInterface::Init`) stays unbounded. A
+legitimate slave waits there for its master. Defect 3's correction is what
+makes an unbounded wait safe on the master side.
+
+The Windows socket (`JackNetWinSocket`) is not changed. It is not built for
+the pi-Stomp targets. Its `SetMulticastIF` keeps the apply-at-call behavior.

build-macos-pkg.sh

@@ -1,5 +1,5 @@
 #!/usr/bin/env bash
-# Build a macOS .pkg installer for the sastraxi/jack2 fork.
+# Build a macOS .pkg installer for the TreeFallSound/jack2 fork.
 #
 # The fork carries the pi-stomp/JackBridge patches on top of upstream
 # v1.9.22 (PI controller reset, master-side and slave-side multicast
@@ -30,14 +30,14 @@ STAGING="$BUILD/staging-pkg"
 PKG_OUT_DIR="$BUILD"
 
 # Version: explicit arg wins, else derived from the fork's commit count
-# past v1.9.22 + a short SHA. e.g. 1.9.22+sastraxi.3.gb3bfc408
+# past v1.9.22 + a short SHA. e.g. 1.9.22+treefall.3.gb3bfc408
 if [ $# -ge 1 ]; then
     VERSION="$1"
 else
     BASE="1.9.22"
     COUNT=$(git rev-list --count "v1.9.22..HEAD" 2>/dev/null || echo "0")
     SHORT=$(git rev-parse --short HEAD)
-    VERSION="${BASE}+sastraxi.${COUNT}.g${SHORT}"
+    VERSION="${BASE}+treefall.${COUNT}.g${SHORT}"
 fi
 
 echo "==> jack2 fork @ $(git rev-parse --short HEAD) ($(git log -1 --pretty=%s))"
@@ -77,24 +77,22 @@ python3 ./waf install --destdir="$STAGING"
 
 echo
 echo "==> build the .pkg"
-PKG_ID="com.sastraxi.jack2"
+PKG_ID="com.treefallsound.jack2"
 PKG_OUT="$PKG_OUT_DIR/jack2-${VERSION}.pkg"
 
-# --root: the directory whose contents become the payload (so the
-#         payload layout matches the install-location layout, /usr/local/...)
-# --install-location: the absolute path the payload is rooted at on
-#         the target. The pkg is relocatable to any prefix only if we
-#         don't hardcode paths in dylibs, but our jackd and dylibs use
-#         absolute @rpath-style install_names, so /usr/local is the
-#         only sensible install-location for this fork.
 pkgbuild \
     --root "$STAGING" \
     --identifier "$PKG_ID" \
     --version "$VERSION" \
-    --install-location /usr/local \
+    --install-location / \
     --ownership recommended \
     "$PKG_OUT"
 
+# The staging tree already contains usr/local/... because waf installs with
+# DESTDIR. The package root is therefore /; using /usr/local here would place
+# the payload at /usr/local/usr/local and leave the live JACK installation
+# untouched.
+
 echo
 echo "==> done"
 ls -la "$PKG_OUT"
@@ -105,6 +103,5 @@ echo
 echo "Install with:"
 echo "  sudo installer -pkg $PKG_OUT -target /"
 echo
-echo "Verify after install:"
-echo "  /usr/local/bin/jackd --version"
-echo "  strings /usr/local/lib/jack/netmanager.so | grep JACK_NETJACK_MULTICAST_IF"
+echo "JACK netJACK interface pinning landed in netmanager.so:"
+strings /usr/local/lib/jack/netmanager.so | grep -E 'JACK_NETJACK_MULTICAST_IF|pinning masters' || echo "  NOT FOUND — install the package at /usr/local and restart JACK"

common/JackAudioAdapterInterface.cpp

@@ -283,9 +283,11 @@ namespace Jack
                 }
             }
         }
-        // Reset all ringbuffers in case of failure
         if (failure) {
-            jack_error("JackAudioAdapterInterface::PushAndPull ringbuffer failure... reset");
+            uint64_t failure_count = FailureReportCount();
+            if (failure_count > 0) {
+                jack_error("JackAudioAdapterInterface::PushAndPull ringbuffer failure... reset; failures since last report = %llu", (unsigned long long)failure_count);
+            }
             if (fAdaptative) {
                 GrowRingBufferSize();
                 jack_info("Ringbuffer size = %d frames", fRingbufferCurSize);
@@ -297,9 +299,21 @@ namespace Jack
         }
     }
 
+    uint64_t JackAudioAdapterInterface::FailureReportCount()
+    {
+        ++fFailureCount;
+        jack_time_t now = GetMicroSeconds();
+        if (fLastFailureReport != 0 && now - fLastFailureReport < 1000000) {
+            return 0;
+        }
+        fLastFailureReport = now;
+        uint64_t count = fFailureCount;
+        fFailureCount = 0;
+        return count;
+    }
+
     int JackAudioAdapterInterface::PullAndPush(float** inputBuffer, float** outputBuffer, unsigned int frames)
     {
-        fPullAndPushTime = GetMicroSeconds();
         if (!fRunning) {
             return 0;
         }

common/JackAudioAdapterInterface.h

@@ -94,6 +94,10 @@ namespace Jack
 
         bool fRunning;
         bool fAdaptative;
+        uint64_t fFailureCount;
+        jack_time_t fLastFailureReport;
+
+        uint64_t FailureReportCount();
 
         void ResetRingBuffers();
         void AdaptRingBufferSize();
@@ -114,7 +118,9 @@ namespace Jack
                                 fRingbufferCurSize(ring_buffer_size),
                                 fPullAndPushTime(0),
                                 fRunning(false),
-                                fAdaptative(true)
+                                fAdaptative(true),
+                                fFailureCount(0),
+                                fLastFailureReport(0)
         {}
 
         JackAudioAdapterInterface(jack_nframes_t host_buffer_size,
@@ -133,7 +139,9 @@ namespace Jack
                                 fRingbufferCurSize(ring_buffer_size),
                                 fPullAndPushTime(0),
                                 fRunning(false),
-                                fAdaptative(true)
+                                fAdaptative(true),
+                                fFailureCount(0),
+                                fLastFailureReport(0)
         {}
 
         virtual ~JackAudioAdapterInterface()

common/JackClient.cpp

@@ -29,6 +29,12 @@ Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
 #include "driver_interface.h"
 #include "JackLibGlobals.h"
 
+#ifdef __APPLE__
+#include "JackWorkgroup.h"
+#include "JackPosixThread.h"
+#include <stdlib.h>
+#endif
+
 #include <math.h>
 #include <string>
 #include <algorithm>
@@ -557,6 +563,40 @@ void JackClient::SetupRealTime()
     if (fThread.AcquireSelfRealTime(GetEngineControl()->fClientPriority) < 0) {
         jack_error("JackClient::AcquireSelfRealTime error");
     }
+
+#ifdef __APPLE__
+    /*
+        Join the backend device's CoreAudio workgroup. Plain time-constraint
+        realtime is not enough on Apple Silicon: an unjoined thread can still be
+        preempted by WindowServer right before the cycle deadline, which is what
+        makes netmanager's client miss its slot under GUI load.
+
+        Order matters. os_workgroup_join returns EINVAL on a thread that Mach
+        does not consider realtime, so this must come after AcquireSelfRealTime,
+        and it must run here -- on the client's own realtime thread -- because a
+        join cannot be performed on another thread's behalf.
+
+        JACK_NO_WORKGROUP lets a client opt out. A client that already belongs
+        to some other device's workgroup (JackBridge's daemon holds its HAL
+        device's, because it publishes that device's timeline) sets this so we
+        do not fight over its membership.
+
+        Failure is never fatal: the thread simply keeps the realtime scheduling
+        it already had, which is the pre-existing behaviour.
+    */
+    if (getenv("JACK_NO_WORKGROUP") == NULL) {
+        if (JackWorkgroupJoinSelfForDevice(GetEngineControl()->fCoreAudioDeviceID) == 0) {
+            /*
+                Leave the workgroup before this thread ends. This is not
+                optional tidiness: libdispatch stops the process when a thread
+                ends while it is a member. See JackPosixThread::ThreadHandler.
+            */
+            JackSetThreadExitHook(JackWorkgroupLeaveSelf);
+        }
+    } else {
+        jack_info("JackClient::SetupRealTime : JACK_NO_WORKGROUP set, staying out of the backend workgroup");
+    }
+#endif
 }
 
 int JackClient::StartThread()

common/JackConstants.h

@@ -72,7 +72,7 @@
 
 #define ALL_CLIENTS -1 // for notification
 
-#define JACK_PROTOCOL_VERSION 9
+#define JACK_PROTOCOL_VERSION 10
 
 #define SOCKET_TIME_OUT 2               // in sec
 #define DRIVER_OPEN_TIMEOUT 5           // in sec