Github/littlefs
1
Fork 0
Code Actions Activity

Found+fixed a bug where arbitrary dir seeks can return unrelated entries

Browse Source 
It turned out our dir-read-idempotent test never created non-dstart
neighbors. This was a bit of a problem since we relied on dstart entries
to know when our dir read terminates. If we seek to an invalid position
(in theory undefined behavior, but easily possible with concurrent
modifications to the directory), we can end up reading an unrealted,
non-dstart entry, and incorrectly reporting that entry as in our current
dir.

This fix reintroduces the did into the lfsr_dir_t struct and uses the
did to determine end-of-dir. This adds some RAM cost, but is more
resilient to any seeks that overshoot the end of the directory.

Using did is also a stronger guarantee we will never accidentally report
unrelated entries as a part of the current directory.
This commit is contained in:
Christopher Haster
2023-07-29 01:21:49 -05:00
parent ee9cc185a1
commit 2ce6567683
3 changed files with 33 additions and 14 deletions
Download Patch File Download Diff File Expand all files Collapse all files
lfs.c
+18 -14
View File
@@ -7768,8 +7768,9 @@ int lfsr_dir_open(lfs_t *lfs, lfsr_dir_t *dir, const char *path) {
}
// read our did from the mdir, unless we're root
lfs_size_t did = 0;
if (err != LFS_ERR_INVAL) {
if (err == LFS_ERR_INVAL) {
dir->did = 0;
} else {
lfsr_data_t data;
int err = lfsr_mdir_lookup(lfs, &mdir, rid, LFSR_TAG_DID,
NULL, &data);
@@ -7777,7 +7778,7 @@ int lfsr_dir_open(lfs_t *lfs, lfsr_dir_t *dir, const char *path) {
return err;
}
lfs_ssize_t d = lfsr_data_readleb128(lfs, data, 0, &did);
lfs_ssize_t d = lfsr_data_readleb128(lfs, data, 0, &dir->did);
if (d < 0) {
return d;
}
@@ -7787,7 +7788,7 @@ int lfsr_dir_open(lfs_t *lfs, lfsr_dir_t *dir, const char *path) {
dir->pos = 0;
// lookup our dstart in the mtree
err = lfsr_mtree_dnamelookup(lfs, did, NULL, 0,
err = lfsr_mtree_dnamelookup(lfs, dir->did, NULL, 0,
&dir->mdir.mdir, &dir->mdir.rid, NULL, NULL);
if (err) {
LFS_ASSERT(err != LFS_ERR_NOENT);
@@ -7849,19 +7850,19 @@ int lfsr_dir_read(lfs_t *lfs, lfsr_dir_t *dir, struct lfs_info *info) {
return err;
}
// found another directory's dstart? we must be done
if (tag == LFSR_TAG_DSTART) {
return LFS_ERR_NOENT;
}
// get file type from the tag
info->type = lfsr_tag_filetype(tag);
// get file name from the name entry
lfs_ssize_t d = lfsr_data_readleb128(lfs, data, 0, &(uint32_t){0});
// get our did
lfs_size_t did;
lfs_ssize_t d = lfsr_data_readleb128(lfs, data, 0, &did);
if (d < 0) {
return d;
}
// did mismatch? we must be done
if (did != dir->did) {
return LFS_ERR_NOENT;
}
// get file name from the name entry
LFS_ASSERT(lfsr_data_size(data)-d <= LFS_NAME_MAX);
d = lfsr_data_read(lfs, data, d, info->name, LFS_NAME_MAX);
if (d < 0) {
@@ -7869,6 +7870,9 @@ int lfsr_dir_read(lfs_t *lfs, lfsr_dir_t *dir, struct lfs_info *info) {
}
info->name[d] = '\0';
// get file type from the tag
info->type = lfsr_tag_filetype(tag);
// TODO get size once we actually have regular files
// eagerly look up the next entry
lfs.h
+1
View File
@@ -424,6 +424,7 @@ typedef struct lfs_dir {
typedef struct lfsr_dir {
lfsr_openedmdir_t mdir;
lfs_size_t did;
lfs_ssize_t dstart_mid;
lfs_ssize_t dstart_rid;
lfs_off_t pos;
tests/t5_dirs.toml
+14
View File
@@ -5271,11 +5271,13 @@ code = '''
if (NEIGHBORS & 0x2) {
assert(lfs_crc32c(0, "a_IplRNrPH", 10) == 0x00000000);
lfsr_mkdir(&lfs, "a_IplRNrPH") => 0;
lfsr_mkdir(&lfs, "a_IplRNrPH/a_child") => 0;
}
if (NEIGHBORS & 0x1) {
assert(lfs_crc32c(0, "f_VtoMnwRH", 10) == 0xffffffff);
lfsr_mkdir(&lfs, "f_VtoMnwRH") => 0;
lfsr_mkdir(&lfs, "f_VtoMnwRH/f_child") => 0;
}
}
@@ -5364,11 +5366,13 @@ code = '''
if (NEIGHBORS & 0x2) {
assert(lfs_crc32c(0, "a_IplRNrPH", 10) == 0x00000000);
lfsr_mkdir(&lfs, "a_IplRNrPH") => 0;
lfsr_mkdir(&lfs, "a_IplRNrPH/a_child") => 0;
}
if (NEIGHBORS & 0x1) {
assert(lfs_crc32c(0, "f_VtoMnwRH", 10) == 0xffffffff);
lfsr_mkdir(&lfs, "f_VtoMnwRH") => 0;
lfsr_mkdir(&lfs, "f_VtoMnwRH/f_child") => 0;
}
}
@@ -5457,11 +5461,13 @@ code = '''
if (NEIGHBORS & 0x2) {
assert(lfs_crc32c(0, "a_IplRNrPH", 10) == 0x00000000);
lfsr_mkdir(&lfs, "a_IplRNrPH") => 0;
lfsr_mkdir(&lfs, "a_IplRNrPH/a_child") => 0;
}
if (NEIGHBORS & 0x1) {
assert(lfs_crc32c(0, "f_VtoMnwRH", 10) == 0xffffffff);
lfsr_mkdir(&lfs, "f_VtoMnwRH") => 0;
lfsr_mkdir(&lfs, "f_VtoMnwRH/f_child") => 0;
}
}
@@ -5562,11 +5568,13 @@ code = '''
if (NEIGHBORS & 0x2) {
assert(lfs_crc32c(0, "a_IplRNrPH", 10) == 0x00000000);
lfsr_mkdir(&lfs, "a_IplRNrPH") => 0;
lfsr_mkdir(&lfs, "a_IplRNrPH/a_child") => 0;
}
if (NEIGHBORS & 0x1) {
assert(lfs_crc32c(0, "f_VtoMnwRH", 10) == 0xffffffff);
lfsr_mkdir(&lfs, "f_VtoMnwRH") => 0;
lfsr_mkdir(&lfs, "f_VtoMnwRH/f_child") => 0;
}
}
@@ -5779,11 +5787,13 @@ code = '''
if (NEIGHBORS & 0x2) {
assert(lfs_crc32c(0, "a_IplRNrPH", 10) == 0x00000000);
lfsr_mkdir(&lfs, "a_IplRNrPH") => 0;
lfsr_mkdir(&lfs, "a_IplRNrPH/a_child") => 0;
}
if (NEIGHBORS & 0x1) {
assert(lfs_crc32c(0, "f_VtoMnwRH", 10) == 0xffffffff);
lfsr_mkdir(&lfs, "f_VtoMnwRH") => 0;
lfsr_mkdir(&lfs, "f_VtoMnwRH/f_child") => 0;
}
}
@@ -5892,11 +5902,13 @@ code = '''
if (NEIGHBORS & 0x2) {
assert(lfs_crc32c(0, "a_IplRNrPH", 10) == 0x00000000);
lfsr_mkdir(&lfs, "a_IplRNrPH") => 0;
lfsr_mkdir(&lfs, "a_IplRNrPH/a_child") => 0;
}
if (NEIGHBORS & 0x1) {
assert(lfs_crc32c(0, "f_VtoMnwRH", 10) == 0xffffffff);
lfsr_mkdir(&lfs, "f_VtoMnwRH") => 0;
lfsr_mkdir(&lfs, "f_VtoMnwRH/f_child") => 0;
}
}
@@ -6002,11 +6014,13 @@ code = '''
if (NEIGHBORS & 0x2) {
assert(lfs_crc32c(0, "a_IplRNrPH", 10) == 0x00000000);
lfsr_mkdir(&lfs, "a_IplRNrPH") => 0;
lfsr_mkdir(&lfs, "a_IplRNrPH/a_child") => 0;
}
if (NEIGHBORS & 0x1) {
assert(lfs_crc32c(0, "f_VtoMnwRH", 10) == 0xffffffff);
lfsr_mkdir(&lfs, "f_VtoMnwRH") => 0;
lfsr_mkdir(&lfs, "f_VtoMnwRH/f_child") => 0;
}
// create our directories