From 2ce656768392aaf0461cebefbfb7a6306c0b2625 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Sat, 29 Jul 2023 01:21:49 -0500 Subject: [PATCH] Found+fixed a bug where arbitrary dir seeks can return unrelated entries 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. --- lfs.c | 32 ++++++++++++++++++-------------- lfs.h | 1 + tests/t5_dirs.toml | 14 ++++++++++++++ 3 files changed, 33 insertions(+), 14 deletions(-) diff --git a/lfs.c b/lfs.c index 80ab0bc6..d37e864a 100644 --- a/lfs.c +++ b/lfs.c @@ -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 diff --git a/lfs.h b/lfs.h index 7b331a35..7c60a8dd 100644 --- a/lfs.h +++ b/lfs.h @@ -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; diff --git a/tests/t5_dirs.toml b/tests/t5_dirs.toml index 5a0b8ce0..3848168a 100644 --- a/tests/t5_dirs.toml +++ b/tests/t5_dirs.toml @@ -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