Skip to content

Commit

Permalink
Fixed bug where globals were poisoning move commits
Browse files Browse the repository at this point in the history 
The issue lies in the reuse of the id field for globals. Before globals,
the only tags with a non-null (0x3ff) id field were names, structs, and
other file-specific metadata. But globals are also using this field for
the indirect delete, since otherwise the globals structure would be very
unaligned (74-bits long).

To make matters worse, the id field for globals contains the delta used
to reconstruct the globals at mount time. Which means the id field could
take on very absurd values and break the dir fetch logic if we're not
careful.

Solution is to use the scope portion of the type field where necessary,
although unforunately this does add some code cost.
  • Loading branch information
@geky
geky committed Oct 15, 2018
1 parent b46fcac commit 2ff32d2
Show file tree
Hide file tree
Showing 3 changed files with 56 additions and 12 deletions.
18 changes: 18 additions & 0 deletions lfs.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -664,6 +664,13 @@ static int lfs_commit_movescan(lfs_t *lfs, void *p, lfs_mattr_t attr) {
return 0;
}

// TODO AHHHH, scopes, what about user scope above?
if (lfs_tag_scope(attr.tag) == LFS_SCOPE_FS ||
lfs_tag_scope(attr.tag) == LFS_SCOPE_DIR) {
// ignore non-matching ids
return 0;
}

