usbip: drain controller callbacks and centralize ret_submit validation

P1: The IOUSBHostControllerInterface command/doorbell blocks ran on an
Apple-owned dispatch queue, so callbacks could still fire (and panic in
cgo.Handle.Value) after Close deleted the handle. The controller now owns
a serial dispatch queue and drains it via dispatch_sync before tearing
the wrapper down — the watcher's pattern, extracted into a shared
box_usbhost_drain_and_release_queue helper.

P2: scatterResponse only bounds-checked ActualLength on IN transfers, so
an OUT RET_SUBMIT with a negative or oversized actual_length flowed
straight to box_usbhost_endpoint_sm_complete as a wrapped C.size_t.
Validation moves into pendingTransfer.accept, which reconciles every
response against the request before the length leaves the package.

Also wraps cgo.Handle in cgoCallbackHandle so destroy-then-Delete
ordering is encoded structurally for both the watcher and the controller.

CONTEXT.md

@@ -1,89 +0,0 @@
-# sing-box-usbip domain glossary
-
-This file names the load-bearing concepts in the USBIP subsystem so design
-discussions, ADRs, and architecture reviews share vocabulary. Concepts here
-should be reused verbatim in code identifiers and comments.
-
-## USBIP subsystem
-
-**USBIP** — a wire protocol that exports a USB device over TCP. sing-box
-implements both server (export) and client (import) roles. Linux uses
-kernel `usbip-host` / `vhci_hcd`; Darwin uses an IOUSBHost capture.
-
-**Bus ID** (`busid`) — string identifying a USB device location on the host
-(e.g. `1-1.4`). The unit of admission control: the ledger reserves a busid,
-not a device handle.
-
-**Export** — a host-owned handle to a single locally attached device that
-the server publishes to control subscribers and hands to an import
-session. The `Export` interface (`service/usbip/host.go`) is the seam
-between platform-specific host code and the platform-neutral ledger.
-Once published into a reconcile snapshot, an Export pointer is treated
-as immutable (see ADR-0001).
-
-**Export Host** (`ExportHost`) — the platform implementation that
-discovers candidate devices, owns the OS-level capture (sysfs bind on
-Linux, IOKit open on Darwin), and produces the Export map via
-`Reconcile`. One `ExportHost` per server.
-
-**Import Host** (`ImportHost`) — the symmetric platform implementation
-on the client side; takes a wire conn and attaches it to the local
-USB stack (Linux: `vhci_hcd` attach; Darwin: stub).
-
-**Export Ledger** (`exportLedger` in `service/usbip/export_ledger.go`) —
-the per-server admission, lease, and broadcast authority. Owns three
-pieces of state under one mutex (the "inventory" lock):
-
-- `exports` — published Export pointers, keyed by busid.
-- `busy` — busids with an active import session.
-- `leases` — short-lived holds that block admission until the holder
-  consumes them or the TTL expires.
-
-Plus broadcast bookkeeping under a separate "fast" lock (sequence,
-subscribers, last-broadcast state).
-
-**Reserved State** — the union of busy and unexpired-lease entries for
-a given busid. `reservedLocked(busid)` returns this. Every change to
-reserved state MUST broadcast a control-frame delta to subscribers —
-the `mutateAndBroadcast` / `withInventoryWrite` accessor enforces this
-structurally.
-
-**Lease** — a control-channel reservation (`LEASE_REQ` → `IMPORT_EXT`)
-that pins a device for one extended client. Identified by `(lease ID,
-client nonce)`; pinned to the export's `LeaseIdentity` (registry ID on
-Darwin, sysfs identity on Linux) so reconcile-driven swaps invalidate
-stale leases.
-
-**Reconcile** — the host operation that diffs the desired set of
-matched devices against the currently exported set and produces a
-plan (`toAdd`, `toRemove`, `toStale`). Stale entries remain in the
-published snapshot (with `state: unavailable`) so subscribers see a
-transition instead of a silent removal.
-
-**Stale Export** — an Export the host wanted to drop while it was still
-reserved. Stays owned by the host until the holding import session
-ends; surfaces to subscribers as `state: unavailable`. Linux marks the
-clone; Darwin marks the clone (after ADR-0001).
-
-**Control Subscriber** — a client connected to the control channel
-receiving `controlFrameDeviceSnapshot` and `controlFrameDeviceDelta`
-frames. "Extended" subscribers (`supportsControlExtensions`) get
-deltas; legacy subscribers get `controlFrameChanged` and re-fetch.
-
-## Platform conventions
-
-**Linux Export** (`linuxExport`) — wraps a sysfs `usbip-host` binding.
-Identity is the cached descriptor + identity slice; published pointers
-are immutable, cloned via `cloneLinuxExport` before stale-marking.
-
-**Darwin Export** (`darwinExport`) — wraps an IOUSBHost capture
-(`darwinUSBHostDevice`). Identity is the IOKit `registryID`; published
-pointers are immutable, cloned via `cloneDarwinExport` before
-stale-marking.
-
-## Decision records
-
-Architecture decisions live in `docs/adr/`. Active records:
-
-- ADR-0001 — Export pointer immutability (the rule that lets the ledger
-  read `Snapshot`/`LeaseCheck` outside any lock).

