Skip to content

Commit

Permalink
Fixed dir iteration being broken by concurrent removes
Browse files Browse the repository at this point in the history
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
  • Loading branch information
geky committed Feb 4, 2025
1 parent 0494ce7 commit caba4f3
Show file tree
Hide file tree
Showing 2 changed files with 78 additions and 1 deletion.
3 changes: 2 additions & 1 deletion lfs.c
Original file line number Diff line number Diff line change
Expand Up @@ -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 &&
Expand Down
76 changes: 76 additions & 0 deletions tests/test_dirs.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down

0 comments on commit caba4f3

Please sign in to comment.