if (lfs_tag_id(attr.tag) != move->id.from) {
// ignore non-matching ids
return 0;
Expand Down Expand Up @@ -1023,6 +1030,9 @@ static int lfs_dir_compact(lfs_t *lfs, lfs_mdir_t *dir, lfs_mattrlist_t *list,

// There's nothing special about our global delta, so feed it back
// into the global global delta
// TODO IMMENSE HMM globals get bleed into from above, need to be fixed after commits due to potential moves
lfs_globals_t gtemp = dir->globals; // TODO hmm, why did we have different variables then?

lfs->diff = lfs_globals_xor(&lfs->diff, &dir->globals);
dir->globals = (lfs_globals_t){0};

Expand Down Expand Up @@ -1196,6 +1206,8 @@ static int lfs_dir_compact(lfs_t *lfs, lfs_mdir_t *dir, lfs_mattrlist_t *list,
lfs->diff = (lfs_globals_t){0};
}

lfs->globals = lfs_globals_xor(&lfs->globals, &gtemp); // TODO hmm, why did we have different variables then?

return 0;
}

Expand Down Expand Up @@ -1244,6 +1256,8 @@ static int lfs_dir_commit(lfs_t *lfs, lfs_mdir_t *dir, lfs_mattrlist_t *list) {
// successful commit, lets update dir
dir->off = commit.off;
dir->etag = commit.ptag;
// // TODO hm
// dir->globals = lfs_globals_xor(&dir->globals, &lfs->diff);
lfs->globals = lfs_globals_xor(&lfs->globals, &lfs->diff);
lfs->diff = (lfs_globals_t){0};
break;
Expand Down Expand Up @@ -2954,6 +2968,10 @@ int lfs_rename(lfs_t *lfs, const char *oldpath, const char *newpath) {
return res;
}

// TODO test for global state stealing?
// steal global state
lfs->globals = lfs_globals_xor(&lfs->globals, &prevdir.globals);

LFS_ASSERT(res); // must have pred
newcwd.tail[0] = prevdir.tail[0];
newcwd.tail[1] = prevdir.tail[1];
Expand Down
2 changes: 1 addition & 1 deletion lfs.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ enum lfs_type {
// internal sources
LFS_FROM_REGION = 0x000,
LFS_FROM_DISK = 0x200,
LFS_FROM_MOVE = 0x0ff,
LFS_FROM_MOVE = 0x03f,
};

// File open flags
Expand Down
48 changes: 37 additions & 11 deletions tests/test_move.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ tests/test.py << TEST
TEST

echo "--- Move file after corrupt ---"
tests/test.py -s << TEST
tests/test.py << TEST
lfs_mount(&lfs, &cfg) => 0;
lfs_rename(&lfs, "c/hello", "d/hello") => 0;
lfs_unmount(&lfs) => 0;
Expand Down Expand Up @@ -166,7 +166,7 @@ tests/test.py << TEST
lfs_rename(&lfs, "b/hi", "c/hi") => 0;
lfs_unmount(&lfs) => 0;
TEST
rm -v blocks/7
truncate -s-11 blocks/7
tests/test.py << TEST
lfs_mount(&lfs, &cfg) => 0;
lfs_dir_open(&lfs, &dir[0], "b") => 0;
Expand All @@ -182,8 +182,6 @@ tests/test.py << TEST
lfs_dir_read(&lfs, &dir[0], &info) => 1;
strcmp(info.name, "..") => 0;
lfs_dir_read(&lfs, &dir[0], &info) => 1;
strcmp(info.name, "hello") => 0;
lfs_dir_read(&lfs, &dir[0], &info) => 1;
strcmp(info.name, "hi") => 0;
lfs_dir_read(&lfs, &dir[0], &info) => 0;
lfs_unmount(&lfs) => 0;
Expand All @@ -195,8 +193,8 @@ tests/test.py << TEST
lfs_rename(&lfs, "c/hi", "d/hi") => 0;
lfs_unmount(&lfs) => 0;
TEST
rm -v blocks/9
rm -v blocks/a
truncate -s-11 blocks/9
truncate -s-11 blocks/b
tests/test.py << TEST
lfs_mount(&lfs, &cfg) => 0;
lfs_dir_open(&lfs, &dir[0], "c") => 0;
Expand All @@ -205,16 +203,44 @@ tests/test.py << TEST
lfs_dir_read(&lfs, &dir[0], &info) => 1;
strcmp(info.name, "..") => 0;
lfs_dir_read(&lfs, &dir[0], &info) => 1;
strcmp(info.name, "hi") => 0;
lfs_dir_read(&lfs, &dir[0], &info) => 0;
lfs_dir_close(&lfs, &dir[0]) => 0;
lfs_dir_open(&lfs, &dir[0], "d") => 0;
lfs_dir_read(&lfs, &dir[0], &info) => 1;
strcmp(info.name, ".") => 0;
lfs_dir_read(&lfs, &dir[0], &info) => 1;
strcmp(info.name, "..") => 0;
lfs_dir_read(&lfs, &dir[0], &info) => 1;
strcmp(info.name, "hello") => 0;
lfs_dir_read(&lfs, &dir[0], &info) => 0;
lfs_unmount(&lfs) => 0;
TEST

echo "--- Move dir after corrupt ---"
tests/test.py << TEST
lfs_mount(&lfs, &cfg) => 0;
lfs_rename(&lfs, "c/hi", "d/hi") => 0;
lfs_unmount(&lfs) => 0;
TEST
tests/test.py << TEST
lfs_mount(&lfs, &cfg) => 0;
lfs_dir_open(&lfs, &dir[0], "c") => 0;
lfs_dir_read(&lfs, &dir[0], &info) => 1;
strcmp(info.name, "hi") => 0;
strcmp(info.name, ".") => 0;
lfs_dir_read(&lfs, &dir[0], &info) => 1;
strcmp(info.name, "..") => 0;
lfs_dir_read(&lfs, &dir[0], &info) => 0;
lfs_dir_close(&lfs, &dir[0]) => 0;
lfs_dir_open(&lfs, &dir[0], "d") => 0;
lfs_dir_read(&lfs, &dir[0], &info) => 1;
strcmp(info.name, ".") => 0;
lfs_dir_read(&lfs, &dir[0], &info) => 1;
strcmp(info.name, "..") => 0;
lfs_dir_read(&lfs, &dir[0], &info) => 1;
strcmp(info.name, "hello") => 0;
lfs_dir_read(&lfs, &dir[0], &info) => 1;
strcmp(info.name, "hi") => 0;
lfs_dir_read(&lfs, &dir[0], &info) => 0;
lfs_unmount(&lfs) => 0;
TEST
Expand All @@ -225,9 +251,9 @@ tests/test.py << TEST
lfs_dir_open(&lfs, &dir[0], "a/hi") => LFS_ERR_NOENT;
lfs_dir_open(&lfs, &dir[0], "b/hi") => LFS_ERR_NOENT;
lfs_dir_open(&lfs, &dir[0], "d/hi") => LFS_ERR_NOENT;
lfs_dir_open(&lfs, &dir[0], "c/hi") => LFS_ERR_NOENT;
lfs_dir_open(&lfs, &dir[0], "c/hi") => 0;
lfs_dir_open(&lfs, &dir[0], "d/hi") => 0;
lfs_dir_read(&lfs, &dir[0], &info) => 1;
strcmp(info.name, ".") => 0;
lfs_dir_read(&lfs, &dir[0], &info) => 1;
Expand All @@ -243,9 +269,9 @@ tests/test.py << TEST
lfs_dir_open(&lfs, &dir[0], "a/hello") => LFS_ERR_NOENT;
lfs_dir_open(&lfs, &dir[0], "b/hello") => LFS_ERR_NOENT;
lfs_dir_open(&lfs, &dir[0], "d/hello") => LFS_ERR_NOENT;
lfs_dir_open(&lfs, &dir[0], "c/hello") => LFS_ERR_NOENT;
lfs_file_open(&lfs, &file[0], "c/hello", LFS_O_RDONLY) => 0;
lfs_file_open(&lfs, &file[0], "d/hello", LFS_O_RDONLY) => 0;
lfs_file_read(&lfs, &file[0], buffer, 5) => 5;
memcmp(buffer, "hola\n", 5) => 0;
lfs_file_read(&lfs, &file[0], buffer, 8) => 8;
Expand Down

0 comments on commit 2ff32d2

Please sign in to comment.