Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix DivideByZero exception when filesystem is completely full. #966

Merged

Conversation

BrianPugh
Copy link
Contributor

There were reports of a DivideByZero exception in esp_littlefs when the filesystem was completely full. @huming2207 was able to get a stacktrace, and it points to the logging line in this PR. We're still figuring out if this is the certainly the cause, but the current code looks like an overlooked bug from #912.

Might address joltwallet/esp_littlefs#183

Original original thread: https://esp32.com/viewtopic.php?f=13&t=39197&p=130708

@geky-bot
Copy link
Collaborator

Tests passed ✓, Code: 17032 B (+0.0%), Stack: 1432 B (+0.0%), Structs: 812 B (+0.0%)
Code Stack Structs Coverage
Default 17032 B (+0.0%) 1432 B (+0.0%) 812 B (+0.0%) Lines 2391/2571 lines (+0.0%)
Readonly 6190 B (+0.0%) 448 B (+0.0%) 812 B (+0.0%) Branches 1243/1582 branches (+0.0%)
Threadsafe 17896 B (+0.0%) 1432 B (+0.0%) 820 B (+0.0%) Benchmarks
Multiversion 17096 B (+0.0%) 1432 B (+0.0%) 816 B (+0.0%) Readed 29369693876 B (+0.0%)
Migrate 18728 B (+0.0%) 1736 B (+0.0%) 816 B (+0.0%) Proged 1482874766 B (+0.0%)
Error-asserts 17712 B (+0.0%) 1424 B (+0.0%) 812 B (+0.0%) Erased 1568888832 B (+0.0%)

@geky
Copy link
Member

geky commented Apr 17, 2024

Hi @BrianPugh, thanks for this.

Yeah, it looks like a simple mistake. I think what happened is I rebased #912 from a branch started before #866, and the lfs->cfg->block_size -> lfs->block_size change technically wasn't a merge conflict.

Will merge, though we should probably have a test to prevent regression.

@geky geky added needs test all fixes need test coverage to prevent regression next patch labels Apr 17, 2024
@geky
Copy link
Member

geky commented Apr 17, 2024

A quick grep shows lfs->cfg->block_size is only used in lfs_mount_ and lfs_migrate_, so hopefully this is the only case of this.

@huming2207
Copy link

@geky as per our quick tests, by changing the lfs->cfg->block_size to lfs->block_size will fix the crash for us.

The block allocator is an area where inferred block counts (when
cfg.block_count=0) are more likely to cause problems.

As is shown by the recent divide-by-zero-exhaustion issue.
- Prefer "defaults to blablabla when zero" to hint that this is the
  default state when both explicitly set to zero and implicitly set to
  zero thanks to C's initializers.

- Prefer "disk" when referencing something stored "on disk". Other terms
  can quickly get ambiguous. Except maybe "block device"...
@geky geky removed the needs test all fixes need test coverage to prevent regression label Apr 17, 2024
@geky
Copy link
Member

geky commented Apr 17, 2024

I've gone ahead and extended test_alloc to test with an inferred block count. It's a bit hacky since our test runner doesn't really expect config to be mutable, but it should work.

I think this is worthwhile as the block allocator is one of subsystems more likely to run into block_count-related edge cases.

I also tweaked the struct lfs_config comments for consistency and to avoid on-disk/off-disk ambiguity. But it's good to mention the inferring behavior of block_count=0.

Let me know if anything looks concerning.

@geky
Copy link
Member

geky commented Apr 17, 2024

A bit humorous this only affects an LFS_DEBUG statement.

Many users turn these off for code savings so that likely made this issue less common.

@geky-bot
Copy link
Collaborator

Tests passed ✓, Code: 17032 B (+0.0%), Stack: 1432 B (+0.0%), Structs: 812 B (+0.0%)
Code Stack Structs Coverage
Default 17032 B (+0.0%) 1432 B (+0.0%) 812 B (+0.0%) Lines 2391/2571 lines (+0.0%)
Readonly 6190 B (+0.0%) 448 B (+0.0%) 812 B (+0.0%) Branches 1243/1582 branches (+0.0%)
Threadsafe 17896 B (+0.0%) 1432 B (+0.0%) 820 B (+0.0%) Benchmarks
Multiversion 17096 B (+0.0%) 1432 B (+0.0%) 816 B (+0.0%) Readed 29369693876 B (+0.0%)
Migrate 18728 B (+0.0%) 1736 B (+0.0%) 816 B (+0.0%) Proged 1482874766 B (+0.0%)
Error-asserts 17712 B (+0.0%) 1424 B (+0.0%) 812 B (+0.0%) Erased 1568888832 B (+0.0%)

@BrianPugh
Copy link
Contributor Author

all looks good to me!

@geky geky changed the base branch from master to devel April 17, 2024 17:37
@geky geky merged commit 68d28b5 into littlefs-project:devel Apr 17, 2024
93 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants