Skip to content

Conversation

@geky
Copy link
Member

@geky geky commented Jan 17, 2024

Wild this hasn't been caught until now.

Because the exact ordering of the comparison in lfs_bd_cmp is a bit ambiguous, lfs_dir_find_match returned the wrong result when filenames were equal, and only differed in length.

For example:

  • cmp("a", "aa") should be LFS_CMP_LT
  • cmp("aaa", "aa") should be LFS_CMP_GT

We're quite lucky that none of the littlefs internals currently depend on the sorted order, otherwise we'd probably be stuck with this weird ordering for backwards compatibility reasons...

Fixed, and added some test cases over directory ordering to prevent regression in the future.

Found by @andriyndev
Related issue #923

Wild this hasn't been caught until now.

Because the exact ordering of the comparison in lfs_bd_cmp is a bit
ambiguous, lfs_dir_find_match returned the wrong result when filenames
were equal, and only differed in length.

For example:

  - cmp("a", "aa") should be LFS_CMP_LT
  - cmp("aaa", "aa") should be LFS_CMP_GT

We're quite lucky that none of the littlefs internals currently depend
on the sorted order, otherwise we'd probably be stuck with this weird
ordering for backwards compatibility reasons...

Fixed, and added some test cases over directory ordering to prevent
regression in the future.

Found by andriyndev
@geky-bot
Copy link
Collaborator

Tests passed ✓, Code: 16828 B (+0.0%), Stack: 1448 B (+0.0%), Structs: 800 B (+0.0%)
Code Stack Structs Coverage
Default 16828 B (+0.0%) 1448 B (+0.0%) 800 B (+0.0%) Lines 2357/2533 lines (+0.0%)
Readonly 6130 B (+0.0%) 448 B (+0.0%) 800 B (+0.0%) Branches 1202/1528 branches (+0.0%)
Threadsafe 17696 B (+0.0%) 1448 B (+0.0%) 808 B (+0.0%) Benchmarks
Multiversion 16892 B (+0.0%) 1448 B (+0.0%) 804 B (+0.0%) Readed 29369693876 B (+0.0%)
Migrate 18508 B (+0.0%) 1752 B (+0.0%) 804 B (+0.0%) Proged 1482874766 B (+0.0%)
Error-asserts 17484 B (+0.0%) 1440 B (+0.0%) 800 B (+0.0%) Erased 1568888832 B (+0.0%)

@geky geky added this to the v2.9 milestone Jan 17, 2024
@geky geky added needs minor version new functionality only allowed in minor versions needs test all fixes need test coverage to prevent regression and removed next minor labels Jan 19, 2024
@geky geky removed this from the v2.9 milestone Jan 19, 2024
@geky geky marked this pull request as draft January 19, 2024 05:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs minor version new functionality only allowed in minor versions needs test all fixes need test coverage to prevent regression on-disk minor postponed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants