Skip to content

Commit 18cf2f6

Browse files
committed
Error handling for Async IO task (macOS)
1 parent c69c072 commit 18cf2f6

3 files changed

Lines changed: 69 additions & 16 deletions

File tree

‎java-does-usb/robustness.md‎

Lines changed: 36 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -366,25 +366,55 @@ The original description follows.
366366
All are cheap to fix with `volatile`. Practical impact is low (stale reads, not corruption), but
367367
`isConnected()` returning `true` long after unplug is user-visible.
368368

369-
---
370-
371-
## Open findings
369+
## 9. `messagePort` (remote) creation is unchecked (macOS) — **FIXED**
370+
371+
> **Status:** Fixed on 2026-07-06. `startAsyncIOThread` now null-checks all *three* CF creations
372+
> that can return NULL — `CFMessagePortCreateLocal`, `CFMessagePortCreateRunLoopSource`, and
373+
> `CFMessagePortCreateRemote` (a NULL anywhere in the chain causes the same downstream damage,
374+
> e.g. when bootstrap-port registration fails in a sandboxed process) — and throws a `UsbException`
375+
> naming the failed call. The remote port is assigned to the `messagePort` field only after the
376+
> check, so `removeEventSource` can never send on a NULL port.
377+
>
378+
> The naive throw would have traded the leak for a hang: the singleton would be stuck in state
379+
> `STARTING` and every later `addEventSource` call would block forever in `waitForRunLoopReady`.
380+
> The catch block therefore releases the partially created ports (`CFRelease`; releasing the last
381+
> reference also invalidates the local port and deregisters its name, so a retry can recreate it)
382+
> and resets the state to `NOT_STARTED`, so a later `open()` retries cleanly. The exception
383+
> surfaces from `MacosUsbDevice.open()` before anything is registered with the run loop.
384+
>
385+
> Verified with the full hardware test suite on macOS (59 tests, all passing); the failure paths
386+
> themselves require injected CF failures and are verified by inspection.
372387
373-
## 9. `messagePort` (remote) creation is unchecked (macOS)
388+
The original description follows.
374389

375390
`MacosAsyncTask.java:135`: `CFMessagePortCreateRemote` can return NULL (e.g. a
376391
race where the local port's run-loop source isn't serviced yet).
377392
`removeEventSource` would then send on a NULL port and the event source would
378393
never be removed — a slow leak of run-loop sources. Worth asserting non-null at
379394
creation.
380395

381-
## 10. Magic-number reinterpret in `loadDescription` (macOS)
396+
## 10. Magic-number reinterpret in `loadDescription` (macOS) — **FIXED**
397+
398+
> **Status:** Fixed on 2026-07-06. The pointer returned by `GetConfigurationDescriptorPtr` is now
399+
> first reinterpreted to exactly `ConfigurationDescriptor.LAYOUT.byteSize()` (9 bytes) — the
400+
> minimum needed to read the header — then, after a sanity check that `wTotalLength` is at least
401+
> the header size (throwing a `UsbException` naming the bogus value otherwise), reinterpreted to
402+
> exactly `totalLength`. No magic number remains, and the view never claims memory beyond the
403+
> kernel buffer, which IOKit itself sized from the same `wTotalLength` field when caching the
404+
> descriptor. Verified with the full hardware test suite on macOS (59 tests, all passing) —
405+
> `loadDescription` runs during enumeration of every connected device.
406+
407+
The original description follows.
382408

383409
`MacosUsbDevice.java:194`: `dereference(descPtrHolder).reinterpret(999999)`
384410
before re-slicing to `totalLength()`. It's bounded immediately afterward so it's
385411
safe in practice, but the arbitrary size is fragile if a malformed descriptor
386412
ever reports a `totalLength` beyond the real buffer.
387413

414+
---
415+
416+
## Open findings
417+
388418
## 11. `open()` holds the device monitor across up to 4×90 ms sleeps (macOS)
389419

390420
`MacosUsbDevice.java:103-135` is `synchronized` and sleeps while retrying
@@ -464,8 +494,7 @@ timeout rather than an infinite block).
464494

465495
## Recommended priority
466496

467-
Findings 1–8 are fixed (see above). Next up: the macOS message-port null check of
468-
finding 9. Findings 10–12 are low-priority robustness notes.
497+
Findings 1–10 are fixed (see above). Findings 11–12 are low-priority robustness notes.
469498

470499
The Linux fixes for findings 1–3 still need a hardware test run on a Linux machine, and the
471500
finding 7 fix a hardware regression run on Windows.