service/usbip/endpoint_darwin.go

@@ -8,10 +8,16 @@ import (
 	"unsafe"
 
 	"github.com/sagernet/sing-box/log"
+	E "github.com/sagernet/sing/common/exceptions"
 
 	"golang.org/x/sys/unix"
 )
 
+var (
+	errResponseNegativeActualLength = E.New("RET_SUBMIT actual_length is negative")
+	errResponseOverflow             = E.New("RET_SUBMIT actual_length exceeds request length")
+)
+
 type darwinEndpoint struct {
 	ctx    context.Context
 	cancel context.CancelFunc
@@ -184,7 +190,12 @@ func (e *darwinEndpoint) finalizePending(pending *pendingTransfer) {
 		status = -int32(unix.EIO)
 		length = 0
 	default:
-		status, length = e.scatterResponse(pending, response)
+		var acceptErr error
+		status, length, acceptErr = pending.accept(response)
+		if acceptErr != nil {
+			e.logger.Debug("RET_SUBMIT validation: ", acceptErr, " (request length ", pending.requestLen, ")")
+			e.cancel()
+		}
 	}
 	if pending.noResponse {
 		return
@@ -196,31 +207,36 @@ func (e *darwinEndpoint) finalizePending(pending *pendingTransfer) {
 	}
 }
 
-func (e *darwinEndpoint) scatterResponse(pending *pendingTransfer, response SubmitResponse) (int32, int) {
-	if pending.direction != USBIPDirIn {
-		return response.Status, int(response.ActualLength)
-	}
+// accept validates a RET_SUBMIT against the original request and, for IN
+// transfers, scatters the payload into the Apple-owned buffer. The returned
+// length is guaranteed to be in [0, p.requestLen] regardless of direction.
+// A non-nil err signals a protocol violation; the caller is expected to
+// cancel the endpoint so no further wire-corrupt completions are delivered
+// to IOUSBHost.
+func (p *pendingTransfer) accept(response SubmitResponse) (int32, int, error) {
 	if response.ActualLength < 0 {
-		e.logger.Debug("RET_SUBMIT actual_length is negative: ", response.ActualLength)
-		e.cancel()
-		return -int32(unix.EPROTO), 0
+		return -int32(unix.EPROTO), 0, errResponseNegativeActualLength
 	}
 	actualLength := int(response.ActualLength)
-	if actualLength > pending.requestLen || len(response.Buffer) > pending.requestLen {
-		e.logger.Debug("RET_SUBMIT actual_length ", actualLength, " exceeds request length ", pending.requestLen)
-		e.cancel()
-		return -int32(unix.EOVERFLOW), 0
+	if actualLength > p.requestLen {
+		return -int32(unix.EOVERFLOW), 0, errResponseOverflow
+	}
+	if p.direction != USBIPDirIn {
+		return response.Status, actualLength, nil
+	}
+	if len(response.Buffer) > p.requestLen {
+		return -int32(unix.EOVERFLOW), 0, errResponseOverflow
 	}
 	copyLength := min(actualLength, len(response.Buffer))
-	if copyLength > 0 && pending.bufferPtr != nil {
+	if copyLength > 0 && p.bufferPtr != nil {
 		if len(response.IsoPackets) > 0 {
-			dst := unsafe.Slice((*byte)(pending.bufferPtr), pending.requestLen)
+			dst := unsafe.Slice((*byte)(p.bufferPtr), p.requestLen)
 			ScatterIsoResponse(dst, response.Buffer[:copyLength], response.IsoPackets)
 		} else {
-			copy(unsafe.Slice((*byte)(pending.bufferPtr), copyLength), response.Buffer[:copyLength])
+			copy(unsafe.Slice((*byte)(p.bufferPtr), copyLength), response.Buffer[:copyLength])
 		}
 	}
-	return response.Status, actualLength
+	return response.Status, actualLength, nil
 }
 
 func (e *darwinEndpoint) startTransfer(transfer darwinCITransfer, noResponse bool) *pendingTransfer {

service/usbip/usbhost_darwin.go

@@ -75,9 +75,39 @@ type darwinCITransfer struct {
 	message darwinCIMessage
 }
 
+// cgoCallbackHandle pairs a cgo.Handle whose value is invoked asynchronously
+// from C with the ordering rule that the handle MUST NOT be deleted until the
+// C-side producer has been destroyed AND any in-flight callbacks have been
+// drained. closeAfter encodes that ordering: the destroyC callback runs first,
+// and only on its return is the handle deleted. destroyC MUST NOT return
+// until the C-side serial dispatch queue has been synchronously drained (see
+// box_usbhost_drain_and_release_queue in usbhost_darwin.m).
+type cgoCallbackHandle struct {
+	handle cgo.Handle
+}
+
+func newCgoCallbackHandle(value any) cgoCallbackHandle {
+	return cgoCallbackHandle{handle: cgo.NewHandle(value)}
+}
+
+func (c cgoCallbackHandle) token() C.uintptr_t {
+	return C.uintptr_t(c.handle)
+}
+
+// deleteRaw releases the handle without waiting for any C-side drain. Use only
+// to roll back a failed C-side create; otherwise prefer closeAfter.
+func (c cgoCallbackHandle) deleteRaw() {
+	c.handle.Delete()
+}
+
+func (c cgoCallbackHandle) closeAfter(destroyC func()) {
+	destroyC()
+	c.handle.Delete()
+}
+
 type darwinUSBHostController struct {
-	handle *C.box_usbhost_controller_t
-	ref    cgo.Handle
+	handle   *C.box_usbhost_controller_t
+	callback cgoCallbackHandle
 }
 
 type darwinUSBHostDeviceSM struct {
@@ -131,48 +161,50 @@ func darwinOpenUSBHostDevice(registryID uint64, capture bool) (*darwinUSBHostDev
 }
 
 type darwinUSBHostDeviceWatcher struct {
-	handle *C.box_usbhost_device_watcher_t
-	ref    cgo.Handle
+	handle   *C.box_usbhost_device_watcher_t
+	callback cgoCallbackHandle
 }
 
 func darwinWatchUSBHostDevices(callback func()) (*darwinUSBHostDeviceWatcher, error) {
-	ref := cgo.NewHandle(callback)
+	ref := newCgoCallbackHandle(callback)
 	var errorPtr *C.char
-	handle := C.box_usbhost_device_watcher_create(C.uintptr_t(ref), &errorPtr)
+	handle := C.box_usbhost_device_watcher_create(ref.token(), &errorPtr)
 	if handle == nil {
-		ref.Delete()
+		ref.deleteRaw()
 		return nil, darwinCError(errorPtr)
 	}
-	return &darwinUSBHostDeviceWatcher{handle: handle, ref: ref}, nil
+	return &darwinUSBHostDeviceWatcher{handle: handle, callback: ref}, nil
 }
 
 func (w *darwinUSBHostDeviceWatcher) Close() {
 	if w == nil || w.handle == nil {
 		return
 	}
-	C.box_usbhost_device_watcher_destroy(w.handle)
-	w.handle = nil
-	w.ref.Delete()
+	w.callback.closeAfter(func() {
+		C.box_usbhost_device_watcher_destroy(w.handle)
+		w.handle = nil
+	})
 }
 
 func darwinCreateUSBHostController(controller *darwinVirtualController, portCount uint8, speed uint32) (*darwinUSBHostController, error) {
-	ref := cgo.NewHandle(controller)
+	ref := newCgoCallbackHandle(controller)
 	var errorPtr *C.char
-	handle := C.box_usbhost_controller_create(C.uintptr_t(ref), C.uint8_t(portCount), C.uint32_t(speed), &errorPtr)
+	handle := C.box_usbhost_controller_create(ref.token(), C.uint8_t(portCount), C.uint32_t(speed), &errorPtr)
 	if handle == nil {
-		ref.Delete()
+		ref.deleteRaw()
 		return nil, darwinCError(errorPtr)
 	}
-	return &darwinUSBHostController{handle: handle, ref: ref}, nil
+	return &darwinUSBHostController{handle: handle, callback: ref}, nil
 }
 
 func (c *darwinUSBHostController) Close() {
 	if c == nil || c.handle == nil {
 		return
 	}
-	C.box_usbhost_controller_destroy(c.handle)
-	c.handle = nil
-	c.ref.Delete()
+	c.callback.closeAfter(func() {
+		C.box_usbhost_controller_destroy(c.handle)
+		c.handle = nil
+	})
 }
 
 func (c *darwinUSBHostController) respond(message darwinCIMessage, status int) error {

service/usbip/usbhost_darwin.m

@@ -55,6 +55,7 @@ struct box_usbhost_device_watcher {
 
 struct box_usbhost_controller {
 	void *object;
+	void *queue;
 };
 
 struct box_usbhost_device_sm {
@@ -102,6 +103,24 @@ void box_usbhost_free_error(char *error) {
 	free(error);
 }
 
+// box_usbhost_drain_and_release_queue empties any blocks already enqueued or
+// executing on the serial dispatch queue referenced by *queue_slot and releases
+// the CFBridgingRetain reference. The caller MUST first stop the producer of
+// new callbacks (IONotificationPortDestroy, [IOUSBHostControllerInterface
+// destroy], ...) so the drain converges; otherwise new blocks could keep
+// arriving. After this returns, the Go-side cgo.Handle paired with the
+// callbacks is safe to Delete.
+static void box_usbhost_drain_and_release_queue(void **queue_slot) {
+	if (queue_slot == NULL || *queue_slot == NULL) {
+		return;
+	}
+	dispatch_queue_t queue = (__bridge dispatch_queue_t)*queue_slot;
+	dispatch_sync(queue, ^{
+	});
+	CFRelease(*queue_slot);
+	*queue_slot = NULL;
+}
+
 static CFMutableDictionaryRef box_usbhost_device_matching_dictionary(void) {
 	return [IOUSBHostDevice createMatchingDictionaryWithVendorID:nil
 	                                                  productID:nil
@@ -537,15 +556,7 @@ void box_usbhost_device_watcher_destroy(box_usbhost_device_watcher_t *watcher) {
 		IONotificationPortDestroy(watcher->port);
 		watcher->port = NULL;
 	}
-	if (watcher->queue != NULL) {
-		// Drain pending IOKit notification callbacks before releasing the
-		// cgo.Handle: the serial queue guarantees any block already
-		// enqueued (or executing) finishes before this empty block runs.
-		dispatch_queue_t queue = (__bridge dispatch_queue_t)watcher->queue;
-		dispatch_sync(queue, ^{});
-		CFRelease(watcher->queue);
-		watcher->queue = NULL;
-	}
+	box_usbhost_drain_and_release_queue(&watcher->queue);
 	free(watcher);
 }
 
@@ -794,9 +805,10 @@ box_usbhost_controller_t *box_usbhost_controller_create(uintptr_t ref, uint8_t p
 				box_usbip_darwin_controller_doorbell(ref, doorbells[i]);
 			}
 		};
+		dispatch_queue_t queue = dispatch_queue_create("io.nekohasekai.sing-box.usbhost-controller", DISPATCH_QUEUE_SERIAL);
 		NSError *error = nil;
 		IOUSBHostControllerInterface *controller = [[IOUSBHostControllerInterface alloc] initWithCapabilities:capabilities
-		                                                                                                queue:nil
+		                                                                                                queue:queue
 		                                                                                      interruptRateHz:0
 		                                                                                                error:&error
 		                                                                                       commandHandler:command_handler
@@ -811,15 +823,27 @@ box_usbhost_controller_t *box_usbhost_controller_create(uintptr_t ref, uint8_t p
 		box.ref = ref;
 		box_usbhost_controller_t *handle = calloc(1, sizeof(*handle));
 		handle->object = (void *)CFBridgingRetain(box);
+		handle->queue = (void *)CFBridgingRetain(queue);
 		return handle;
 	}
 }
 
 void box_usbhost_controller_destroy(box_usbhost_controller_t *controller) {
+	if (controller == NULL) {
+		return;
+	}
 	BoxUSBHostController *box = box_controller(controller);
 	if (box != nil) {
 		[box.controller destroy];
+	}
+	// Drain the serial command/doorbell queue before releasing the
+	// BoxUSBHostController wrapper (and, on the Go side, before deleting the
+	// cgo.Handle paired with the callback ref). This mirrors the watcher's
+	// teardown.
+	box_usbhost_drain_and_release_queue(&controller->queue);
+	if (controller->object != NULL) {
 		CFBridgingRelease(controller->object);
+		controller->object = NULL;
 	}
 	free(controller);
 }