From ddbfcaa7227c7655d9e7e4a81c92e875a662347f Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Sun, 4 Feb 2024 14:20:02 -0600 Subject: [PATCH] Fixed synthetic move underflows in lfs_dir_get By "luck" the previous code somehow managed to not be broken, though it was possible to traverse the same file twice in lfs_fs_traverse/size (which is not an error). The problem was an underlying assumption in lfs_dir_get that it would never be called when the requested id is pending removal because of a powerloss. The assumption was either: 1. lfs_dir_find would need to be called first to find the id, and it would correctly toss out pending-rms with LFS_ERR_NOENT. 2. lfs_fs_mkconsistent would be implicitly called before any filesystem traversals, cleaning up any pending-rms. This is at least true for allocator scans. But, as noted by andriyndev, both lfs_fs_traverse and lfs_fs_size can call lfs_fs_get with a pending-rm id if called in a readonly context. --- By "luck" this somehow manages to not break anything: 1. If the pending-rm id is >0, the id is decremented by 1 in lfs_fs_get, returning the previous file entry during traversal. Worst case, this reports any blocks owned by the previous file entry twice. Note this is not an error, lfs_fs_traverse/size may return the same block multiple times due to underlying copy-on-write structures. 2. More concerning, if the pending-rm id is 0, the id is decremented by 1 in lfs_fs_get and underflows. This underflow propagates into the type field of the tag we are searching for, decrementing it from 0x200 (LFS_TYPE_STRUCT) to 0x1ff (LFS_TYPE_INTERNAL(UNUSED)). Fortunately, since this happens to underflow to the INTERNAL tag type, the type intended to never exist on disk, we should never find a matching tag during our lfs_fs_get search. The result? lfs_dir_get returns LFS_ERR_NOENT, which is actually what we want. Also note that LFS_ERR_NOENT does not terminate the mdir traversal early. If it did we would have missed files instead of duplicating files, which is a slightly worse situation. --- The fix is to add an explicit check for pending-rms in lfs_dir_get, just like in lfs_dir_find. This avoids relying on unintended underflow propagation, and should make the internal API behavior more consistent. This is especially important for potential future gc extensions. Found by andriyndev --- lfs.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/lfs.c b/lfs.c index d5885ca0..e963485f 100644 --- a/lfs.c +++ b/lfs.c @@ -710,11 +710,14 @@ static lfs_stag_t lfs_dir_getslice(lfs_t *lfs, const lfs_mdir_t *dir, lfs_tag_t ntag = dir->etag; lfs_stag_t gdiff = 0; + // synthetic moves if (lfs_gstate_hasmovehere(&lfs->gdisk, dir->pair) && - lfs_tag_id(gmask) != 0 && - lfs_tag_id(lfs->gdisk.tag) <= lfs_tag_id(gtag)) { - // synthetic moves - gdiff -= LFS_MKTAG(0, 1, 0); + lfs_tag_id(gmask) != 0) { + if (lfs_tag_id(lfs->gdisk.tag) == lfs_tag_id(gtag)) { + return LFS_ERR_NOENT; + } else if (lfs_tag_id(lfs->gdisk.tag) < lfs_tag_id(gtag)) { + gdiff -= LFS_MKTAG(0, 1, 0); + } } // iterate over dir block backwards (for faster lookups)