‎java-does-usb/src/main/java/net/codecrete/usb/macos/MacosAsyncTask.java‎

Lines changed: 25 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,8 @@ void removeEventSource(MemorySegment source) {
118118
*/
119119
@SuppressWarnings("java:S125")
120120
private void startAsyncIOThread() {
121-
MemorySegment messagePortSource;
121+
MemorySegment messagePortSource = NULL;
122+
MemorySegment localPort = NULL;
122123

123124
try {
124125
state = TaskState.STARTING;
@@ -130,19 +131,37 @@ private void startAsyncIOThread() {
130131
var methodHandle = asyncIOCompletedMH.bindTo(this);
131132
completionUpcallStub = Linker.nativeLinker().upcallStub(methodHandle, completionHandlerFuncDesc, Arena.global());
132133

133-
// create local and remote message ports
134+
// create local and remote message ports (all three CF creations can return NULL,
135+
// e.g. if bootstrap port registration fails in a sandboxed process)
134136
var pid = ProcessHandle.current().pid();
135137
var portName = CoreFoundationHelper.createCFStringRef("net.codecrete.usb.macos.eventsource." + pid, Arena.global());
136138
var messagePortCallback = CFMessagePortCreateLocal$callout.allocate(this::messagePortCallback, Arena.global());
137-
var localPort = CoreFoundation.CFMessagePortCreateLocal(NULL, portName, messagePortCallback, NULL, NULL);
139+
localPort = CoreFoundation.CFMessagePortCreateLocal(NULL, portName, messagePortCallback, NULL, NULL);
140+
if (localPort.address() == 0)
141+
throw new UsbException("internal error (CFMessagePortCreateLocal failed)");
138142
messagePortSource = CoreFoundation.CFMessagePortCreateRunLoopSource(NULL, localPort, 0);
139-
messagePort = CoreFoundation.CFMessagePortCreateRemote(NULL, portName);
143+
if (messagePortSource.address() == 0)
144+
throw new UsbException("internal error (CFMessagePortCreateRunLoopSource failed)");
145+
var remotePort = CoreFoundation.CFMessagePortCreateRemote(NULL, portName);
146+
if (remotePort.address() == 0)
147+
throw new UsbException("internal error (CFMessagePortCreateRemote failed)");
148+
messagePort = remotePort;
140149

141-
} catch (IllegalAccessException | NoSuchMethodException e) {
150+
} catch (Exception e) {
151+
// release partially created ports and reset the state; otherwise the task would be
152+
// stuck in STARTING and all later addEventSource() calls would wait forever
153+
if (messagePortSource.address() != 0)
154+
CoreFoundation.CFRelease(messagePortSource);
155+
if (localPort.address() != 0)
156+
CoreFoundation.CFRelease(localPort);
157+
state = TaskState.NOT_STARTED;
158+
if (e instanceof RuntimeException runtimeException)
159+
throw runtimeException;
142160
throw new UsbException("internal error (creating method handle)", e);
143161
}
144162

145-
var thread = new Thread(() -> asyncIOCompletionTask(messagePortSource), "USB async IO");
163+
var source = messagePortSource;
164+
var thread = new Thread(() -> asyncIOCompletionTask(source), "USB async IO");
146165
thread.setDaemon(true);
147166
thread.start();
148167
}

‎java-does-usb/src/main/java/net/codecrete/usb/macos/MacosUsbDevice.java‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -199,9 +199,14 @@ private void loadDescription(MemorySegment device) {
199199
if (ret != 0)
200200
throwException(ret, "querying first configuration failed");
201201

202-
var configDesc = dereference(descPtrHolder).reinterpret(999999);
203-
var configDescHeader = new ConfigurationDescriptor(configDesc);
204-
configDesc = configDesc.asSlice(0, configDescHeader.totalLength());
202+
// read the descriptor header with a minimally sized view first, then resize to the
203+
// total length the header reports (the kernel buffer was sized from the same field)
204+
var headerSize = ConfigurationDescriptor.LAYOUT.byteSize();
205+
var configDescHeader = new ConfigurationDescriptor(dereference(descPtrHolder).reinterpret(headerSize));
206+
var totalLength = configDescHeader.totalLength();
207+
if (totalLength < headerSize)
208+
throwException("invalid configuration descriptor (wTotalLength: %d)", totalLength);
209+
var configDesc = dereference(descPtrHolder).reinterpret(totalLength);
205210

206211
var configuration = setConfigurationDescriptor(configDesc);
207212
configurationValue = 255 & configuration.configValue();

0 commit comments

Comments
 (0)