From 4db2957a498e1ced3c1794caf34f6d7d7fbe5624 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E4=B8=96=E7=95=8C?= Date: Sat, 16 May 2026 14:34:28 +0800 Subject: [PATCH] usbip: drain controller callbacks and centralize ret_submit validation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 89 -------------------------------- service/usbip/endpoint_darwin.go | 48 +++++++++++------ service/usbip/usbhost_darwin.go | 68 +++++++++++++++++------- service/usbip/usbhost_darwin.m | 44 ++++++++++++---- 4 files changed, 116 insertions(+), 133 deletions(-) delete mode 100644 CONTEXT.md diff --git a/CONTEXT.md b/CONTEXT.md deleted file mode 100644 index 5ff4bf04d..000000000 --- a/CONTEXT.md +++ /dev/null @@ -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). diff --git a/service/usbip/endpoint_darwin.go b/service/usbip/endpoint_darwin.go index fb692d6e6..374e6517d 100644 --- a/service/usbip/endpoint_darwin.go +++ b/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 { diff --git a/service/usbip/usbhost_darwin.go b/service/usbip/usbhost_darwin.go index dbaf54ade..d33039c25 100644 --- a/service/usbip/usbhost_darwin.go +++ b/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 { diff --git a/service/usbip/usbhost_darwin.m b/service/usbip/usbhost_darwin.m index 21272180a..26d8a79bd 100644 --- a/service/usbip/usbhost_darwin.m +++ b/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); }