extmod: Check vfs block device invalid return values, increase error handling #13572
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
|
Code size report: |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #13572 +/- ##
==========================================
- Coverage 98.57% 98.57% -0.01%
==========================================
Files 164 164
Lines 21341 21330 -11
==========================================
- Hits 21036 21025 -11
Misses 305 305 ☔ View full report in Codecov by Sentry. |
This comment was marked as resolved.
This comment was marked as resolved.
1a178bc to
d278b46
Compare
projectgus
commented
Sep 3, 2024
d278b46 to
4bc7276
Compare
7f74230 to
7eb2fbb
Compare
|
This is awesome! We get a bug fix, better error handling, remove a TODO, de-duplicated code, and code size reduction! |
dpgeorge
reviewed
Sep 6, 2024
7eb2fbb to
a6cae89
Compare
dpgeorge
approved these changes
Sep 10, 2024
a6cae89 to
c893f2e
Compare
|
Rebased, fixed inconsistent commit message prefixes. 😁 |
A positive result here can result in eventual memory corruption as littlefs expects the result of a cache read/write function to be 0 or a negative integer for an error. Closes micropython#13046 This work was funded through GitHub Sponsors. Signed-off-by: Angus Gratton <angus@redyak.com.au>
- Code size saving as all of these functions are very similar. - Resolves the "TODO" of the plain read and write functions not propagating errors. An error in the underlying block device now causes VFatFs to return EIO, for example. This work was funded through GitHub Sponsors. Signed-off-by: Angus Gratton <angus@redyak.com.au>
c893f2e to
f4ab9d9
Compare
|
Thanks, now merged. |
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.
Summary
Two fixes, currently in two commits as the second one is more complex than the first:
mp_vfs_blockdev_read_ext/mp_vfs_blockdev_write_extto be 0 or a negative integer. Returning positive integers here confuses littlefs (see lfs1.c:66 for example). Adds a unit test for an invalid block device of this kind. Closes heap-buffer-overflow: from integer overflow at mp_stream_rw #13046.EDIT: There was previously a semi-related fix for the "stream" part of the linked issue in this PR, but it was impossible to test and only necessary if the underlying stream (in C) violated its API contract. The block device fix in this PR already prevents this from happening in the one known case.
Trade-offs and Alternatives
There is a risk that an old implementation of BlockDev readblocks/writeblocks somewhere returns a non-zero integer or other value on success and will now fail as the result is checked. I think this is unlikely though, as the same function is called for the
_extvariants and those results have always been checked.Testing
Ran the
vfstests on both unix port and rp2 port.This work was funded through GitHub Sponsors.