diff --git a/docs/adr/0001-export-pointer-immutability.md b/docs/adr/0001-export-pointer-immutability.md deleted file mode 100644 index 5e5ef0505..000000000 --- a/docs/adr/0001-export-pointer-immutability.md +++ /dev/null @@ -1,76 +0,0 @@ -# ADR-0001 — Export pointer immutability - -- Status: Accepted -- Date: 2026-05-16 -- Scope: `service/usbip/host.go`, `service/usbip/host_linux.go`, - `service/usbip/host_darwin.go`, `service/usbip/export_ledger.go` - -## Context - -The `Export` interface (`service/usbip/host.go`) is the seam between -platform `ExportHost` implementations and the platform-neutral -`exportLedger`. The ledger publishes Export pointers to control -subscribers and import sessions, and it calls `Snapshot`, -`LeaseCheck`, `LeaseIdentity`, and `DeviceInfo` on those pointers -**outside** any lock (`export_ledger.go` ~ lines 190, 274, 377, 592). -Holding the inventory lock across these calls is not viable because -they may perform syscalls (Linux re-reads `usbip_status`; Darwin -re-reads IOKit state). - -Two implementation paths existed: - -1. Each host carries a per-Export lock and `Snapshot` etc. acquire it. - Cost: every method on every Export takes a lock. The lock has to - live in the platform struct (the interface is read-only). Three - call sites in the ledger × N hosts. Easy to forget on a new host. -2. Hosts treat published Export pointers as immutable. Mutation is - "clone the published pointer, mutate the clone, swap it into the - host's committed map under the host's lock". The ledger's - unlocked reads are safe because the reader holds a pointer that - nobody mutates. - -Linux already followed pattern 2 by convention (`cloneLinuxExport` + -the `committed` map swap in `Reconcile`). Darwin did not — its -`Reconcile` set `exp.stale = true` and `exp.pendingRegistryID = …` -on the same `*darwinExport` value the ledger had already received -(`host_darwin.go:177-180`). Under `go test -race` this is a clean -data race; in production it can publish an inconsistent tuple -(`stale=true`, `pendingRegistryID=0`) if writes reorder. - -## Decision - -Pattern 2 is the contract. Published Export pointers are immutable -from the ledger's perspective. Hosts that need to change Export -state MUST clone, mutate the clone, then swap the clone into their -committed map under their own lock. The previously-handed-out pointer -is never written to. - -The contract is documented on the `Export` interface declaration -itself. A shared helper `applyStaleClones` (`host.go`) enforces the -clone-then-mark rule for the common "mark a busid stale" transition; -both Linux and Darwin reconcile flows use it. - -## Consequences - -- The ledger reads `Snapshot/LeaseCheck/…` outside its inventory lock - without per-Export locking and without races. This keeps the - inventory lock fast and avoids re-entering hosts under it. -- Hosts pay a small allocation per stale/replace transition (one - struct value copy plus a `slices.Clone` of any embedded slice). - Reconcile is not on the hot path. -- New host implementations (Windows, FreeBSD, …) inherit the rule - via the interface comment, the `applyStaleClones` helper, and this - ADR. The race detector catches violations on normal test runs - once `go test -race` is part of CI. -- The contract does not cover external state hidden behind the - Export (the IOKit handle, the kernel binding). Hosts continue to - manage that under their own locks; the rule is specifically about - fields read through the `Export` interface methods. - -## References - -- `service/usbip/host.go` — interface doc and `applyStaleClones`. -- `service/usbip/host_linux.go` — `cloneLinuxExport`, `committed` - map pattern in `Reconcile`. -- `service/usbip/host_darwin.go` — `cloneDarwinExport`, mirrored - `Reconcile` after this ADR. diff --git a/service/usbip/endpoint_darwin.go b/service/usbip/endpoint_darwin.go index 4d20c2fe9..4badb3fd5 100644 --- a/service/usbip/endpoint_darwin.go +++ b/service/usbip/endpoint_darwin.go @@ -13,11 +13,6 @@ import ( "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 @@ -208,21 +203,19 @@ func (e *darwinEndpoint) finalizePending(pending *pendingTransfer) { } // validateResponse reconciles a RET_SUBMIT against the original request shape. -// Every wire-shape rule lives here so accept can assume well-formed input and -// future defects land in one place. Returns the wire-level errno status and a -// non-nil err on protocol violation. +// All wire-shape rules live here so accept can assume well-formed input. func (p *pendingTransfer) validateResponse(response SubmitResponse) (int32, error) { if response.ActualLength < 0 { - return -int32(unix.EPROTO), errResponseNegativeActualLength + return -int32(unix.EPROTO), E.New("RET_SUBMIT actual_length is negative: ", response.ActualLength) } if int(response.ActualLength) > p.requestLen { - return -int32(unix.EOVERFLOW), errResponseOverflow + return -int32(unix.EOVERFLOW), E.New("RET_SUBMIT actual_length exceeds request length: actual_length ", response.ActualLength, ", request ", p.requestLen) } if p.direction != USBIPDirIn { return 0, nil } if len(response.Buffer) > p.requestLen { - return -int32(unix.EOVERFLOW), errResponseOverflow + return -int32(unix.EOVERFLOW), E.New("RET_SUBMIT buffer exceeds request length: buffer ", len(response.Buffer), ", request ", p.requestLen) } if len(response.IsoPackets) > 0 { err := ValidateIsoResponse(p.requestLen, int(response.ActualLength), response.IsoPackets, len(response.Buffer)) diff --git a/service/usbip/export_ledger.go b/service/usbip/export_ledger.go index 27eb8b891..7c2455dd8 100644 --- a/service/usbip/export_ledger.go +++ b/service/usbip/export_ledger.go @@ -13,34 +13,25 @@ import ( "github.com/sagernet/sing-box/log" ) -// exportLedger uses two internal mutexes that are NEVER held -// simultaneously. Each public method acquires at most one at a time; -// multi-stage methods acquire one, do unlocked work (including -// syscalls), then acquire the other. -// -// fast: broadcast bookkeeping. seq, nextSubID, subs, state. -// inventory: exports, busy, leases, nextLeaseID. Direct access is -// forbidden; route every read/write through one of -// withInventoryRead / withInventoryWrite / withInventoryWriteQuiet -// so the broadcast-on-mutation invariant cannot be skipped by -// a future caller. The field is unexported within the package -// to make any l.inventory.Lock() bypass obvious in review. +// exportLedger holds two mutexes that are never acquired together. The +// inventory lock must be released before BroadcastIfChanged re-takes it +// through snapshotDeviceState. type exportLedger struct { logger log.ContextLogger now func() time.Time ttl time.Duration - fast sync.Mutex - seq uint64 - nextSubID uint64 - subs map[uint64]*exportSubscriber - state map[string]DeviceInfoV2 + broadcastAccess sync.Mutex + seq uint64 + nextSubID uint64 + subs map[uint64]*exportSubscriber + state map[string]DeviceInfoV2 - inventory sync.Mutex - exports map[string]Export - busy map[string]bool - leases map[string]serverImportLease - nextLeaseID uint64 + inventoryAccess sync.Mutex + exports map[string]Export + busy map[string]bool + leases map[string]serverImportLease + nextLeaseID uint64 } type exportSubscriber struct { @@ -81,43 +72,28 @@ func newExportLedger(logger log.ContextLogger, ttl time.Duration, now func() tim } } -// withInventoryWrite holds the inventory lock for body, then broadcasts -// iff body reports a change. This is the only path that may both mutate -// reserved state AND fire the resulting broadcast; routing every -// mutation through here makes the "any reservedLocked-affecting change -// must broadcast" invariant structural rather than a discipline rule. -// body must NOT acquire fast (lock-ordering rule documented on -// exportLedger). +// withInventoryWrite broadcasts iff body returns true. body must not +// acquire the broadcast lock. func (l *exportLedger) withInventoryWrite(ctx context.Context, body func() bool) { - l.inventory.Lock() + l.inventoryAccess.Lock() changed := body() - l.inventory.Unlock() + l.inventoryAccess.Unlock() if changed { l.BroadcastIfChanged(ctx) } } -// withInventoryRead holds the inventory lock for body without -// broadcasting. Use for read-only sections that observe reserved state. func (l *exportLedger) withInventoryRead(body func()) { - l.inventory.Lock() - defer l.inventory.Unlock() + l.inventoryAccess.Lock() + defer l.inventoryAccess.Unlock() body() } -// withInventoryWriteQuiet holds the inventory lock for body without -// broadcasting. Use ONLY when the caller is contractually responsible -// for the broadcast at a different point in its flow: -// -// - ApplyHostSnapshot: paired with reconcileAndBroadcast in server.go. -// - TryReserveForImport stage 2: the server's import setup broadcasts -// after the full session is wired up. -// - ResetForClose: shutdown path; subscribers are about to be closed. -// -// Every other write site MUST use withInventoryWrite. +// withInventoryWriteQuiet is for mutations whose broadcast is the +// caller's responsibility (paired with BroadcastIfChanged or shutdown). func (l *exportLedger) withInventoryWriteQuiet(body func()) { - l.inventory.Lock() - defer l.inventory.Unlock() + l.inventoryAccess.Lock() + defer l.inventoryAccess.Unlock() body() } @@ -129,9 +105,7 @@ func (l *exportLedger) IsReserved(busid string) bool { return reserved } -// reservedLocked reports whether busid is unavailable for new admission: -// either marked busy by an active session or covered by an unexpired -// import lease. Caller must hold l.inventory. +// reservedLocked: caller must hold l.inventoryAccess. func (l *exportLedger) reservedLocked(busid string) bool { if l.busy[busid] { return true @@ -173,20 +147,20 @@ func (l *exportLedger) ApplyHostSnapshot(snapshot map[string]Export, released [] func (l *exportLedger) SeedBroadcastState(ctx context.Context) { nextState := deviceInfoV2Map(l.snapshotDeviceState(ctx)) - l.fast.Lock() + l.broadcastAccess.Lock() l.state = nextState - l.fast.Unlock() + l.broadcastAccess.Unlock() } func (l *exportLedger) BroadcastIfChanged(ctx context.Context) bool { nextState := deviceInfoV2Map(l.snapshotDeviceState(ctx)) - l.fast.Lock() + l.broadcastAccess.Lock() nextSequence := l.seq + 1 delta := buildControlDeviceDelta(nextSequence, l.state, nextState) if len(delta.Added) == 0 && len(delta.Updated) == 0 && len(delta.Removed) == 0 { l.state = nextState - l.fast.Unlock() + l.broadcastAccess.Unlock() return false } l.seq = nextSequence @@ -196,7 +170,7 @@ func (l *exportLedger) BroadcastIfChanged(ctx context.Context) bool { for _, sub := range l.subs { targets = append(targets, sub) } - l.fast.Unlock() + l.broadcastAccess.Unlock() frame := controlFrame{ Type: controlFrameChanged, @@ -217,13 +191,9 @@ func (l *exportLedger) BroadcastIfChanged(ctx context.Context) bool { return true } -// TryReserveForImport runs Export.LeaseCheck outside the inventory lock; -// the busy mark is inserted only after a second availability re-check -// confirms no goroutine raced in. An outstanding lease counts as busy so -// legacy OP_REQ_IMPORT cannot steal a slot a control client has already -// reserved via IssueLease. The caller must pair every success with a -// later ReleaseImport, and is responsible for broadcasting the busy -// transition once the session is fully wired up. +// TryReserveForImport runs LeaseCheck outside the lock and re-checks +// availability before marking busy. Caller must pair success with +// ReleaseImport and broadcast once the session is wired up. func (l *exportLedger) TryReserveForImport(ctx context.Context, busid string) (Export, bool, string) { var ( export Export @@ -278,13 +248,8 @@ func (l *exportLedger) ReleaseImport(ctx context.Context, busid string, removeEx }) } -// IssueLease captures the current broadcast sequence as opaque metadata -// for control clients. Lease correctness itself is pinned to the -// export's internal identity, not to that sequence number. Both -// inventory stages run through withInventoryWrite so any lease insert, -// re-validation, or TTL sweep broadcasts the resulting reserved-state -// change to subscribers — without this, an unexpired lease would block -// new imports while extended clients still saw the device as available. +// IssueLease pins lease correctness to the export identity; the +// broadcast sequence on the response is opaque metadata for clients. func (l *exportLedger) IssueLease(ctx context.Context, subID uint64, request controlLeaseRequest) controlLeaseResponse { response := controlLeaseResponse{ BusID: request.BusID, @@ -296,9 +261,9 @@ func (l *exportLedger) IssueLease(ctx context.Context, subID uint64, request con return response } - l.fast.Lock() + l.broadcastAccess.Lock() generation := l.seq - l.fast.Unlock() + l.broadcastAccess.Unlock() var ( export Export @@ -378,16 +343,9 @@ func (l *exportLedger) IssueLease(ctx context.Context, subID uint64, request con return response } -// ConsumeLeaseAndReserve validates the requested lease against the -// current export identity, reruns LeaseCheck outside the inventory lock, -// then atomically consumes the lease and marks the busid busy. -// Correctness is tied to the export identity rather than to the control -// sequence. -// -// Consume-on-read semantics from the old ConsumeLease are preserved: -// the lease entry is removed on every outcome except a nonce/ID -// mismatch (which preserves the lease for the legitimate holder). -// The caller must pair every success with a later ReleaseImport. +// ConsumeLeaseAndReserve consumes the lease on every outcome except an +// ID/nonce mismatch (the latter preserves the lease for the legitimate +// holder). Caller must pair success with ReleaseImport. func (l *exportLedger) ConsumeLeaseAndReserve(ctx context.Context, request ImportExtRequest) (Export, bool, string) { var ( export Export @@ -527,23 +485,23 @@ func (l *exportLedger) Subscribe(ctx context.Context, conn net.Conn, capabilitie if extended { // Keep the snapshot and sequence from the same stable generation. for { - l.fast.Lock() + l.broadcastAccess.Lock() sequence = l.seq - l.fast.Unlock() + l.broadcastAccess.Unlock() snapshot = l.snapshotDeviceState(ctx) - l.fast.Lock() + l.broadcastAccess.Lock() if sequence == l.seq { break } - l.fast.Unlock() + l.broadcastAccess.Unlock() } } else { - l.fast.Lock() + l.broadcastAccess.Lock() sequence = l.seq } - defer l.fast.Unlock() + defer l.broadcastAccess.Unlock() l.nextSubID++ sub := &exportSubscriber{ id: l.nextSubID, @@ -572,9 +530,9 @@ func (l *exportLedger) Subscribe(ctx context.Context, conn net.Conn, capabilitie // broadcast so remaining subscribers see the busid become available // again. func (l *exportLedger) Unsubscribe(ctx context.Context, sub *exportSubscriber) { - l.fast.Lock() + l.broadcastAccess.Lock() delete(l.subs, sub.id) - l.fast.Unlock() + l.broadcastAccess.Unlock() l.withInventoryWrite(ctx, func() bool { released := false for busid, lease := range l.leases { @@ -590,13 +548,13 @@ func (l *exportLedger) Unsubscribe(ctx context.Context, sub *exportSubscriber) { // CloseAllSubscribers returns the underlying connections so the caller // can close them outside any lock. func (l *exportLedger) CloseAllSubscribers() []net.Conn { - l.fast.Lock() + l.broadcastAccess.Lock() conns := make([]net.Conn, 0, len(l.subs)) for _, sub := range l.subs { conns = append(conns, sub.conn) } l.subs = make(map[uint64]*exportSubscriber) - l.fast.Unlock() + l.broadcastAccess.Unlock() return conns } @@ -612,9 +570,9 @@ func (l *exportLedger) HandleControlLeaseRequest(ctx context.Context, sub *expor var request controlLeaseRequest err := unmarshalControlPayload(payload, &request) if err != nil { - l.fast.Lock() + l.broadcastAccess.Lock() sequence := l.seq - l.fast.Unlock() + l.broadcastAccess.Unlock() l.enqueuePayload(sub, controlFrame{ Type: controlFrameLeaseResponse, Version: controlProtocolVersion, @@ -625,9 +583,9 @@ func (l *exportLedger) HandleControlLeaseRequest(ctx context.Context, sub *expor return } response := l.IssueLease(ctx, sub.id, request) - l.fast.Lock() + l.broadcastAccess.Lock() sequence := l.seq - l.fast.Unlock() + l.broadcastAccess.Unlock() l.enqueuePayload(sub, controlFrame{ Type: controlFrameLeaseResponse, Version: controlProtocolVersion, diff --git a/service/usbip/host.go b/service/usbip/host.go index deaec1ab5..2024c7e13 100644 --- a/service/usbip/host.go +++ b/service/usbip/host.go @@ -7,22 +7,15 @@ import ( "net" ) -// ExportHost lifecycle: Start → Reconcile* → Close. Reconcile may be -// called many times; it returns the committed post-reconcile export -// state, and callers must apply snapshot/released even when err != nil. -// FinishImport runs after each data session ends so the platform can do -// post-import cleanup (Linux: write -1 to usbip_sockfd; Darwin: -// release stale-marked captures). +// ExportHost lifecycle: Start → Reconcile* → Close. Callers must apply +// the Reconcile snapshot and released list even when err != nil. type ExportHost interface { Start(ctx context.Context) error Close() error Reconcile(ctx context.Context, isReserved func(busid string) bool) (snapshot map[string]Export, released []string, err error) FinishImport(ctx context.Context, busid string) (released bool, err error) - // Events returns a coalescing channel that signals topology - // changes. The channel is closed when Close() is called, which - // also stops the host's background goroutines. Events MUST be - // called after Start succeeds. A non-nil error means the host - // could not subscribe and the server must not continue. + // Events MUST be called after Start succeeds. The channel closes when + // Close() runs. Events() (<-chan struct{}, error) } @@ -35,20 +28,10 @@ type ImportHost interface { type ExportLeaseIdentity string -// Export is the host-owned handle to a single USB device that the ledger -// publishes to control subscribers and hands to import sessions. -// -// Pointers returned from ExportHost.Reconcile MUST be treated as -// immutable from the ledger's perspective: every field read by -// BusID, Snapshot, LeaseIdentity, LeaseCheck, or DeviceInfo must not -// change after the Export is published into the snapshot map. The -// ledger calls these methods outside any lock — concurrent mutation is -// a data race. Hosts that need to change such state (mark stale, swap -// underlying device handle, …) MUST publish a fresh Export pointer by -// cloning, mutating the clone, then swapping it into the host's -// committed map under the host's own lock. See cloneLinuxExport and -// docs/adr/0001-export-pointer-immutability.md for the canonical -// pattern; applyStaleClones enforces the rule for stale transitions. +// Export pointers handed back from Reconcile are immutable from the +// ledger's perspective: the ledger calls the methods below outside any +// lock. Hosts that need to mutate must clone, mutate the clone, then +// swap it into their committed map under the host's own lock. type Export interface { BusID() string Snapshot(ctx context.Context, busy bool) ExportSnapshot @@ -58,31 +41,8 @@ type Export interface { NewServerDataSession(ctx context.Context, conn net.Conn) (DataSession, error) } -// applyStaleClones honours the Export immutability contract for -// stale-mark transitions: for every busid in toStale that exists in -// committed, it allocates a clone, runs mark on the clone, and writes -// the clone back. The previously published pointer is never mutated. -// Callers that consumed the original pointer continue to observe its -// pre-stale snapshot until they next refresh from committed. -func applyStaleClones[T any](committed map[string]*T, toStale []string, clone func(*T) *T, mark func(*T)) { - for _, busid := range toStale { - exp, found := committed[busid] - if !found { - continue - } - cloned := clone(exp) - mark(cloned) - committed[busid] = cloned - } -} - -// ExportSnapshot Backend and StableID fields are populated -// unconditionally. Unavailable snapshots keep a cached Entry, -// including BusID, so the caller can broadcast a state transition -// instead of removing the device outright; snapshots without a BusID -// are treated as non-broadcastable. ExportHost.Reconcile returns -// stale entries in its snapshot for the same reason — the ledger -// filters non-broadcastable snapshots in one place, not the host. +// ExportSnapshot entries with an empty BusID are filtered out by the +// ledger; hosts may keep stale snapshots so state transitions broadcast. type ExportSnapshot struct { Entry DeviceEntry Backend string @@ -92,16 +52,9 @@ type ExportSnapshot struct { RawStatus int } -// DataSession implementations MUST close the channel returned by Done -// when the session terminates for any reason. Err is only valid after -// Done is closed; it returns nil for a clean detach. Close is idempotent -// and safe to call from any goroutine. -// -// Start transfers the session from "prepared" to "running". The session -// is returned in the prepared state with the userspace conn still alive -// so the server can send OP_REP_IMPORT before the kernel takes wire -// ownership. Start is idempotent. Close before Start releases all -// resources, including the userspace conn. +// DataSession implementations MUST close Done when the session +// terminates. Err is only valid after Done. Start and Close are +// idempotent; Close before Start releases all resources. type DataSession interface { Done() <-chan struct{} Err() error diff --git a/service/usbip/host_darwin.go b/service/usbip/host_darwin.go index a200cd933..a019e9097 100644 --- a/service/usbip/host_darwin.go +++ b/service/usbip/host_darwin.go @@ -178,25 +178,20 @@ func (h *darwinExportHost) Reconcile(ctx context.Context, isReserved func(busid released = append(released, busid) } - // Build the committed map off-lock so we can clone any stale entry - // before mutating it. The ledger reads Snapshot/LeaseCheck on the - // previously published pointers without locks; mutating them in - // place is a data race. See docs/adr/0001-export-pointer-immutability.md. committed := make(map[string]*darwinExport, len(current)+len(toAdd)) maps.Copy(committed, current) - staleBusIDs := make([]string, 0, len(toStale)) - pendingByBusID := make(map[string]uint64, len(toStale)) for _, mark := range toStale { - staleBusIDs = append(staleBusIDs, mark.busid) - pendingByBusID[mark.busid] = mark.pendingRegistryID - } - applyStaleClones(committed, staleBusIDs, cloneDarwinExport, func(exp *darwinExport) { - exp.stale = true - pending := pendingByBusID[exp.busid] - if pending != 0 { - exp.pendingRegistryID = pending + exp, found := committed[mark.busid] + if !found { + continue } - }) + cloned := cloneDarwinExport(exp) + cloned.stale = true + if mark.pendingRegistryID != 0 { + cloned.pendingRegistryID = mark.pendingRegistryID + } + committed[mark.busid] = cloned + } for _, exp := range toRemove { delete(committed, exp.busid) } @@ -281,11 +276,6 @@ func snapshotDarwinExports(exports map[string]*darwinExport) map[string]Export { return out } -// cloneDarwinExport returns a fresh *darwinExport with the slice fields -// inside entry duplicated so callers can mutate the clone without -// affecting the previously published pointer. The device handle and -// logger are shared because they are externally synchronised. See -// docs/adr/0001-export-pointer-immutability.md. func cloneDarwinExport(exp *darwinExport) *darwinExport { if exp == nil { return nil diff --git a/service/usbip/host_linux.go b/service/usbip/host_linux.go index 5da8b0c7a..739dbdb10 100644 --- a/service/usbip/host_linux.go +++ b/service/usbip/host_linux.go @@ -340,9 +340,15 @@ func (h *linuxExportHost) Reconcile(ctx context.Context, isReserved func(busid s maps.Copy(committed, current) var reconcileErrors []error - applyStaleClones(committed, plan.toStale, cloneLinuxExport, func(exp *linuxExport) { - exp.stale = true - }) + for _, busid := range plan.toStale { + exp, found := committed[busid] + if !found { + continue + } + cloned := cloneLinuxExport(exp) + cloned.stale = true + committed[busid] = cloned + } for _, exp := range plan.toRelease { releaseErr := h.releaseExport(exp) diff --git a/service/usbip/iso_scheduler.go b/service/usbip/iso_scheduler.go index b099867a0..e5ad0204c 100644 --- a/service/usbip/iso_scheduler.go +++ b/service/usbip/iso_scheduler.go @@ -35,36 +35,26 @@ func RebaseFrame(currentFrame uint64, low8 uint8) uint64 { return base } -var ( - errIsoDescriptorCount = E.New("RET_SUBMIT iso descriptor count mismatch") - errIsoDescriptorRange = E.New("RET_SUBMIT iso descriptor range mismatch") - errIsoDescriptorActualLength = E.New("RET_SUBMIT iso descriptor actual_length exceeds length") - errIsoDescriptorSum = E.New("RET_SUBMIT iso descriptor actual_length sum does not match header") - errIsoPayloadShort = E.New("RET_SUBMIT iso payload shorter than descriptor range") -) - // ValidateIsoResponse enforces the per-descriptor invariants of a RET_SUBMIT // against the original CMD_SUBMIT shape. startIsoTransfer emits single-packet // CMD_SUBMITs that cover the whole request, so the response must mirror that -// exact shape. If multi-packet ISO submits are added later, the count check -// relaxes to a non-empty count and the offset/length check becomes a per- -// descriptor walk that proves coverage is non-overlapping and in-range. +// exact shape. func ValidateIsoResponse(requestLen int, actualLength int, packets []IsoPacketDescriptor, payloadLen int) error { if len(packets) != 1 { - return E.Extend(errIsoDescriptorCount, "expected 1, got ", len(packets)) + return E.New("RET_SUBMIT iso descriptor count mismatch: expected 1, got ", len(packets)) } descriptor := packets[0] if descriptor.Offset != 0 || int(descriptor.Length) != requestLen { - return E.Extend(errIsoDescriptorRange, "offset ", descriptor.Offset, ", length ", descriptor.Length, ", request ", requestLen) + return E.New("RET_SUBMIT iso descriptor range mismatch: offset ", descriptor.Offset, ", length ", descriptor.Length, ", request ", requestLen) } if descriptor.ActualLength < 0 || descriptor.ActualLength > descriptor.Length { - return E.Extend(errIsoDescriptorActualLength, "actual_length ", descriptor.ActualLength, ", length ", descriptor.Length) + return E.New("RET_SUBMIT iso descriptor actual_length exceeds length: actual_length ", descriptor.ActualLength, ", length ", descriptor.Length) } if int(descriptor.ActualLength) != actualLength { - return E.Extend(errIsoDescriptorSum, "sum ", descriptor.ActualLength, " != header ", actualLength) + return E.New("RET_SUBMIT iso descriptor actual_length sum does not match header: sum ", descriptor.ActualLength, " != header ", actualLength) } if int(descriptor.ActualLength) > payloadLen { - return E.Extend(errIsoPayloadShort, "actual_length ", descriptor.ActualLength, " > payload ", payloadLen) + return E.New("RET_SUBMIT iso payload shorter than descriptor range: actual_length ", descriptor.ActualLength, " > payload ", payloadLen) } return nil } diff --git a/service/usbip/usbhost_darwin.go b/service/usbip/usbhost_darwin.go index d33039c25..186228844 100644 --- a/service/usbip/usbhost_darwin.go +++ b/service/usbip/usbhost_darwin.go @@ -75,13 +75,8 @@ 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). +// cgoCallbackHandle: destroyC MUST synchronously drain the C-side queue +// before returning; only then is it safe to delete the handle. type cgoCallbackHandle struct { handle cgo.Handle } @@ -94,8 +89,7 @@ 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. +// deleteRaw rolls back a failed C-side create without a drain. func (c cgoCallbackHandle) deleteRaw() { c.handle.Delete() }