Skip to content

Commit

Permalink
Fixed name ordering when names only differ in length
Browse files Browse the repository at this point in the history
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
  • Loading branch information
geky committed Jan 17, 2024
1 parent 3513ff1 commit 52df70c
Show file tree
Hide file tree
Showing 2 changed files with 101 additions and 2 deletions.
4 changes: 2 additions & 2 deletions lfs.c
Original file line number Diff line number Diff line change
Expand Up @@ -1430,8 +1430,8 @@ static int lfs_dir_find_match(void *data,
}

// only equal if our size is still the same
if (name->size != lfs_tag_size(tag)) {
return (name->size < lfs_tag_size(tag)) ? LFS_CMP_LT : LFS_CMP_GT;
if (lfs_tag_size(tag) != name->size) {
return (lfs_tag_size(tag) < name->size) ? LFS_CMP_LT : LFS_CMP_GT;
}

// found a match!
Expand Down
99 changes: 99 additions & 0 deletions tests/test_dirs.toml
Original file line number Diff line number Diff line change
Expand Up @@ -717,6 +717,105 @@ code = '''
lfs_unmount(&lfs) => 0;
'''

# littlefs should keep directories in lexicographic order
[cases.test_dirs_ordering]
# ORDER=0 => inorder
# ORDER=1 => reversed
# ORDER=2 => random
defines.ORDER = [0, 1, 2]
code = '''
lfs_t lfs;
lfs_format(&lfs, cfg) => 0;
lfs_mount(&lfs, cfg) => 0;
if (ORDER == 0) {
lfs_mkdir(&lfs, "a") => 0;
lfs_mkdir(&lfs, "b") => 0;
lfs_mkdir(&lfs, "c") => 0;
} else if (ORDER == 1) {
lfs_mkdir(&lfs, "c") => 0;
lfs_mkdir(&lfs, "b") => 0;
lfs_mkdir(&lfs, "a") => 0;
} else if (ORDER == 2) {
// "random"
lfs_mkdir(&lfs, "a") => 0;
lfs_mkdir(&lfs, "c") => 0;
lfs_mkdir(&lfs, "b") => 0;
}

// check the order
lfs_dir_t dir;
lfs_dir_open(&lfs, &dir, "/") => 0;
struct lfs_info info;
lfs_dir_read(&lfs, &dir, &info) => 1;
assert(info.type == LFS_TYPE_DIR);
assert(strcmp(info.name, ".") == 0);
lfs_dir_read(&lfs, &dir, &info) => 1;
assert(info.type == LFS_TYPE_DIR);
assert(strcmp(info.name, "..") == 0);
lfs_dir_read(&lfs, &dir, &info) => 1;
assert(info.type == LFS_TYPE_DIR);
assert(strcmp(info.name, "a") == 0);
lfs_dir_read(&lfs, &dir, &info) => 1;
assert(info.type == LFS_TYPE_DIR);
assert(strcmp(info.name, "b") == 0);
lfs_dir_read(&lfs, &dir, &info) => 1;
assert(info.type == LFS_TYPE_DIR);
assert(strcmp(info.name, "c") == 0);
lfs_dir_read(&lfs, &dir, &info) => 0;
lfs_dir_close(&lfs, &dir) => 0;

lfs_unmount(&lfs) => 0;
'''

[cases.test_dirs_ordering_length]
# ORDER=0 => inorder
# ORDER=1 => reversed
# ORDER=2 => random
defines.ORDER = [0, 1, 2]
code = '''
lfs_t lfs;
lfs_format(&lfs, cfg) => 0;
lfs_mount(&lfs, cfg) => 0;
if (ORDER == 0) {
lfs_mkdir(&lfs, "a") => 0;
lfs_mkdir(&lfs, "aa") => 0;
lfs_mkdir(&lfs, "aaa") => 0;
} else if (ORDER == 1) {
lfs_mkdir(&lfs, "aaa") => 0;
lfs_mkdir(&lfs, "aa") => 0;
lfs_mkdir(&lfs, "a") => 0;
} else if (ORDER == 2) {
// "random"
lfs_mkdir(&lfs, "a") => 0;
lfs_mkdir(&lfs, "aaa") => 0;
lfs_mkdir(&lfs, "aa") => 0;
}

// check the order
lfs_dir_t dir;
lfs_dir_open(&lfs, &dir, "/") => 0;
struct lfs_info info;
lfs_dir_read(&lfs, &dir, &info) => 1;
assert(info.type == LFS_TYPE_DIR);
assert(strcmp(info.name, ".") == 0);
lfs_dir_read(&lfs, &dir, &info) => 1;
assert(info.type == LFS_TYPE_DIR);
assert(strcmp(info.name, "..") == 0);
lfs_dir_read(&lfs, &dir, &info) => 1;
assert(info.type == LFS_TYPE_DIR);
assert(strcmp(info.name, "a") == 0);
lfs_dir_read(&lfs, &dir, &info) => 1;
assert(info.type == LFS_TYPE_DIR);
assert(strcmp(info.name, "aa") == 0);
lfs_dir_read(&lfs, &dir, &info) => 1;
assert(info.type == LFS_TYPE_DIR);
assert(strcmp(info.name, "aaa") == 0);
lfs_dir_read(&lfs, &dir, &info) => 0;
lfs_dir_close(&lfs, &dir) => 0;

lfs_unmount(&lfs) => 0;
'''

[cases.test_dirs_other_errors]
code = '''
lfs_t lfs;
Expand Down

0 comments on commit 52df70c

Please sign in to comment.