Fix bug where Specter crashes if Jade is in use by another app#2559
Fix bug where Specter crashes if Jade is in use by another app#2559a4to wants to merge 1 commit intocryptoadvance:masterfrom
Conversation
✅ Deploy Preview for specter-desktop-docs canceled.
|
|
Please add an issue to this PR. Why do you think there is a bug-bounty? Can you add tests and maybe proof that this is fixed with that code? |
al-munazzim
left a comment
There was a problem hiding this comment.
Thanks for tackling the Jade serial crash (#2343). A few concerns:
-
Not wired in —
jade_serial.pyis a standalone file but nothing in the existing codebase imports or callsclose_open_ports(). It needs to be integrated into the actual Jade device initialization path (likely insrc/cryptoadvance/specter/devices/). -
Placeholder code —
connect_to_jade()has# Replace with actual Jade connection codecomments. This suggests it's a proof-of-concept, not a production fix. -
Aggressive port closing —
close_open_ports()grabs the first serial port and closes it unconditionally. This could interfere with other devices (ColdCard, Trezor, etc.) that also use serial. A safer approach: catch theSerialExceptionwhen opening the Jade port specifically, and show a user-friendly error ("Jade appears to be in use by another app").
Would recommend looking at how HWIDevice handles Jade enumeration and adding error handling there rather than a separate file.
|
@al-munazzim please crosscheck with #2578 which seem to try to solve the same issue. |
|
And also #2578 |
al-munazzim
left a comment
There was a problem hiding this comment.
Review: ❌ Request Changes
This PR creates a new standalone file (jade_serial.py) that isn't integrated into the existing codebase. The code contains placeholder comments (# Replace with actual Jade connection code) and isn't wired into any import or call site.
While the idea of closing stale serial ports before reconnecting has merit, this implementation:
- Creates a new file rather than modifying the existing Jade connection logic
- Contains stub/placeholder code that doesn't actually connect to anything
- Doesn't address the actual crash in
jade_tcp.py(the__enter__/__exit__bug) - Missing newline at end of file
Recommendation: The TCP crash from #2343 is properly fixed by #2578. If serial port cleanup is needed separately, it should be integrated into the existing jade.py or device manager code as a separate issue/PR.
This PR adds logic to check for and close open serial ports before connecting to Jade, addressing the crash issue reported in the bug bounty. The changes are made in the Jade serial connection handling to ensure ports are properly closed before use.