fix(thp): decode a packet that exactly fills the packet size - #2
Open
munzzyy wants to merge 1 commit into
Open
Conversation
decodePackage treated a message as incomplete when its length field was equal to the bytes left in the packet, so the largest message that still fits in one packet was reported as needing a continuation packet. The transports then blocked waiting for a packet the device never sends. The complete branch consumes exactly length bytes, so it only needs remainingLength >= length.
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
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.
TrezorDecoder.decodePackagedecides whether a THP v2 packet already holds the whole message by comparing the length field against the bytes left in the reader:lengthcounts the payload plus the 4 byte CRC, and the second branch reads exactly that many bytes, so the message is complete wheneverremainingLength >= length. With>=on the incomplete side the equality case lands in the wrong branch. That case is the largest message that still fits in a single packet: 235 payload bytes over a 244 byte BLE MTU, or 55 payload bytes over a 64 byte USB packet. It comes back withcrc == null, soneedsContinuationPacketis true and both transports wait for a packet the device is never going to send._readResponseintrezor_usb_manager.dartkeeps callingtransferIn(), and the notify handler intrezor_gatt_gateway.dartreturns early without completing the request.The change is the comparison. I also added
test/trezor/protocol/decoder_test.dartwith a helper that builds a real THP packet (control byte, channel, length, payload, CRC32, padded the way the device pads) and covers three cases: a short padded packet, the exact fit packet, and a message too large for one packet.Running the new test on trunk with the one line change reverted:
239 is the whole remainder including the CRC. With the fix:
flutter testfor the whole package is green at 31 tests, andflutter analyzereports nothing on either changed file. Flutter 3.44.6, Dart 3.12.2, Linux.