Fix USB CAT write/read NPE when port I/O races an in-progress (re)open#651
Conversation
CommonUsbSerialPort.open() publishes mConnection *before* openInt() assigns
mReadEndpoint/mWriteEndpoint (openInt and the catch-path close() both need the
connection during setup). This leaves a window where mConnection != null but the
endpoints are still null. read()/write() only guarded `mConnection == null`, so
a call landing in that window passed the check and then NPE'd dereferencing the
endpoint — write() at `mWriteEndpoint.getMaxPacketSize()`.
sendData() catches IOException but not the RuntimeException NPE, so it escaped to
the TX worker thread and crashed the app. The window is hit in practice during
CAT auto-reconnect: prepare() allocates a fresh port (endpoints null) and a TX
PTT firing from the transmit thread while open() is running keys the crash
(observed on a Pixel 8 / Android 16: Yaesu39 setPTT -> FT8TransmitSignal ->
CommonUsbSerialPort.write).
Guard the endpoint as well as the connection via a small, unit-testable
ioEndpointReady() helper, applied symmetrically to read() and write(). A missing
connection OR endpoint now throws the same recoverable IOException("Connection
closed") the CAT/TX layer already handles instead of crashing. Strict superset of
the previous check — no behavior change once the port is fully open.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #651 +/- ##
============================================
+ Coverage 36.73% 36.81% +0.08%
- Complexity 197 207 +10
============================================
Files 216 218 +2
Lines 26888 27012 +124
Branches 3287 3315 +28
============================================
+ Hits 9877 9945 +68
- Misses 16785 16832 +47
- Partials 226 235 +9
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR hardens the USB-serial CAT transport against a real-world race during (re)open where mConnection becomes visible before openInt() assigns mReadEndpoint/mWriteEndpoint, preventing an uncaught NullPointerException from escaping CommonUsbSerialPort.read()/write() and crashing the TX/CAT worker thread.
Changes:
- Add
CommonUsbSerialPort.ioEndpointReady(connection, endpoint)to centralize the “safe to do I/O” decision and make it unit-testable. - Update
CommonUsbSerialPort.read()andwrite()to treat a missing endpoint the same as a missing connection (throw recoverableIOException("Connection closed")). - Add a pure-JVM unit test covering endpoint readiness cases, including the exact crash window.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| ft8af/app/src/main/java/com/k1af/ft8af/serialport/CommonUsbSerialPort.java | Adds an endpoint-readiness helper and extends read/write guards to cover the connection-published/endpoint-null race window. |
| ft8af/app/src/test/java/com/k1af/ft8af/serialport/CommonUsbSerialPortEndpointReadyTest.java | Adds JVM unit coverage for the readiness decision to prevent regressions of the crash window. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
A concurrent close() (cable yanked on another thread) nulls the mConnection / mReadEndpoint / mWriteEndpoint fields, so an in-flight read()/write() that dereferenced the live fields could still NPE after the readiness guard passed. Read them into locals once, up front, and use the locals throughout — a concurrent close now degrades to the existing recoverable IOException path instead of crashing the IO thread. The guard still covers the (re)open race the PR targets. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Root cause
CommonUsbSerialPort.open()publishesmConnectionbeforeopenInt()assignsmReadEndpoint/mWriteEndpoint:openInt()and the catch-pathclose()both need the connection during setup, so the ordering is deliberate — but it leaves a window wheremConnection != nullwhilemWriteEndpoint/mReadEndpointare stillnull.read()/write()only guardedmConnection == null. A call landing in that window passed the guard and then dereferenced a null endpoint:CableSerialPort.sendData()catchesIOExceptionbut not theRuntimeExceptionNPE, so it escaped to the TX worker thread and crashed the app.Evidence
Sentry fatal (Pixel 8 / Android 16,
radio.ks3ckc.ft8af@1.1.0-dev+5):The window is hit in practice during CAT auto-reconnect:
CableSerialPort.prepare()allocates a fresh port (driver.getPorts().get(portNum), endpoints null) andconnect()callsopen(); a TX PTT firing from the transmit thread during thatopen()keys the crash. This is the endpoint-side twin of the earlier disconnect-race guards (writeIfOpen, PR #647).Fix
Guard the endpoint as well as the connection, via a small unit-testable helper applied symmetrically to
read()andwrite():A missing connection or endpoint now throws the same recoverable
IOException("Connection closed")that the CAT/TX layer already handles, instead of crashing. This is a strict superset of the previousmConnection == nullcheck — no behavior change once the port is fully open.Testing
CommonUsbSerialPortEndpointReadyTest(pure JVM) covers the readiness decision, including the exact crash caseioEndpointReady(connection, null) == false../gradlew :app:testDebugUnitTest— full suite green.Risk
Very low. Localized to two guard checks in
CommonUsbSerialPort; only adds an earlier, already-handledIOExceptionon a previously-crashing path. No protocol, DSP, or performance impact.🤖 Generated with Claude Code