Fix #3347 by implementing a read loop until all file bytes are read. #3349
Conversation
This file contains 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
ke6jjj
requested review from
skotopes,
DrZlo13,
hedger,
gornekich and
Astrrra
as code owners
|
This issue likely also exists in the "read file value" and "read file records" code paths. I've only fixed the "read normal file" path. |
Fix an off-by-one bug which drops DESFire responses which are actually perfectly fine. The bug occurs due to the fact that the response handling code forgets to discount the "MF_DESFIRE_FLAG_HAS_NEXT" byte at the front of all continued reponses, and thus, determines that the reponse is too big (by one byte).
Update the file read loop to size its read requests perfectly for the receive buffer, rather than banging its head repeatedly against the wall in the receive handler by dropping incoming responses.
|
As noted in #3347, I've pushed in some more fixes to this PR. One fixes an off-by-one counting bug which was causing received responses to be thrown away when they actually were perfectly fine. The other is a change to the file read polling loop such that it requests no more bytes than can actually fit into the receive buffer. |
gornekich
requested changes
Jan 12, 2024
| @@ -301,24 +303,35 @@ MfDesfireError mf_desfire_poller_read_file_data( | |||
| size_t size, | |||
| MfDesfireFileData* data) { | |||
| furi_assert(instance); | |||
| size_t cur_offset, read_size, num_read; | |||
| MfDesfireError error; | |||
There was a problem hiding this comment.
Compiler thinks that we might not get into for loop, where we update error variable. Please, initialize this variable here
There was a problem hiding this comment.
No problem. I'll look at it this week.
|
Un-draft when ready |
|
Fixed arrive in dev |
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.
What's new
This fixes #3347 by implementing a read loop which reads the file bit by bit until completed.
Verification
Tested with a Mifare DESFire card which exhibited the initial issue.
Checklist (For Reviewer)