Fixed dir iteration being broken by concurrent removes

When removing a file, we mark all open handles as "removed" (
pair={-1,-1}) to avoid trying to later read metadata that no longer
exists. Unfortunately, this also includes open dir handles that happen
to be pointing at the removed file, causing them to return
LFS_ERR_CORRUPT on the next read.

The good news is this is _not_ actual filesystem corruption, only a
logic error in lfs_dir_read.

We actually already have logic in place to nudge the dir to the next id,
but it was unreachable with the existing logic. I suspect this worked at
one point but was broken during a refactor due to lack of testing.

---

Fortunately, all we need to do is _not_ clobber the handle if the
internal type is a dir. Then the dir-nudging logic can correctly take
over.

I've also added test_dirs_remove_read to test this and prevent another
regression, adapted from tests provided by tpwrules that identified the
original bug.

Found by tpwrules

lfs.c

@@ -2369,7 +2369,8 @@ fixmlist:;
             if (d->m.pair != pair) {
                 for (int i = 0; i < attrcount; i++) {
                     if (lfs_tag_type3(attrs[i].tag) == LFS_TYPE_DELETE &&
-                            d->id == lfs_tag_id(attrs[i].tag)) {
+                            d->id == lfs_tag_id(attrs[i].tag) &&
+                            d->type != LFS_TYPE_DIR) {
                         d->m.pair[0] = LFS_BLOCK_NULL;
                         d->m.pair[1] = LFS_BLOCK_NULL;
                     } else if (lfs_tag_type3(attrs[i].tag) == LFS_TYPE_DELETE &&
@@ -3725,13 +3726,8 @@ static lfs_soff_t lfs_file_seek_(lfs_t *lfs, lfs_file_t *file,
 
     // if we're only reading and our new offset is still in the file's cache
     // we can avoid flushing and needing to reread the data
-    if (
-#ifndef LFS_READONLY
-        !(file->flags & LFS_F_WRITING)
-#else
-        true
-#endif
-            ) {
+    if ((file->flags & LFS_F_READING)
+            && file->off != lfs->cfg->block_size) {
         int oindex = lfs_ctz_index(lfs, &(lfs_off_t){file->pos});
         lfs_off_t noff = npos;
         int nindex = lfs_ctz_index(lfs, &noff);
@@ -4217,6 +4213,14 @@ static int lfs_init(lfs_t *lfs, const struct lfs_config *cfg) {
     // which littlefs currently does not support
     LFS_ASSERT((bool)0x80000000);
 
+    // check that the required io functions are provided
+    LFS_ASSERT(lfs->cfg->read != NULL);
+#ifndef LFS_READONLY
+    LFS_ASSERT(lfs->cfg->prog != NULL);
+    LFS_ASSERT(lfs->cfg->erase != NULL);
+    LFS_ASSERT(lfs->cfg->sync != NULL);
+#endif
+
     // validate that the lfs-cfg sizes were initiated properly before
     // performing any arithmetic logics with them
     LFS_ASSERT(lfs->cfg->read_size != 0);

lfs.h

@@ -21,7 +21,7 @@ extern "C"
 // Software library version
 // Major (top-nibble), incremented on backwards incompatible changes
 // Minor (bottom-nibble), incremented on feature additions
-#define LFS_VERSION 0x00020009
+#define LFS_VERSION 0x0002000a
 #define LFS_VERSION_MAJOR (0xffff & (LFS_VERSION >> 16))
 #define LFS_VERSION_MINOR (0xffff & (LFS_VERSION >>  0))
 

scripts/prettyasserts.py

@@ -86,6 +86,13 @@ def write_header(f, limit=LIMIT):
     f.writeln("}")
     f.writeln()
     f.writeln("__attribute__((unused))")
+    f.writeln("static void __pretty_assert_print_ptr(")
+    f.writeln("        const void *v, size_t size) {")
+    f.writeln("    (void)size;")
+    f.writeln("    printf(\"%p\", v);")
+    f.writeln("}")
+    f.writeln()
+    f.writeln("__attribute__((unused))")
     f.writeln("static void __pretty_assert_print_mem(")
     f.writeln("        const void *v, size_t size) {")
     f.writeln("    const uint8_t *v_ = v;")
@@ -183,6 +190,23 @@ def write_header(f, limit=LIMIT):
         f.writeln("                _rh, strlen(_rh)); \\")
         f.writeln("    } \\")
         f.writeln("} while (0)")
+    for op, cmp in sorted(CMP.items()):
+        # Only EQ and NE are supported when compared to NULL.
+        if cmp not in ['eq', 'ne']:
+            continue
+        f.writeln("#define __PRETTY_ASSERT_PTR_%s(lh, rh) do { \\"
+            % cmp.upper())
+        f.writeln("    const void *_lh = (const void*)(uintptr_t)lh; \\")
+        f.writeln("    const void *_rh = (const void*)(uintptr_t)rh; \\")
+        f.writeln("    if (!(_lh %s _rh)) { \\" % op)
+        f.writeln("        __pretty_assert_fail( \\")
+        f.writeln("                __FILE__, __LINE__, \\")
+        f.writeln("                __pretty_assert_print_ptr, \"%s\", \\"
+            % cmp)
+        f.writeln("                (const void*){_lh}, 0, \\")
+        f.writeln("                (const void*){_rh}, 0); \\")
+        f.writeln("    } \\")
+        f.writeln("} while (0)")
     f.writeln()
     f.writeln()
 
@@ -301,6 +325,8 @@ def p_assert(p):
         cmp = p.expect('cmp') ; p.accept('ws')
         rh = p_expr(p) ; p.accept('ws')
         p.expect(')')
+        if rh == 'NULL' or lh == 'NULL':
+            return mkassert('ptr', CMP[cmp], lh, rh)
         return mkassert('int', CMP[cmp], lh, rh)
     except ParseFailure:
         p.pop(state)

tests/test_dirs.toml

@@ -725,6 +725,82 @@ code = '''
     lfs_unmount(&lfs) => 0;
 '''
 
+[cases.test_dirs_remove_read]
+defines.N = 10
+if = 'N < BLOCK_COUNT/2'
+code = '''
+    lfs_t lfs;
+    lfs_format(&lfs, cfg) => 0;
+    lfs_mount(&lfs, cfg) => 0;
+    lfs_mkdir(&lfs, "prickly-pear") => 0;
+    for (int i = 0; i < N; i++) {
+        char path[1024];
+        sprintf(path, "prickly-pear/cactus%03d", i);
+        lfs_mkdir(&lfs, path) => 0;
+    }
+    lfs_dir_t dir;
+    lfs_dir_open(&lfs, &dir, "prickly-pear") => 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);
+    for (int i = 0; i < N; i++) {
+        char path[1024];
+        sprintf(path, "cactus%03d", i);
+        lfs_dir_read(&lfs, &dir, &info) => 1;
+        assert(info.type == LFS_TYPE_DIR);
+        assert(strcmp(info.name, path) == 0);
+    }
+    lfs_dir_read(&lfs, &dir, &info) => 0;
+    lfs_dir_close(&lfs, &dir) => 0;
+    lfs_unmount(&lfs);
+
+    for (lfs_size_t k = 0; k < N; k++) {
+        for (lfs_size_t j = 0; j < N; j++) {
+            lfs_mount(&lfs, cfg) => 0;
+            lfs_dir_open(&lfs, &dir, "prickly-pear") => 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, "..") == 0);
+            // iterate over dirs < j
+            for (unsigned i = 0; i < j; i++) {
+                char path[1024];
+                sprintf(path, "cactus%03d", i);
+                lfs_dir_read(&lfs, &dir, &info) => 1;
+                assert(info.type == LFS_TYPE_DIR);
+                assert(strcmp(info.name, path) == 0);
+            }
+
+            // remove k while iterating
+            char path[1024];
+            sprintf(path, "prickly-pear/cactus%03d", k);
+            lfs_remove(&lfs, path) => 0;
+
+            // iterate over dirs >= j
+            for (unsigned i = j; i < ((k >= j) ? N-1 : N); i++) {
+                char path[1024];
+                sprintf(path, "cactus%03d", (k >= j && i >= k) ? i+1 : i);
+                lfs_dir_read(&lfs, &dir, &info) => 1;
+                assert(info.type == LFS_TYPE_DIR);
+                assert(strcmp(info.name, path) == 0);
+            }
+            lfs_dir_read(&lfs, &dir, &info) => 0;
+            lfs_dir_close(&lfs, &dir) => 0;
+
+            // recreate k
+            sprintf(path, "prickly-pear/cactus%03d", k);
+            lfs_mkdir(&lfs, path) => 0;
+            lfs_unmount(&lfs) => 0;
+        }
+    }
+'''
+
 [cases.test_dirs_other_errors]
 code = '''
     lfs_t lfs;

tests/test_seek.toml

@@ -137,6 +137,130 @@ code = '''
     lfs_unmount(&lfs) => 0;
 '''
 
+# boundary seek and reads
+[cases.test_seek_boundary_read]
+defines.COUNT = 132
+code = '''
+    lfs_t lfs;
+    lfs_format(&lfs, cfg) => 0;
+    lfs_mount(&lfs, cfg) => 0;
+    lfs_file_t file;
+    lfs_file_open(&lfs, &file, "kitty",
+            LFS_O_WRONLY | LFS_O_CREAT | LFS_O_APPEND) => 0;
+    size_t size = strlen("kittycatcat");
+    uint8_t buffer[1024];
+    memcpy(buffer, "kittycatcat", size);
+    for (int j = 0; j < COUNT; j++) {
+        lfs_file_write(&lfs, &file, buffer, size);
+    }
+    lfs_file_close(&lfs, &file) => 0;
+    lfs_unmount(&lfs) => 0;
+
+    lfs_mount(&lfs, cfg) => 0;
+    lfs_file_open(&lfs, &file, "kitty", LFS_O_RDONLY) => 0;
+
+    size = strlen("kittycatcat");
+    const lfs_soff_t offsets[] = {
+        512,
+        1024-4,
+        512+1,
+        1024-4+1,
+        512-1,
+        1024-4-1,
+
+        512-strlen("kittycatcat"),
+        1024-4-strlen("kittycatcat"),
+        512-strlen("kittycatcat")+1,
+        1024-4-strlen("kittycatcat")+1,
+        512-strlen("kittycatcat")-1,
+        1024-4-strlen("kittycatcat")-1,
+
+        strlen("kittycatcat")*(COUNT-2)-1,
+    };
+
+    for (unsigned i = 0; i < sizeof(offsets) / sizeof(offsets[0]); i++) {
+        lfs_soff_t off = offsets[i];
+        // read @ offset
+        lfs_file_seek(&lfs, &file, off, LFS_SEEK_SET) => off;
+        lfs_file_read(&lfs, &file, buffer, size) => size;
+        memcmp(buffer,
+                &"kittycatcatkittycatcat"[off % strlen("kittycatcat")],
+                size) => 0;
+        // read after
+        lfs_file_seek(&lfs, &file, off+strlen("kittycatcat")+1, LFS_SEEK_SET)
+                => off+strlen("kittycatcat")+1;
+        lfs_file_read(&lfs, &file, buffer, size) => size;
+        memcmp(buffer,
+                &"kittycatcatkittycatcat"[(off+1) % strlen("kittycatcat")],
+                size) => 0;
+        // read before
+        lfs_file_seek(&lfs, &file, off-strlen("kittycatcat")-1, LFS_SEEK_SET)
+                => off-strlen("kittycatcat")-1;
+        lfs_file_read(&lfs, &file, buffer, size) => size;
+        memcmp(buffer,
+                &"kittycatcatkittycatcat"[(off-1) % strlen("kittycatcat")],
+                size) => 0;
+
+        // read @ 0
+        lfs_file_seek(&lfs, &file, 0, LFS_SEEK_SET) => 0;
+        lfs_file_read(&lfs, &file, buffer, size) => size;
+        memcmp(buffer, "kittycatcat", size) => 0;
+
+        // read @ offset
+        lfs_file_seek(&lfs, &file, off, LFS_SEEK_SET) => off;
+        lfs_file_read(&lfs, &file, buffer, size) => size;
+        memcmp(buffer,
+                &"kittycatcatkittycatcat"[off % strlen("kittycatcat")],
+                size) => 0;
+        // read after
+        lfs_file_seek(&lfs, &file, off+strlen("kittycatcat")+1, LFS_SEEK_SET)
+                => off+strlen("kittycatcat")+1;
+        lfs_file_read(&lfs, &file, buffer, size) => size;
+        memcmp(buffer,
+                &"kittycatcatkittycatcat"[(off+1) % strlen("kittycatcat")],
+                size) => 0;
+        // read before
+        lfs_file_seek(&lfs, &file, off-strlen("kittycatcat")-1, LFS_SEEK_SET)
+                => off-strlen("kittycatcat")-1;
+        lfs_file_read(&lfs, &file, buffer, size) => size;
+        memcmp(buffer,
+                &"kittycatcatkittycatcat"[(off-1) % strlen("kittycatcat")],
+                size) => 0;
+
+        // sync
+        lfs_file_sync(&lfs, &file) => 0;
+
+        // read @ 0
+        lfs_file_seek(&lfs, &file, 0, LFS_SEEK_SET) => 0;
+        lfs_file_read(&lfs, &file, buffer, size) => size;
+        memcmp(buffer, "kittycatcat", size) => 0;
+
+        // read @ offset
+        lfs_file_seek(&lfs, &file, off, LFS_SEEK_SET) => off;
+        lfs_file_read(&lfs, &file, buffer, size) => size;
+        memcmp(buffer,
+                &"kittycatcatkittycatcat"[off % strlen("kittycatcat")],
+                size) => 0;
+        // read after
+        lfs_file_seek(&lfs, &file, off+strlen("kittycatcat")+1, LFS_SEEK_SET)
+                => off+strlen("kittycatcat")+1;
+        lfs_file_read(&lfs, &file, buffer, size) => size;
+        memcmp(buffer,
+                &"kittycatcatkittycatcat"[(off+1) % strlen("kittycatcat")],
+                size) => 0;
+        // read before
+        lfs_file_seek(&lfs, &file, off-strlen("kittycatcat")-1, LFS_SEEK_SET)
+                => off-strlen("kittycatcat")-1;
+        lfs_file_read(&lfs, &file, buffer, size) => size;
+        memcmp(buffer,
+                &"kittycatcatkittycatcat"[(off-1) % strlen("kittycatcat")],
+                size) => 0;
+    }
+
+    lfs_file_close(&lfs, &file) => 0;
+    lfs_unmount(&lfs) => 0;
+'''
+
 # boundary seek and writes
 [cases.test_seek_boundary_write]
 defines.COUNT = 132
@@ -160,31 +284,54 @@ code = '''
     lfs_file_open(&lfs, &file, "kitty", LFS_O_RDWR) => 0;
 
     size = strlen("hedgehoghog");
-    const lfs_soff_t offsets[] = {512, 1020, 513, 1021, 511, 1019, 1441};
+    const lfs_soff_t offsets[] = {
+        512,
+        1024-4,
+        512+1,
+        1024-4+1,
+        512-1,
+        1024-4-1,
+
+        512-strlen("kittycatcat"),
+        1024-4-strlen("kittycatcat"),
+        512-strlen("kittycatcat")+1,
+        1024-4-strlen("kittycatcat")+1,
+        512-strlen("kittycatcat")-1,
+        1024-4-strlen("kittycatcat")-1,
+
+        strlen("kittycatcat")*(COUNT-2)-1,
+    };
 
     for (unsigned i = 0; i < sizeof(offsets) / sizeof(offsets[0]); i++) {
         lfs_soff_t off = offsets[i];
+        // write @ offset
         memcpy(buffer, "hedgehoghog", size);
         lfs_file_seek(&lfs, &file, off, LFS_SEEK_SET) => off;
         lfs_file_write(&lfs, &file, buffer, size) => size;
+
+        // read @ offset
         lfs_file_seek(&lfs, &file, off, LFS_SEEK_SET) => off;
         lfs_file_read(&lfs, &file, buffer, size) => size;
         memcmp(buffer, "hedgehoghog", size) => 0;
 
+        // read @ 0
         lfs_file_seek(&lfs, &file, 0, LFS_SEEK_SET) => 0;
         lfs_file_read(&lfs, &file, buffer, size) => size;
         memcmp(buffer, "kittycatcat", size) => 0;
 
+        // read @ offset
         lfs_file_seek(&lfs, &file, off, LFS_SEEK_SET) => off;
         lfs_file_read(&lfs, &file, buffer, size) => size;
         memcmp(buffer, "hedgehoghog", size) => 0;
 
         lfs_file_sync(&lfs, &file) => 0;
 
+        // read @ 0
         lfs_file_seek(&lfs, &file, 0, LFS_SEEK_SET) => 0;
         lfs_file_read(&lfs, &file, buffer, size) => size;
         memcmp(buffer, "kittycatcat", size) => 0;
 
+        // read @ offset
         lfs_file_seek(&lfs, &file, off, LFS_SEEK_SET) => off;
         lfs_file_read(&lfs, &file, buffer, size) => size;
         memcmp(buffer, "hedgehoghog", size) => 0;