Fixed name ordering when names only differ in length

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

lfs.c

@@ -505,7 +505,6 @@ static bool lfs_mlist_isopen(struct lfs_mlist *head,
 #endif
 
 static void lfs_mlist_remove(lfs_t *lfs, struct lfs_mlist *mlist) {
-    LFS_ASSERT(lfs_mlist_isopen(lfs->mlist, mlist));
     for (struct lfs_mlist **p = &lfs->mlist; *p; p = &(*p)->next) {
         if (*p == mlist) {
             *p = (*p)->next;
@@ -515,7 +514,6 @@ static void lfs_mlist_remove(lfs_t *lfs, struct lfs_mlist *mlist) {
 }
 
 static void lfs_mlist_append(lfs_t *lfs, struct lfs_mlist *mlist) {
-    LFS_ASSERT(!lfs_mlist_isopen(lfs->mlist, mlist));
     mlist->next = lfs->mlist;
     lfs->mlist = mlist;
 }
@@ -1432,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!
@@ -3034,7 +3032,8 @@ static int lfs_file_rawopencfg(lfs_t *lfs, lfs_file_t *file,
     // allocate entry for file if it doesn't exist
     lfs_stag_t tag = lfs_dir_find(lfs, &file->m, &path, &file->id);
     if (tag < 0 && !(tag == LFS_ERR_NOENT && file->id != 0x3ff)) {
-        return tag;
+        err = tag;
+        goto cleanup;
     }
 
     // get id, add to list of mdirs to catch update changes
@@ -5930,6 +5929,7 @@ int lfs_file_open(lfs_t *lfs, lfs_file_t *file, const char *path, int flags) {
     }
     LFS_TRACE("lfs_file_open(%p, %p, \"%s\", %x)",
             (void*)lfs, (void*)file, path, flags);
+    LFS_ASSERT(!lfs_mlist_isopen(lfs->mlist, (struct lfs_mlist*)file));
 
     err = lfs_file_rawopen(lfs, file, path, flags);
 
@@ -5950,6 +5950,7 @@ int lfs_file_opencfg(lfs_t *lfs, lfs_file_t *file,
                  ".buffer=%p, .attrs=%p, .attr_count=%"PRIu32"})",
             (void*)lfs, (void*)file, path, flags,
             (void*)cfg, cfg->buffer, (void*)cfg->attrs, cfg->attr_count);
+    LFS_ASSERT(!lfs_mlist_isopen(lfs->mlist, (struct lfs_mlist*)file));
 
     err = lfs_file_rawopencfg(lfs, file, path, flags, cfg);
 
@@ -5964,6 +5965,7 @@ int lfs_file_close(lfs_t *lfs, lfs_file_t *file) {
         return err;
     }
     LFS_TRACE("lfs_file_close(%p, %p)", (void*)lfs, (void*)file);
+    LFS_ASSERT(lfs_mlist_isopen(lfs->mlist, (struct lfs_mlist*)file));
 
     err = lfs_file_rawclose(lfs, file);
 
@@ -5979,6 +5981,7 @@ int lfs_file_sync(lfs_t *lfs, lfs_file_t *file) {
         return err;
     }
     LFS_TRACE("lfs_file_sync(%p, %p)", (void*)lfs, (void*)file);
+    LFS_ASSERT(lfs_mlist_isopen(lfs->mlist, (struct lfs_mlist*)file));
 
     err = lfs_file_rawsync(lfs, file);
 
@@ -5996,6 +5999,7 @@ lfs_ssize_t lfs_file_read(lfs_t *lfs, lfs_file_t *file,
     }
     LFS_TRACE("lfs_file_read(%p, %p, %p, %"PRIu32")",
             (void*)lfs, (void*)file, buffer, size);
+    LFS_ASSERT(lfs_mlist_isopen(lfs->mlist, (struct lfs_mlist*)file));
 
     lfs_ssize_t res = lfs_file_rawread(lfs, file, buffer, size);
 
@@ -6013,6 +6017,7 @@ lfs_ssize_t lfs_file_write(lfs_t *lfs, lfs_file_t *file,
     }
     LFS_TRACE("lfs_file_write(%p, %p, %p, %"PRIu32")",
             (void*)lfs, (void*)file, buffer, size);
+    LFS_ASSERT(lfs_mlist_isopen(lfs->mlist, (struct lfs_mlist*)file));
 
     lfs_ssize_t res = lfs_file_rawwrite(lfs, file, buffer, size);
 
@@ -6030,6 +6035,7 @@ lfs_soff_t lfs_file_seek(lfs_t *lfs, lfs_file_t *file,
     }
     LFS_TRACE("lfs_file_seek(%p, %p, %"PRId32", %d)",
             (void*)lfs, (void*)file, off, whence);
+    LFS_ASSERT(lfs_mlist_isopen(lfs->mlist, (struct lfs_mlist*)file));
 
     lfs_soff_t res = lfs_file_rawseek(lfs, file, off, whence);
 
@@ -6046,6 +6052,7 @@ int lfs_file_truncate(lfs_t *lfs, lfs_file_t *file, lfs_off_t size) {
     }
     LFS_TRACE("lfs_file_truncate(%p, %p, %"PRIu32")",
             (void*)lfs, (void*)file, size);
+    LFS_ASSERT(lfs_mlist_isopen(lfs->mlist, (struct lfs_mlist*)file));
 
     err = lfs_file_rawtruncate(lfs, file, size);
 
@@ -6061,6 +6068,7 @@ lfs_soff_t lfs_file_tell(lfs_t *lfs, lfs_file_t *file) {
         return err;
     }
     LFS_TRACE("lfs_file_tell(%p, %p)", (void*)lfs, (void*)file);
+    LFS_ASSERT(lfs_mlist_isopen(lfs->mlist, (struct lfs_mlist*)file));
 
     lfs_soff_t res = lfs_file_rawtell(lfs, file);
 
@@ -6089,6 +6097,7 @@ lfs_soff_t lfs_file_size(lfs_t *lfs, lfs_file_t *file) {
         return err;
     }
     LFS_TRACE("lfs_file_size(%p, %p)", (void*)lfs, (void*)file);
+    LFS_ASSERT(lfs_mlist_isopen(lfs->mlist, (struct lfs_mlist*)file));
 
     lfs_soff_t res = lfs_file_rawsize(lfs, file);
 
@@ -6119,6 +6128,7 @@ int lfs_dir_open(lfs_t *lfs, lfs_dir_t *dir, const char *path) {
         return err;
     }
     LFS_TRACE("lfs_dir_open(%p, %p, \"%s\")", (void*)lfs, (void*)dir, path);
+    LFS_ASSERT(!lfs_mlist_isopen(lfs->mlist, (struct lfs_mlist*)dir));
 
     err = lfs_dir_rawopen(lfs, dir, path);
 

tests/test_dirs.toml

@@ -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;