fix incorrect MTP xact_len calculation #3284
Merged
Conversation
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
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug in the MTP device class where transfers larger than 65535 bytes would fail due to incorrect type casting in the transaction length calculation. The issue occurred because the subtraction result was cast to uint16_t before taking the minimum, causing overflow for large file transfers.
- Fixed the order of operations in
xact_lencalculation to prevent integer overflow - Changed from
tu_min16()with prematureuint16_tcast totu_min32()with post-calculation cast
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Describe the PR
Fixes the calculation of remaining bytes to transfer in MTP device class.
The original code casts the result of subtraction into
uint16_tbefore taking the minimum, which will break all transfers with more than 65535 bytes. Only(filesize & 0xFFFF)bytes can be transferred before the transfer comes to a halt. This PR has deferred the cast touint16_tafter calculating the transfer size inuint32_tdomain.This bug is discovered when I'm testing MTP Device with an LittleFS on SPI flash as the backing store, with about 11MiB of free space I copied a lot more data than what was available in the examples and it immediately revealed this issue. After applying this change, the issue was resolved and I can copy megabytes of files into the responder device.
And file hashes are compared. The file was copied to the responder device on macOS and is copied out from responder device on Windows.
Additional context
The test project can be found at https://github.com/RigoLigoRLC/esp32s3-tusb-mtp . It uses my own fork of espressif/tinyusb with MTP implementation applied on top of v0.18 branch.