fix: false uninitialized read warning

[elupus]

Add asserts on file system reads to make sure
no positive values are returned, which would
make assumptions on error checks invalid.

This fixes clang tidy warnings on uninitialized
reads in uses of lfs_dir_get where only negative
returns are considered errors.

[geky-bot]

Tests passed ✓, Code: 17112 B (+0.0%), Stack: 1448 B (+0.0%), Structs: 812 B (+0.0%)

	Code	Stack	Structs		Coverage
Default	17112 B (+0.0%)	1448 B (+0.0%)	812 B (+0.0%)	Lines	2437/2597 lines (-0.0%)
Readonly	6230 B (+0.0%)	448 B (+0.0%)	812 B (+0.0%)	Branches	1285/1620 branches (-0.1%)
Threadsafe	17964 B (+0.0%)	1448 B (+0.0%)	820 B (+0.0%)		Benchmarks
Multiversion	17184 B (+0.0%)	1448 B (+0.0%)	816 B (+0.0%)	Readed	29369693876 B (+0.0%)
Migrate	18776 B (+0.0%)	1752 B (+0.0%)	816 B (+0.0%)	Proged	1482874766 B (+0.0%)
Error-asserts	17936 B (+0.1%)	1440 B (+0.0%)	812 B (+0.0%)	Erased	1568888832 B (+0.0%)

[BenBE]

Just looking at the changed code in this PR, there are the following likely scenarios:

1. lfs_bd_read may on some code paths not properly return a value --> That would be a grave bug in that function
2. clang-tidy is buggy -> Very low priority
3. There's some other hidden issue, that triggers this -> If that's the case, a more thorough architectural fix would be needed.

Given there's a similar issue I reported a while back in #904, with potentially uninitialized memory, and the overall design in the core LittleFS codebase not ensuring properly initialized memory (at least as far as I had a look at for LittleFS v2), there may be further similar issues hidden.

I this particular case I'm strongly leaning towards option 1 (buggy lfs_bd_read with potential for uninitialized return value).

If that's the case, the PR as presented is insufficient and only fixes symptoms, not bugs.

[elupus]

1. The problem is lfs_bd_read is actually implemented as a callout to user supplied reading function. There is one assert in that file already for this case, but not in all paths:

Ie it's checked here:

littlefs/lfs.c

Line 118 in 8e251dd

LFS_ASSERT(err <= 0);

but not here:

littlefs/lfs.c

Line 96 in 8e251dd

if (err) {

So I would have hoped clang-tidy would have been happy with fixing that extra assert inside lfs_bd_read, but it was not oddly. Seems it could not follow that assertion.

One better option could have been to change all cases to if (err < 0) to make sure no positive values ever slip out since they brake assumptions and asserts can be turned off.

[BenBE]

Wouldn't you need to put an assert in line 96 (i.e. after the lfs->cfg->read) too?

This looks to me like a case of inconsistent and leaky API enforcement …

[elupus]

Wouldn't you need to put an assert in line 96 (i.e. after the lfs->cfg->read) too?

Yea probably should add that to this patch. But annoyingly was not enough.

[geky-bot]

Tests passed ✓, Code: 17112 B (+0.0%), Stack: 1448 B (+0.0%), Structs: 812 B (+0.0%)

	Code	Stack	Structs		Coverage
Default	17112 B (+0.0%)	1448 B (+0.0%)	812 B (+0.0%)	Lines	2438/2598 lines (-0.0%)
Readonly	6230 B (+0.0%)	448 B (+0.0%)	812 B (+0.0%)	Branches	1286/1622 branches (-0.1%)
Threadsafe	17964 B (+0.0%)	1448 B (+0.0%)	820 B (+0.0%)		Benchmarks
Multiversion	17184 B (+0.0%)	1448 B (+0.0%)	816 B (+0.0%)	Readed	29369693876 B (+0.0%)
Migrate	18776 B (+0.0%)	1752 B (+0.0%)	816 B (+0.0%)	Proged	1482874766 B (+0.0%)
Error-asserts	17940 B (+0.1%)	1440 B (+0.0%)	812 B (+0.0%)	Erased	1568888832 B (+0.0%)

[geky]

The problem is lfs_bd_read is actually implemented as a callout to user supplied reading function. There is one assert in that file already for this case, but not in all paths:

That would be a bug on our end, thanks for creating a PR!

So I would have hoped clang-tidy would have been happy with fixing that extra assert inside lfs_bd_read, but it was not oddly. Seems it could not follow that assertion.

I think that makes sense if clang-tidy wants to protect against future implementations of lfs_bd_read. I've adopted a similar pattern in littlefs3 for functions that create sign-unioned errors from non-unioned errors.

Frustratingly, I couldn't come up with an LFS_ASSERT definition that informs GCC about these assumptions without dragging in extra code, so there's a few unnecessary initializations to keep GCC happy. It's neat that clang-tidy can understand these asserts though.

One better option could have been to change all cases to if (err < 0) to make sure no positive values ever slip out since they brake assumptions and asserts can be turned off.

I've toyed around with this before, but it created more issues than it solved. If a positive error is returned, the logic will fall through to the rest of the code with garbage state, which creates problems/warnings.

[geky]

Given there's a similar issue I reported a while back in #904, with potentially uninitialized memory, and the overall design in the core LittleFS codebase not ensuring properly initialized memory (at least as far as I had a look at for LittleFS v2), there may be further similar issues hidden.

It's worth noting rambd is a special case where we're treating memory as storage. We shouldn't be relying on uninitialized memory in the core code, but uninitialized storage is something littlefs needs to be prepared to handle. (Even if we erased the entire disk during format, storage after powerloss could be considered uninitialized.)

If you need reproducibility you should probably use emubd (coincidentally I'm working on a lighter-weight variant of emubd for benchmarking that looks like rambd + erase emulation + read/prog/erase counters).

[geky]

@elupus, sorry about the delay on these, I should have brought them in earlier before becoming distracted out-of-project. Bringing them in now.

Thanks again for the PRs!