Skip to content

extmod/vfs: Skip littlefs detection if mp_vfs_blockdev_read_ext() fails.#19306

Open
projectgus wants to merge 1 commit into
micropython:masterfrom
projectgus:bugfix/vfs_uninit_buf
Open

extmod/vfs: Skip littlefs detection if mp_vfs_blockdev_read_ext() fails.#19306
projectgus wants to merge 1 commit into
micropython:masterfrom
projectgus:bugfix/vfs_uninit_buf

Conversation

@projectgus

@projectgus projectgus commented Jun 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

I noticed in passing that buf can be read uninitialised while checking for littlefs, if mp_vfs_blockdev_read_ext fails.

Rather than initialise buf (expensive), have opted to check the return value of the read function.

This work was funded through GitHub Sponsors.

Testing

  • Flashed a TEENSY41 and an RPI_PICO board, verified the littlefs filesystem still mounted on boot.

Trade-offs and Alternatives

  • Could ignore this, as the chances of this function failing are very low and then the chances of the string littlefs2 ending up on the stack independently of the internal flash is additionally low (and the first match will return, so a stale copy should never end up being checked on the second iteration.)
  • Could zero buf, but as mentioned above, this is more expensive than checking the return value.
  • Small chance of regression if there's a port block driver which doesn't return 0 here for success, but as the littlefs2 read function also calls mp_vfs_blockdev_read_ext and returns the result to littlefs2 it seems like such a port block driver would already not work with littlefs.

Generative AI

I did not use generative AI tools when creating this PR.

@projectgus projectgus added the extmod Relates to extmod/ directory in source label Jun 4, 2026
@github-actions

github-actions Bot commented Jun 4, 2026 •
edited
Loading

Copy link
Copy Markdown

Code size report:

Reference:  esp32: Rename the sdkconfig.spiram_xxx config snippets. [7e55a73]
Comparison: extmod/vfs: Skip littlefs detection if mp_vfs_blockdev_read_ext() fails. [merge of 86f68f5]
  mpy-cross:    +0 +0.000% 
   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:   +16 +0.002% standard
      stm32:   -16 -0.004% PYBV10
      esp32:   -40 -0.002% ESP32_GENERIC
     mimxrt:   -16 -0.004% TEENSY40
        rp2:   -16 -0.002% RPI_PICO_W
       samd:   -16 -0.006% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:    +0 +0.000% VIRT_RV32

@codecov

codecov Bot commented Jun 4, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.48%. Comparing base (7e55a73) to head (86f68f5).

Additional details and impacted files 
@@           Coverage Diff           @@
##           master   #19306   +/-   ##
=======================================
  Coverage   98.47%   98.48%           
=======================================
  Files         176      176           
  Lines       22845    22846    +1     
=======================================
+ Hits        22497    22499    +2     
+ Misses        348      347    -1

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@dpgeorge

dpgeorge commented Jun 4, 2026

Copy link
Copy Markdown
Member

This looks very reasonable to me, to check the return value (regardless of anything else, that seems like the right thing to do).

Small chance of regression if there's a port block driver which doesn't return 0 here for success, but as the littlefs2 read function also calls mp_vfs_blockdev_read_ext and returns the result to littlefs2 it seems like such a port block driver would already not work with littlefs.

I very much doubt there's such a driver that this change would break. So I think it's OK.

But I do see that code coverage is failing. Is there a way to test this change? Maybe try to auto-mount a block device that fails on all reads?

@projectgus projectgus force-pushed the bugfix/vfs_uninit_buf branch from a9d7a75 to 4caffe3 Compare June 10, 2026 06:51
@projectgus
extmod/vfs: Skip littlefs detection if mp_vfs_blockdev_read_ext() fails.
86f68f5 
Otherwise 'buf' is read uninitialised.

Extends vfs_blockdev_invalid test to include the mount auto-detection path.

This work was funded through GitHub Sponsors.

Signed-off-by: Angus Gratton <angus@redyak.com.au>
@projectgus projectgus force-pushed the bugfix/vfs_uninit_buf branch from 4caffe3 to 86f68f5 Compare June 10, 2026 06:52
@projectgus

projectgus commented Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

Updated an existing vfs invalid block device to also cover mount failures, looks like this code path is now covered.

projectgus
projectgus commented Jun 10, 2026
View reviewed changes
Comment thread extmod/vfs.c
Comment on lines 203 to 207
return MP_OBJ_TYPE_GET_SLOT(&mp_fat_vfs_type, make_new)(&mp_fat_vfs_type, 1, 0, &bdev_obj);
#endif

// no filesystem found
mp_raise_OSError(MP_ENODEV);

@projectgus projectgus Jun 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something unexpected I noticed is that if FAT support is enabled, this code path will return EIO. If FAT support is not enabled, it will return ENODEV from a few lines down (and the new test would fail).

It's not a particularly big inconsistency so probably we can ignore it, but mentioning as it'd also be easy to change this function so it returns ENODEV for both filesystems or so it records the block read error code and returns that in the LFS2 case as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

extmod Relates to extmod/ directory in source

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@projectgus @dpgeorge