fix(keepkey-webusb): clearHalt the IN pipe on a stalled read, not OUT#45
Merged
Conversation
readChunk() reads the IN endpoint (transferIn) but on a "stall" it cleared
clearHalt("out", ...) -- the wrong endpoint and direction. A halted IN pipe is
only resumed by resetting that same pipe, so it stayed halted. The browser
transport also fell through and returned the stalled (empty/short) buffer, which
the framing parser then rejected as "message not valid".
Reset the IN pipe and throw a retryable "bad read" instead of surfacing a
non-packet. Applied to both hdwallet-keepkey-nodewebusb (Node/libusb) and
hdwallet-keepkey-webusb (browser).
Split out from #44 (which also added transfer timeouts); this is the isolated,
low-risk correctness fix.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
readChunk()reads the IN endpoint (transferIn), but on a"stall"it calledclearHalt("out", …)— the wrong endpoint and direction. On WinUSB a halted IN pipe is only resumed by resetting that same pipe, so the IN pipe stayed halted and subsequent reads re-stalled. The browser transport additionally fell through and returned the stalled (empty/short) buffer, which the framing parser then rejected as"message not valid".Fix
clearHalt("in", …)on the pipe that actually stalled, and throw a retryable"bad read"instead of surfacing a non-packet. Applied to both:hdwallet-keepkey-nodewebusb(Node/libusb — used by the desktop Vault)hdwallet-keepkey-webusb(browser)Scope
This is split out of #44 as the isolated, low-risk correctness fix. The transfer-timeout work from #44 is intentionally not included here (it touches the shared transport more invasively and wants its own validation). #44 is being closed in favor of this.
Note on impact: on the node transport the old
clearHalt("out")branch was effectively dead code (a node stall returns nodata, so the old&& result.data !== undefinedguard never fired and it already threw). The actively-buggy path was the browser transport. Either way this is the correct behavior.CI
The repo's
buildcheck is currently red on pre-existing prettier/eslint errors inpackages/hdwallet-keepkey/src/zcash.ts+zcash.test.ts(the target of #41) — unrelated to this PR. The two files changed here passprettier --checkandeslintcleanly.Testing
stallstatus assertingclearHalt("in", …)+ throw.