From 1a99c195f052c3da719149416a4f2e2d06e18cb7 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Mon, 30 Dec 2024 22:38:35 -0600 Subject: [PATCH] Added better error handling of unknown filetypes This is the tradeoff of not erroring on unknown filetypes during mount. - lfsr_file_open and lfsr_mtree_pathlookup now returns LFS_ERR_NOTSUP instead of LFS_ERR_NOTDIR/LFS_ERR_ISDIR if it encounters an unkown filetype. This gets a bit subtle. You might think LFS_ERR_NOTDIR is reasonable, but it's possible for our unknown filetype to be something dir-like. Symlinks are an excellent example. - lfsr_remove/lfsr_rename now bail with LFS_ERR_NOTSUP if encountering an unknown filetype. This conflicts with the POSIX philosophy of remove always being allowed, but I'm not sure what other option there is. Maybe allowing removes when mounted with LFS_M_FORCE? We can't just allow removes by default because of the risk of leaking resources. Directories being the main example of this (need to clean up bookmarks). Maybe leaky filetypes should also set WCOMPAT flags? Not doing something is cheaper than doing something, so unfortunately this costs us more than what we saved from dropping the orphan/unknown scan during mount: code stack ctx bail: 38120 2624 725 no-error-no-bail (before): 38020 (-0.3%) 2624 (+0.0%) 752 (+0.0%) error-no-bail (after): 38140 (+0.1%) 2624 (+0.0%) 752 (+0.0%) But this is probably worth it for the extra flexibility. --- lfs.c | 23 +++++++++-- tests/test_mount.toml | 93 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 113 insertions(+), 3 deletions(-) diff --git a/lfs.c b/lfs.c index d50335f2..bae403c5 100644 --- a/lfs.c +++ b/lfs.c @@ -9523,6 +9523,7 @@ static inline bool lfsr_path_isdir(const char *path) { // - LFS_ERR_NOENT, lfsr_path_islast(path) => file not found // - LFS_ERR_NOENT, !lfsr_path_islast(path) => parent not found // - LFS_ERR_NOTDIR => parent not a dir +// - LFS_ERR_NOTSUP => parent of unknown type // // if not found, mdir_/did_ will at least be set up with what should be // the parent @@ -9603,8 +9604,10 @@ static int lfsr_mtree_pathlookup(lfs_t *lfs, const char **path, // only continue if we hit a directory if (tag != LFSR_TAG_DIR) { return (tag == LFSR_TAG_STICKYNOTE) - ? LFS_ERR_NOENT - : LFS_ERR_NOTDIR; + ? LFS_ERR_NOENT + : (lfsr_tag_isunknown(tag)) + ? LFS_ERR_NOTSUP + : LFS_ERR_NOTDIR; } // read the next did from the mdir if this is not the root @@ -10606,6 +10609,10 @@ int lfsr_remove(lfs_t *lfs, const char *path) { if (tag == LFSR_TAG_STICKYNOTE) { return LFS_ERR_NOENT; } + // we can't remove unknown types or else we may leak resources + if (lfsr_tag_isunknown(tag)) { + return LFS_ERR_NOTSUP; + } // trying to remove the root dir? if (mdir.mid == -1) { @@ -10725,6 +10732,10 @@ int lfsr_rename(lfs_t *lfs, const char *old_path, const char *new_path) { if (old_tag == LFSR_TAG_STICKYNOTE) { return LFS_ERR_NOENT; } + // we can't rename unknown types or else we may leak resources + if (lfsr_tag_isunknown(old_tag)) { + return LFS_ERR_NOTSUP; + } // trying to rename the root? if (old_mdir.mid == -1) { @@ -10740,6 +10751,10 @@ int lfsr_rename(lfs_t *lfs, const char *old_path, const char *new_path) { if (err && !(err == LFS_ERR_NOENT && lfsr_path_islast(new_path))) { return err; } + // we can't rename unknown types or else we may leak resources + if (lfsr_tag_isunknown(new_tag)) { + return LFS_ERR_NOTSUP; + } // already exists? bool exists = (err != LFS_ERR_NOENT); @@ -11616,7 +11631,9 @@ int lfsr_file_opencfg(lfs_t *lfs, lfsr_file_t *file, // wrong type? if (tag != LFSR_TAG_REG) { - return LFS_ERR_ISDIR; + return (tag == LFSR_TAG_DIR) + ? LFS_ERR_ISDIR + : LFS_ERR_NOTSUP; } } diff --git a/tests/test_mount.toml b/tests/test_mount.toml index e79f04ec..2b285ae7 100644 --- a/tests/test_mount.toml +++ b/tests/test_mount.toml @@ -902,3 +902,96 @@ code = ''' lfsr_mount(&lfs, LFS_M_RDONLY, CFG) => LFS_ERR_NOTSUP; ''' +# test what happens if we find an unknown file type +[cases.test_mount_incompat_unknown_type] +in = 'lfs.c' +code = ''' + // create a superblock + lfs_t lfs; + lfsr_format(&lfs, LFS_F_RDWR, CFG) => 0; + + // create some files + lfsr_mount(&lfs, LFS_M_RDWR, CFG) => 0; + lfsr_file_t file; + lfsr_file_open(&lfs, &file, "a", + LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0; + lfsr_file_write(&lfs, &file, + "hi a!", strlen("hi a!")) => strlen("hi a!"); + lfsr_file_close(&lfs, &file) => 0; + lfsr_file_open(&lfs, &file, "b", + LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0; + lfsr_file_write(&lfs, &file, + "oh no!", strlen("oh no!")) => strlen("oh no!"); + lfsr_file_close(&lfs, &file) => 0; + lfsr_file_open(&lfs, &file, "c", + LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0; + lfsr_file_write(&lfs, &file, + "hi c!", strlen("hi c!")) => strlen("hi c!"); + lfsr_file_close(&lfs, &file) => 0; + lfsr_unmount(&lfs) => 0; + + // change a file's type to something unknown + lfsr_mount(&lfs, LFS_M_RDWR, CFG) => 0; + const char *path = "b"; + lfsr_mdir_t mdir; + lfsr_did_t did; + lfsr_mtree_pathlookup(&lfs, &path, + &mdir, NULL, &did) => 0; + lfsr_mdir_commit(&lfs, &mdir, LFSR_RATS( + LFSR_RAT_NAME( + LFSR_TAG_SUB | (LFSR_TAG_NAME + 0x13), 0, + did, path, lfsr_path_namelen(path)))) => 0; + lfsr_unmount(&lfs) => 0; + + // mount + lfsr_mount(&lfs, LFS_M_RDWR, CFG) => 0; + + // our file should appear as an unknown type + struct lfs_info info; + lfsr_stat(&lfs, "b", &info) => 0; + assert(strcmp(info.name, "b") == 0); + assert(info.type == 0x13); + assert(info.size == 0); + + lfsr_dir_t dir; + lfsr_dir_open(&lfs, &dir, "/") => 0; + lfsr_dir_read(&lfs, &dir, &info) => 0; + assert(strcmp(info.name, ".") == 0); + assert(info.type == LFS_TYPE_DIR); + assert(info.size == 0); + lfsr_dir_read(&lfs, &dir, &info) => 0; + assert(strcmp(info.name, "..") == 0); + assert(info.type == LFS_TYPE_DIR); + assert(info.size == 0); + lfsr_dir_read(&lfs, &dir, &info) => 0; + assert(strcmp(info.name, "a") == 0); + assert(info.type == LFS_TYPE_REG); + assert(info.size == strlen("hi a!")); + lfsr_dir_read(&lfs, &dir, &info) => 0; + assert(strcmp(info.name, "b") == 0); + assert(info.type == 0x13); + assert(info.size == 0); + lfsr_dir_read(&lfs, &dir, &info) => 0; + assert(strcmp(info.name, "c") == 0); + assert(info.type == LFS_TYPE_REG); + assert(info.size == strlen("hi c!")); + lfsr_dir_read(&lfs, &dir, &info) => LFS_ERR_NOENT; + lfsr_dir_close(&lfs, &dir) => 0; + + // removing/renaming unknown files should return NOTSUP, we could + // remove the metadata entry, but we would probably leak stuff + lfsr_remove(&lfs, "b") => LFS_ERR_NOTSUP; + lfsr_rename(&lfs, "b", "d") => LFS_ERR_NOTSUP; + lfsr_rename(&lfs, "b", "c") => LFS_ERR_NOTSUP; + lfsr_rename(&lfs, "a", "b") => LFS_ERR_NOTSUP; + + lfsr_file_open(&lfs, &file, "b", + LFS_O_RDONLY) => LFS_ERR_NOTSUP; + lfsr_file_open(&lfs, &file, "b", + LFS_O_WRONLY | LFS_O_CREAT) => LFS_ERR_NOTSUP; + + lfsr_mkdir(&lfs, "b") => LFS_ERR_EXIST; + + lfsr_unmount(&lfs) => 0; +''' +