From 26e8bc2e9eaf989fab67c7c3da433280635cac4b Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Wed, 4 Feb 2026 17:55:18 -0600 Subject: [PATCH] Fixed writes bypassing/not updating holes in tracked file leaves Turns out we were using a slightly wrong condition for when to discard file leaves in lfs3_file_flush_. An unsurprising mistake given size vs weight subtleties. As a result, it was possible for a write to bypass the leaf, leaving it with an outdated weight, resulting in an unexpected hole in the file. This was surprisingly hard to find as most writes don't leave the leaf with hole information, only reads. Fortunately a solution is easy. Just don't use the bptr size here, instead use the full leaf weight to decide when to discard tracked file leaves. Code changes humorously canceling out the Valgrind fix: code stack ctx before: 35260 2136 660 after: 35256 (-0.0%) 2136 (+0.0%) 660 (+0.0%) --- This was found by test_fsync_rwtfrwtf_sparse_fuzz, but only by luck after the CRYSTAL_THRESH/8 -> CRYSTAL_THRESH/16 tweak. To prevent a regression, and hopefully catch other bugs like this (something something cache coherency hard problem), I added a couple "clip" tests that try to force the cache/leaf bypassing behavior: - test_fwrite_clip_cache - try clipping the file cache - test_fwrite_clip_leaf - try clipping the file leaf - test_fwrite_clip_hole - try clipping the file leaf+hole test_fwrite_clip_hole does reproduce the bug. --- lfs3.c | 2 +- tests/test_fwrite.toml | 497 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 498 insertions(+), 1 deletion(-) diff --git a/lfs3.c b/lfs3.c index 423a3183..d033387a 100644 --- a/lfs3.c +++ b/lfs3.c @@ -13987,7 +13987,7 @@ fragment:; // until after the commit, so we can't track it in our leaf // quite yet if (!lfs3_bptr_isbptr(&file->leaf.bptr) - || (pos < file->leaf.pos + lfs3_bptr_size(&file->leaf.bptr) + || (pos < file->leaf.pos + file->leaf.weight && pos + size > file->leaf.pos)) { lfs3_file_discardleaf(file); } diff --git a/tests/test_fwrite.toml b/tests/test_fwrite.toml index 7ee50cf7..b2e8bab0 100644 --- a/tests/test_fwrite.toml +++ b/tests/test_fwrite.toml @@ -1198,6 +1198,503 @@ code = ''' lfs3_unmount(&lfs3) => 0; ''' +# test some annoying corner cases with overwriting file caches/leaves +[cases.test_fwrite_clip_cache] +defines.SIZE = '4*BLOCK_SIZE' +# MASK&0x1 => clip before +# MASK&0x2 => clip after +defines.MASK = [0, 1, 2, 3] +defines.CLIP = [ + # large enough to bypass the cache + 'BLOCK_SIZE', + # large enough to bypass the cache, but not steal the file leaf! + 'FCACHE_SIZE + 1', +] +if = [ + # this just saves testing time + 'SIZE <= 4*1024*FRAGMENT_SIZE', +] +in = 'lfs3.c' +code = ''' + lfs3_t lfs3; + lfs3_format(&lfs3, LFS3_F_RDWR, CFG) => 0; + lfs3_mount(&lfs3, LFS3_M_RDWR, CFG) => 0; + + // create a file + lfs3_file_t file; + lfs3_file_open(&lfs3, &file, "hello", + LFS3_O_RDWR | LFS3_O_CREAT | LFS3_O_EXCL) => 0; + uint8_t wbuf[SIZE]; + uint32_t prng = 42; + for (lfs3_size_t i = 0; i < SIZE; i++) { + wbuf[i] = 'a' + (TEST_PRNG(&prng) % 26); + } + lfs3_file_write(&lfs3, &file, wbuf, SIZE) => SIZE; + + // simulate our file in ram + uint8_t sim[SIZE]; + memcpy(sim, wbuf, SIZE); + + // write a bit to fill the cache + lfs3_off_t off = BLOCK_SIZE; + lfs3_off_t size = FCACHE_SIZE/2; + lfs3_file_seek(&lfs3, &file, off, LFS3_SEEK_SET) => off; + for (lfs3_size_t i = 0; i < size; i++) { + wbuf[i] = 'a' + (TEST_PRNG(&prng) % 26); + } + lfs3_file_write(&lfs3, &file, wbuf, size) => size; + + memcpy(sim + off, wbuf, size); + + // cache filled? + assert(file.cache.pos == BLOCK_SIZE); + assert(file.cache.size == FCACHE_SIZE/2); + + // write a bit before, clipping our cache + if (MASK & 0x1) { + lfs3_off_t off = BLOCK_SIZE - CLIP + 1; + lfs3_off_t size = CLIP; + lfs3_file_seek(&lfs3, &file, off, LFS3_SEEK_SET) => off; + for (lfs3_size_t i = 0; i < size; i++) { + wbuf[i] = 'a' + (TEST_PRNG(&prng) % 26); + } + lfs3_file_write(&lfs3, &file, wbuf, size) => size; + + memcpy(sim + off, wbuf, size); + } + + // write a bit after, clipping our cache + if (MASK & 0x2) { + lfs3_off_t off = BLOCK_SIZE + (FCACHE_SIZE/2) - 1; + lfs3_off_t size = CLIP; + lfs3_file_seek(&lfs3, &file, off, LFS3_SEEK_SET) => off; + for (lfs3_size_t i = 0; i < size; i++) { + wbuf[i] = 'a' + (TEST_PRNG(&prng) % 26); + } + lfs3_file_write(&lfs3, &file, wbuf, size) => size; + + memcpy(sim + off, wbuf, size); + } + + // before we close! try reading our file + // + // this has the highest chance of reading something wrong + uint8_t rbuf[2*SIZE]; + memset(rbuf, 0xaa, 2*SIZE); + + // first try reading _in_ our cache (which may have been discarded) + off = BLOCK_SIZE + 1; + size = FCACHE_SIZE/2 - 2; + lfs3_file_seek(&lfs3, &file, off, LFS3_SEEK_SET) => off; + lfs3_file_read(&lfs3, &file, rbuf, size) => size; + // does our file match our simulation? + assert(memcmp(rbuf, sim + off, size) == 0); + + // then try reading the full cache (which may have been discarded) + off = BLOCK_SIZE; + size = FCACHE_SIZE/2; + lfs3_file_seek(&lfs3, &file, off, LFS3_SEEK_SET) => off; + lfs3_file_read(&lfs3, &file, rbuf, size) => size; + // does our file match our simulation? + assert(memcmp(rbuf, sim + off, size) == 0); + + // then try reading the full file + lfs3_file_seek(&lfs3, &file, 0, LFS3_SEEK_SET) => 0; + lfs3_file_read(&lfs3, &file, rbuf, 2*SIZE) => SIZE; + // does our file match our simulation? + assert(memcmp(rbuf, sim, SIZE) == 0); + + lfs3_file_close(&lfs3, &file) => 0; + + // try reading after closing + for (int remount = 0; remount < 2; remount++) { + // remount? + if (remount) { + lfs3_unmount(&lfs3) => 0; + lfs3_mount(&lfs3, LFS3_M_RDWR, CFG) => 0; + } + + // check our file with stat + struct lfs3_info info; + lfs3_stat(&lfs3, "hello", &info) => 0; + assert(strcmp(info.name, "hello") == 0); + assert(info.type == LFS3_TYPE_REG); + assert(info.size == SIZE); + + // and with dir read + lfs3_dir_t dir; + lfs3_dir_open(&lfs3, &dir, "/") => 0; + lfs3_dir_read(&lfs3, &dir, &info) => 0; + assert(strcmp(info.name, ".") == 0); + assert(info.type == LFS3_TYPE_DIR); + assert(info.size == 0); + lfs3_dir_read(&lfs3, &dir, &info) => 0; + assert(strcmp(info.name, "..") == 0); + assert(info.type == LFS3_TYPE_DIR); + assert(info.size == 0); + lfs3_dir_read(&lfs3, &dir, &info) => 0; + assert(strcmp(info.name, "hello") == 0); + assert(info.type == LFS3_TYPE_REG); + assert(info.size == SIZE); + lfs3_dir_read(&lfs3, &dir, &info) => LFS3_ERR_NOENT; + lfs3_dir_close(&lfs3, &dir) => 0; + + // try reading our file + lfs3_file_open(&lfs3, &file, "hello", LFS3_O_RDONLY) => 0; + // is size correct? + lfs3_file_size(&lfs3, &file) => SIZE; + // try reading + uint8_t rbuf[2*SIZE]; + memset(rbuf, 0xaa, 2*SIZE); + lfs3_file_read(&lfs3, &file, rbuf, 2*SIZE) => SIZE; + // does our file match our simulation? + assert(memcmp(rbuf, sim, SIZE) == 0); + lfs3_file_close(&lfs3, &file) => 0; + } + + lfs3_unmount(&lfs3) => 0; +''' + +[cases.test_fwrite_clip_leaf] +defines.SIZE = '4*BLOCK_SIZE' +# MASK&0x1 => clip before +# MASK&0x2 => clip after +defines.MASK = [0, 1, 2, 3] +defines.CLIP = [ + # large enough to bypass the cache + 'BLOCK_SIZE', + # large enough to bypass the cache, but not steal the file leaf! + 'FCACHE_SIZE + 1', +] +if = [ + # this just saves testing time + 'SIZE <= 4*1024*FRAGMENT_SIZE', +] +in = 'lfs3.c' +code = ''' + lfs3_t lfs3; + lfs3_format(&lfs3, LFS3_F_RDWR, CFG) => 0; + lfs3_mount(&lfs3, LFS3_M_RDWR, CFG) => 0; + + // create a file + lfs3_file_t file; + lfs3_file_open(&lfs3, &file, "hello", + LFS3_O_RDWR | LFS3_O_CREAT | LFS3_O_EXCL) => 0; + uint8_t wbuf[SIZE]; + uint32_t prng = 42; + for (lfs3_size_t i = 0; i < SIZE; i++) { + wbuf[i] = 'a' + (TEST_PRNG(&prng) % 26); + } + lfs3_file_write(&lfs3, &file, wbuf, SIZE) => SIZE; + + // simulate our file in ram + uint8_t sim[SIZE]; + memcpy(sim, wbuf, SIZE); + + // write a bit to crystallize a leaf + lfs3_off_t off = BLOCK_SIZE; + lfs3_off_t size = BLOCK_SIZE/2; + lfs3_file_seek(&lfs3, &file, off, LFS3_SEEK_SET) => off; + for (lfs3_size_t i = 0; i < size; i++) { + wbuf[i] = 'a' + (TEST_PRNG(&prng) % 26); + } + lfs3_file_write(&lfs3, &file, wbuf, size) => size; + + memcpy(sim + off, wbuf, size); + + // do we have a tracked leaf now? + assert(file.leaf.pos == BLOCK_SIZE); + assert(lfs3_bptr_size(&file.leaf.bptr) == BLOCK_SIZE/2); + assert(file.leaf.weight == BLOCK_SIZE/2); + + // write a bit before, clipping our leaf + if (MASK & 0x1) { + lfs3_off_t off = BLOCK_SIZE - CLIP + 1; + lfs3_off_t size = CLIP; + lfs3_file_seek(&lfs3, &file, off, LFS3_SEEK_SET) => off; + for (lfs3_size_t i = 0; i < size; i++) { + wbuf[i] = 'a' + (TEST_PRNG(&prng) % 26); + } + lfs3_file_write(&lfs3, &file, wbuf, size) => size; + + memcpy(sim + off, wbuf, size); + } + + // write a bit after, clipping our leaf + if (MASK & 0x2) { + lfs3_off_t off = BLOCK_SIZE + (BLOCK_SIZE/2) - 1; + lfs3_off_t size = CLIP; + lfs3_file_seek(&lfs3, &file, off, LFS3_SEEK_SET) => off; + for (lfs3_size_t i = 0; i < size; i++) { + wbuf[i] = 'a' + (TEST_PRNG(&prng) % 26); + } + lfs3_file_write(&lfs3, &file, wbuf, size) => size; + + memcpy(sim + off, wbuf, size); + } + + // before we close! try reading our file + // + // this has the highest chance of reading something wrong + uint8_t rbuf[2*SIZE]; + memset(rbuf, 0xaa, 2*SIZE); + + // first try reading _in_ our leaf (which may have been discarded) + off = BLOCK_SIZE + 1; + size = BLOCK_SIZE/2 - 2; + lfs3_file_seek(&lfs3, &file, off, LFS3_SEEK_SET) => off; + lfs3_file_read(&lfs3, &file, rbuf, size) => size; + // does our file match our simulation? + assert(memcmp(rbuf, sim + off, size) == 0); + + // then try reading the full leaf (which may have been discarded) + off = BLOCK_SIZE; + size = BLOCK_SIZE/2; + lfs3_file_seek(&lfs3, &file, off, LFS3_SEEK_SET) => off; + lfs3_file_read(&lfs3, &file, rbuf, size) => size; + // does our file match our simulation? + assert(memcmp(rbuf, sim + off, size) == 0); + + // then try reading the full file + lfs3_file_seek(&lfs3, &file, 0, LFS3_SEEK_SET) => 0; + lfs3_file_read(&lfs3, &file, rbuf, 2*SIZE) => SIZE; + // does our file match our simulation? + assert(memcmp(rbuf, sim, SIZE) == 0); + + lfs3_file_close(&lfs3, &file) => 0; + + // try reading after closing + for (int remount = 0; remount < 2; remount++) { + // remount? + if (remount) { + lfs3_unmount(&lfs3) => 0; + lfs3_mount(&lfs3, LFS3_M_RDWR, CFG) => 0; + } + + // check our file with stat + struct lfs3_info info; + lfs3_stat(&lfs3, "hello", &info) => 0; + assert(strcmp(info.name, "hello") == 0); + assert(info.type == LFS3_TYPE_REG); + assert(info.size == SIZE); + + // and with dir read + lfs3_dir_t dir; + lfs3_dir_open(&lfs3, &dir, "/") => 0; + lfs3_dir_read(&lfs3, &dir, &info) => 0; + assert(strcmp(info.name, ".") == 0); + assert(info.type == LFS3_TYPE_DIR); + assert(info.size == 0); + lfs3_dir_read(&lfs3, &dir, &info) => 0; + assert(strcmp(info.name, "..") == 0); + assert(info.type == LFS3_TYPE_DIR); + assert(info.size == 0); + lfs3_dir_read(&lfs3, &dir, &info) => 0; + assert(strcmp(info.name, "hello") == 0); + assert(info.type == LFS3_TYPE_REG); + assert(info.size == SIZE); + lfs3_dir_read(&lfs3, &dir, &info) => LFS3_ERR_NOENT; + lfs3_dir_close(&lfs3, &dir) => 0; + + // try reading our file + lfs3_file_open(&lfs3, &file, "hello", LFS3_O_RDONLY) => 0; + // is size correct? + lfs3_file_size(&lfs3, &file) => SIZE; + // try reading + uint8_t rbuf[2*SIZE]; + memset(rbuf, 0xaa, 2*SIZE); + lfs3_file_read(&lfs3, &file, rbuf, 2*SIZE) => SIZE; + // does our file match our simulation? + assert(memcmp(rbuf, sim, SIZE) == 0); + lfs3_file_close(&lfs3, &file) => 0; + } + + lfs3_unmount(&lfs3) => 0; +''' + +[cases.test_fwrite_clip_hole] +defines.SIZE = '4*BLOCK_SIZE' +# MASK&0x1 => clip before +# MASK&0x2 => clip after +defines.MASK = [0, 1, 2, 3] +defines.CLIP = [ + # large enough to bypass the cache + 'BLOCK_SIZE', + # large enough to bypass the cache, but not steal the file leaf! + 'FCACHE_SIZE + 1', +] +if = [ + # this just saves testing time + 'SIZE <= 4*1024*FRAGMENT_SIZE', +] +in = 'lfs3.c' +code = ''' + lfs3_t lfs3; + lfs3_format(&lfs3, LFS3_F_RDWR, CFG) => 0; + lfs3_mount(&lfs3, LFS3_M_RDWR, CFG) => 0; + + // create a _sparse_ file + lfs3_file_t file; + lfs3_file_open(&lfs3, &file, "hello", + LFS3_O_RDWR | LFS3_O_CREAT | LFS3_O_EXCL) => 0; + + // simulate our file in ram + uint8_t sim[SIZE]; + memset(sim, 0, SIZE); + + // write some data, but only to the head and tail, we want a nice + // big hole in the middle + uint8_t wbuf[SIZE]; + uint32_t prng = 42; + for (lfs3_size_t i = 0; i < BLOCK_SIZE; i++) { + wbuf[i] = 'a' + (TEST_PRNG(&prng) % 26); + } + lfs3_file_write(&lfs3, &file, wbuf, BLOCK_SIZE) => BLOCK_SIZE; + + memcpy(sim, wbuf, BLOCK_SIZE); + + lfs3_file_seek(&lfs3, &file, 3*BLOCK_SIZE, LFS3_SEEK_SET) + => 3*BLOCK_SIZE; + for (lfs3_size_t i = 0; i < BLOCK_SIZE; i++) { + wbuf[i] = 'a' + (TEST_PRNG(&prng) % 26); + } + lfs3_file_write(&lfs3, &file, wbuf, BLOCK_SIZE) => BLOCK_SIZE; + + memcpy(sim + 3*BLOCK_SIZE, wbuf, BLOCK_SIZE); + + // write a bit to crystallize a leaf + lfs3_off_t off = BLOCK_SIZE; + lfs3_off_t size = BLOCK_SIZE/2; + lfs3_file_seek(&lfs3, &file, off, LFS3_SEEK_SET) => off; + for (lfs3_size_t i = 0; i < size; i++) { + wbuf[i] = 'a' + (TEST_PRNG(&prng) % 26); + } + lfs3_file_write(&lfs3, &file, wbuf, size) => size; + + memcpy(sim + off, wbuf, size); + + // read a full block somewhere else to force our leaf into the bshrub + lfs3_file_seek(&lfs3, &file, 0, LFS3_SEEK_SET) => 0; + uint8_t rbuf[2*SIZE]; + lfs3_file_read(&lfs3, &file, rbuf, BLOCK_SIZE/2) => BLOCK_SIZE/2; + assert(memcmp(rbuf, sim, BLOCK_SIZE/2) == 0); + + // and then read our leaf again, note we can't just call + // lfs3_file_flush because it wouldn't fetch the hole information + lfs3_file_seek(&lfs3, &file, BLOCK_SIZE, LFS3_SEEK_SET) => BLOCK_SIZE; + lfs3_file_read(&lfs3, &file, rbuf, BLOCK_SIZE/2) => BLOCK_SIZE/2; + assert(memcmp(rbuf, sim + BLOCK_SIZE, BLOCK_SIZE/2) == 0); + + // do we have a tracked leaf now? + assert(file.leaf.pos == BLOCK_SIZE); + assert(lfs3_bptr_size(&file.leaf.bptr) == BLOCK_SIZE/2); + // note the big hole in our leaf! + assert(file.leaf.weight == 2*BLOCK_SIZE); + + // write a bit before, clipping our leaf + if (MASK & 0x1) { + lfs3_off_t off = BLOCK_SIZE - CLIP + 1; + lfs3_off_t size = CLIP; + lfs3_file_seek(&lfs3, &file, off, LFS3_SEEK_SET) => off; + for (lfs3_size_t i = 0; i < size; i++) { + wbuf[i] = 'a' + (TEST_PRNG(&prng) % 26); + } + lfs3_file_write(&lfs3, &file, wbuf, size) => size; + + memcpy(sim + off, wbuf, size); + } + + // write a bit after, clipping our leaf + if (MASK & 0x2) { + lfs3_off_t off = BLOCK_SIZE + (2*BLOCK_SIZE) - 1; + lfs3_off_t size = CLIP; + lfs3_file_seek(&lfs3, &file, off, LFS3_SEEK_SET) => off; + for (lfs3_size_t i = 0; i < size; i++) { + wbuf[i] = 'a' + (TEST_PRNG(&prng) % 26); + } + lfs3_file_write(&lfs3, &file, wbuf, size) => size; + + memcpy(sim + off, wbuf, size); + } + + // before we close! try reading our file + // + // this has the highest chance of reading something wrong + memset(rbuf, 0xaa, 2*SIZE); + + // first try reading _in_ our leaf (which may have been discarded) + off = BLOCK_SIZE + 1; + size = 2*BLOCK_SIZE - 2; + lfs3_file_seek(&lfs3, &file, off, LFS3_SEEK_SET) => off; + lfs3_file_read(&lfs3, &file, rbuf, size) => size; + // does our file match our simulation? + assert(memcmp(rbuf, sim + off, size) == 0); + + // then try reading the full leaf (which may have been discarded) + off = BLOCK_SIZE; + size = 2*BLOCK_SIZE; + lfs3_file_seek(&lfs3, &file, off, LFS3_SEEK_SET) => off; + lfs3_file_read(&lfs3, &file, rbuf, size) => size; + // does our file match our simulation? + assert(memcmp(rbuf, sim + off, size) == 0); + + // then try reading the full file + lfs3_file_seek(&lfs3, &file, 0, LFS3_SEEK_SET) => 0; + lfs3_file_read(&lfs3, &file, rbuf, 2*SIZE) => SIZE; + // does our file match our simulation? + assert(memcmp(rbuf, sim, SIZE) == 0); + + lfs3_file_close(&lfs3, &file) => 0; + + // try reading after closing + for (int remount = 0; remount < 2; remount++) { + // remount? + if (remount) { + lfs3_unmount(&lfs3) => 0; + lfs3_mount(&lfs3, LFS3_M_RDWR, CFG) => 0; + } + + // check our file with stat + struct lfs3_info info; + lfs3_stat(&lfs3, "hello", &info) => 0; + assert(strcmp(info.name, "hello") == 0); + assert(info.type == LFS3_TYPE_REG); + assert(info.size == SIZE); + + // and with dir read + lfs3_dir_t dir; + lfs3_dir_open(&lfs3, &dir, "/") => 0; + lfs3_dir_read(&lfs3, &dir, &info) => 0; + assert(strcmp(info.name, ".") == 0); + assert(info.type == LFS3_TYPE_DIR); + assert(info.size == 0); + lfs3_dir_read(&lfs3, &dir, &info) => 0; + assert(strcmp(info.name, "..") == 0); + assert(info.type == LFS3_TYPE_DIR); + assert(info.size == 0); + lfs3_dir_read(&lfs3, &dir, &info) => 0; + assert(strcmp(info.name, "hello") == 0); + assert(info.type == LFS3_TYPE_REG); + assert(info.size == SIZE); + lfs3_dir_read(&lfs3, &dir, &info) => LFS3_ERR_NOENT; + lfs3_dir_close(&lfs3, &dir) => 0; + + // try reading our file + lfs3_file_open(&lfs3, &file, "hello", LFS3_O_RDONLY) => 0; + // is size correct? + lfs3_file_size(&lfs3, &file) => SIZE; + // try reading + uint8_t rbuf[2*SIZE]; + memset(rbuf, 0xaa, 2*SIZE); + lfs3_file_read(&lfs3, &file, rbuf, 2*SIZE) => SIZE; + // does our file match our simulation? + assert(memcmp(rbuf, sim, SIZE) == 0); + lfs3_file_close(&lfs3, &file) => 0; + } + + lfs3_unmount(&lfs3) => 0; +''' + # simple truncate test [cases.test_fwrite_truncate] defines.FROM = [