From da9ac39c885be5b6f3bde51d123813ae0a25b1f9 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Tue, 20 Aug 2024 15:15:48 -0500 Subject: [PATCH] Fixed issue where FBIG errors did not set the DESYNC flag I think the assumption was that since these errors are trivially noops, they shouldn't change any file state. But this doesn't match the behavior of other errors, which is inconsistent and probably not what users expect. Also added a couple tests around FBIG that should catch this in the future. Curiously this actually saved a word of code, I guess because of rerouting all errors through the same function epilogues: code stack before: 36416 2616 after: 36412 (-0.0%) 2616 (+0.0%) --- lfs.c | 45 +++--- tests/test_fwrite.toml | 313 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 338 insertions(+), 20 deletions(-) diff --git a/lfs.c b/lfs.c index c08df97e..9e90bd8f 100644 --- a/lfs.c +++ b/lfs.c @@ -12402,11 +12402,6 @@ lfs_ssize_t lfsr_file_write(lfs_t *lfs, lfsr_file_t *file, // can't write to readonly files LFS_ASSERT(!lfsr_o_isrdonly(file->o.o.flags)); - // would this write make our file larger than our file limit? - if (size > lfs->file_limit - file->pos) { - return LFS_ERR_FBIG; - } - // size=0 is a bit special and is guaranteed to have no effects on the // underlying file, this means no updating file pos or file size // @@ -12415,6 +12410,13 @@ lfs_ssize_t lfsr_file_write(lfs_t *lfs, lfsr_file_t *file, return 0; } + // would this write make our file larger than our file limit? + int err; + if (size > lfs->file_limit - file->pos) { + err = LFS_ERR_FBIG; + goto failed; + } + // clobber entangled traversals lfsr_omdir_mkdirty(lfs, &file->o.o); // checkpoint the allocator @@ -12441,7 +12443,6 @@ lfs_ssize_t lfsr_file_write(lfs_t *lfs, lfsr_file_t *file, const uint8_t *buffer_ = buffer; lfs_size_t written = 0; - int err; while (size > 0) { // bypass buffer? // @@ -12609,8 +12610,10 @@ failed:; int lfsr_file_sync(lfs_t *lfs, lfsr_file_t *file) { LFS_ASSERT(lfsr_omdir_isopen(lfs, &file->o.o)); // removed? we can't sync + int err; if (lfsr_o_iszombie(file->o.o.flags)) { - return LFS_ERR_NOENT; + err = LFS_ERR_NOENT; + goto failed; } // first flush any data in our buffer, this is a noop if already @@ -12620,7 +12623,7 @@ int lfsr_file_sync(lfs_t *lfs, lfsr_file_t *file) { // flush succeeds but mdir commit fails it's ok to fall back to // our flushed state // - int err = lfsr_file_flush(lfs, file); + err = lfsr_file_flush(lfs, file); if (err) { goto failed; } @@ -12840,17 +12843,19 @@ int lfsr_file_truncate(lfs_t *lfs, lfsr_file_t *file, lfs_off_t size_) { // can't write to readonly files LFS_ASSERT(!lfsr_o_isrdonly(file->o.o.flags)); - // exceeds our file limit? - if (size_ > lfs->file_limit) { - return LFS_ERR_FBIG; - } - // do nothing if our size does not change lfs_off_t size = lfsr_file_size_(file); if (lfsr_file_size_(file) == size_) { return 0; } + // exceeds our file limit? + int err; + if (size_ > lfs->file_limit) { + err = LFS_ERR_FBIG; + goto failed; + } + // clobber entangled traversals lfsr_omdir_mkdirty(lfs, &file->o.o); // checkpoint the allocator @@ -12859,7 +12864,6 @@ int lfsr_file_truncate(lfs_t *lfs, lfsr_file_t *file, lfs_off_t size_) { file->o.o.flags |= LFS_O_UNSYNC; // does our file become small? - int err; if (size_ <= lfsr_file_inlinesize(lfs, file)) { // if our data is not already in our buffer we unfortunately // need to flush so our buffer is available to hold everything @@ -12948,17 +12952,19 @@ int lfsr_file_fruncate(lfs_t *lfs, lfsr_file_t *file, lfs_off_t size_) { // can't write to readonly files LFS_ASSERT(!lfsr_o_isrdonly(file->o.o.flags)); - // exceeds our file limit? - if (size_ > lfs->file_limit) { - return LFS_ERR_FBIG; - } - // do nothing if our size does not change lfs_off_t size = lfsr_file_size_(file); if (size == size_) { return 0; } + // exceeds our file limit? + int err; + if (size_ > lfs->file_limit) { + err = LFS_ERR_FBIG; + goto failed; + } + // clobber entangled traversals lfsr_omdir_mkdirty(lfs, &file->o.o); // checkpoint the allocator @@ -12967,7 +12973,6 @@ int lfsr_file_fruncate(lfs_t *lfs, lfsr_file_t *file, lfs_off_t size_) { file->o.o.flags |= LFS_O_UNSYNC; // does our file become small? - int err; if (size_ <= lfsr_file_inlinesize(lfs, file)) { // if our data is not already in our buffer we unfortunately // need to flush so our buffer is available to hold everything diff --git a/tests/test_fwrite.toml b/tests/test_fwrite.toml index 1538ce89..fa331fc9 100644 --- a/tests/test_fwrite.toml +++ b/tests/test_fwrite.toml @@ -2892,6 +2892,8 @@ code = ''' ''' # test other corner conditions + +# test that seeking to a negative offset errors [cases.test_fwrite_seek_negative] defines.WHENCE = ['LFS_SEEK_SET', 'LFS_SEEK_CUR', 'LFS_SEEK_END'] defines.SIZE = [ @@ -2999,6 +3001,317 @@ code = ''' lfsr_unmount(&lfs) => 0; ''' +# test that write overflow errors +[cases.test_fwrite_fbig] +defines.SIZE = [ + 'FILE_BUFFER_SIZE/2', + '2*FILE_BUFFER_SIZE', + 'BLOCK_SIZE/2', + 'BLOCK_SIZE', + '2*BLOCK_SIZE', + '4*BLOCK_SIZE', +] +# INIT=0 => no init +# INIT=1 => fill with data +# INIT=2 => truncate to size +defines.INIT = [0, 1, 2] +defines.MODE = ['LFS_O_WRONLY', 'LFS_O_RDWR'] +if = [ + # this just saves testing time + 'SIZE <= 4*1024*FRAGMENT_SIZE', +] +code = ''' + lfs_t lfs; + lfsr_format(&lfs, LFS_F_RDWR, CFG) => 0; + lfsr_mount(&lfs, LFS_M_RDWR, CFG) => 0; + + // create a file + lfsr_file_t file; + lfsr_file_open(&lfs, &file, "hello", + LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0; + // simulate our file in ram + uint8_t sim[SIZE]; + lfs_off_t size; + uint32_t prng = 42; + if (INIT == 0) { + memset(sim, 0, SIZE); + size = 0; + } else if (INIT == 1) { + for (lfs_size_t i = 0; i < SIZE; i++) { + sim[i] = 'a' + (TEST_PRNG(&prng) % 26); + } + lfsr_file_write(&lfs, &file, sim, SIZE) => SIZE; + size = SIZE; + } else { + memset(sim, 0, SIZE); + lfsr_file_truncate(&lfs, &file, SIZE) => 0; + size = SIZE; + } + lfsr_file_close(&lfs, &file) => 0; + + // seek to near the file limit + lfsr_file_open(&lfs, &file, "hello", MODE) => 0; + lfsr_file_seek(&lfs, &file, LFS_FILE_MAX-(SIZE/2), LFS_SEEK_SET) + => LFS_FILE_MAX-(SIZE/2); + // try to write past the file limit, this should fail + uint8_t wbuf[SIZE]; + for (lfs_size_t i = 0; i < SIZE; i++) { + wbuf[i] = 'a' + (TEST_PRNG(&prng) % 26); + } + lfsr_file_write(&lfs, &file, wbuf, SIZE) => LFS_ERR_FBIG; + lfsr_file_close(&lfs, &file) => 0; + + for (int remount = 0; remount < 2; remount++) { + // remount? + if (remount) { + lfsr_unmount(&lfs) => 0; + lfsr_mount(&lfs, LFS_M_RDWR, CFG) => 0; + } + + // check our file with stat + struct lfs_info info; + lfsr_stat(&lfs, "hello", &info) => 0; + assert(strcmp(info.name, "hello") == 0); + assert(info.type == LFS_TYPE_REG); + assert(info.size == size); + + // and with dir read + 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, "hello") == 0); + assert(info.type == LFS_TYPE_REG); + assert(info.size == size); + lfsr_dir_read(&lfs, &dir, &info) => LFS_ERR_NOENT; + lfsr_dir_close(&lfs, &dir) => 0; + + // try reading our file + lfsr_file_open(&lfs, &file, "hello", LFS_O_RDONLY) => 0; + // is size correct? + lfsr_file_size(&lfs, &file) => size; + // try reading + uint8_t rbuf[2*SIZE]; + memset(rbuf, 0xaa, 2*SIZE); + lfsr_file_read(&lfs, &file, rbuf, 2*SIZE) => size; + // does our file match our simulation? + assert(memcmp(rbuf, sim, size) == 0); + lfsr_file_close(&lfs, &file) => 0; + } + + lfsr_unmount(&lfs) => 0; +''' + +# test that truncate overflow errors +[cases.test_fwrite_truncate_fbig] +defines.SIZE = [ + 'FILE_BUFFER_SIZE/2', + '2*FILE_BUFFER_SIZE', + 'BLOCK_SIZE/2', + 'BLOCK_SIZE', + '2*BLOCK_SIZE', + '4*BLOCK_SIZE', +] +# INIT=0 => no init +# INIT=1 => fill with data +# INIT=2 => truncate to size +defines.INIT = [0, 1, 2] +defines.MODE = ['LFS_O_WRONLY', 'LFS_O_RDWR'] +if = [ + # this just saves testing time + 'SIZE <= 4*1024*FRAGMENT_SIZE', +] +code = ''' + lfs_t lfs; + lfsr_format(&lfs, LFS_F_RDWR, CFG) => 0; + lfsr_mount(&lfs, LFS_M_RDWR, CFG) => 0; + + // create a file + lfsr_file_t file; + lfsr_file_open(&lfs, &file, "hello", + LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0; + // simulate our file in ram + uint8_t sim[SIZE]; + lfs_off_t size; + uint32_t prng = 42; + if (INIT == 0) { + memset(sim, 0, SIZE); + size = 0; + } else if (INIT == 1) { + for (lfs_size_t i = 0; i < SIZE; i++) { + sim[i] = 'a' + (TEST_PRNG(&prng) % 26); + } + lfsr_file_write(&lfs, &file, sim, SIZE) => SIZE; + size = SIZE; + } else { + memset(sim, 0, SIZE); + lfsr_file_truncate(&lfs, &file, SIZE) => 0; + size = SIZE; + } + lfsr_file_close(&lfs, &file) => 0; + + // try to truncate the file past the file limit, this should fail + lfsr_file_open(&lfs, &file, "hello", MODE) => 0; + lfsr_file_truncate(&lfs, &file, LFS_FILE_MAX+(SIZE/2)) => LFS_ERR_FBIG; + lfsr_file_close(&lfs, &file) => 0; + + for (int remount = 0; remount < 2; remount++) { + // remount? + if (remount) { + lfsr_unmount(&lfs) => 0; + lfsr_mount(&lfs, LFS_M_RDWR, CFG) => 0; + } + + // check our file with stat + struct lfs_info info; + lfsr_stat(&lfs, "hello", &info) => 0; + assert(strcmp(info.name, "hello") == 0); + assert(info.type == LFS_TYPE_REG); + assert(info.size == size); + + // and with dir read + 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, "hello") == 0); + assert(info.type == LFS_TYPE_REG); + assert(info.size == size); + lfsr_dir_read(&lfs, &dir, &info) => LFS_ERR_NOENT; + lfsr_dir_close(&lfs, &dir) => 0; + + // try reading our file + lfsr_file_open(&lfs, &file, "hello", LFS_O_RDONLY) => 0; + // is size correct? + lfsr_file_size(&lfs, &file) => size; + // try reading + uint8_t rbuf[2*SIZE]; + memset(rbuf, 0xaa, 2*SIZE); + lfsr_file_read(&lfs, &file, rbuf, 2*SIZE) => size; + // does our file match our simulation? + assert(memcmp(rbuf, sim, size) == 0); + lfsr_file_close(&lfs, &file) => 0; + } + + lfsr_unmount(&lfs) => 0; +''' + +# test that fruncate overflow errors +[cases.test_fwrite_fruncate_fbig] +defines.SIZE = [ + 'FILE_BUFFER_SIZE/2', + '2*FILE_BUFFER_SIZE', + 'BLOCK_SIZE/2', + 'BLOCK_SIZE', + '2*BLOCK_SIZE', + '4*BLOCK_SIZE', +] +# INIT=0 => no init +# INIT=1 => fill with data +# INIT=2 => truncate to size +defines.INIT = [0, 1, 2] +defines.MODE = ['LFS_O_WRONLY', 'LFS_O_RDWR'] +if = [ + # this just saves testing time + 'SIZE <= 4*1024*FRAGMENT_SIZE', +] +code = ''' + lfs_t lfs; + lfsr_format(&lfs, LFS_F_RDWR, CFG) => 0; + lfsr_mount(&lfs, LFS_M_RDWR, CFG) => 0; + + // create a file + lfsr_file_t file; + lfsr_file_open(&lfs, &file, "hello", + LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0; + // simulate our file in ram + uint8_t sim[SIZE]; + lfs_off_t size; + uint32_t prng = 42; + if (INIT == 0) { + memset(sim, 0, SIZE); + size = 0; + } else if (INIT == 1) { + for (lfs_size_t i = 0; i < SIZE; i++) { + sim[i] = 'a' + (TEST_PRNG(&prng) % 26); + } + lfsr_file_write(&lfs, &file, sim, SIZE) => SIZE; + size = SIZE; + } else { + memset(sim, 0, SIZE); + lfsr_file_truncate(&lfs, &file, SIZE) => 0; + size = SIZE; + } + lfsr_file_close(&lfs, &file) => 0; + + // try to truncate the file past the file limit, this should fail + lfsr_file_open(&lfs, &file, "hello", MODE) => 0; + lfsr_file_fruncate(&lfs, &file, LFS_FILE_MAX+(SIZE/2)) => LFS_ERR_FBIG; + lfsr_file_close(&lfs, &file) => 0; + + for (int remount = 0; remount < 2; remount++) { + // remount? + if (remount) { + lfsr_unmount(&lfs) => 0; + lfsr_mount(&lfs, LFS_M_RDWR, CFG) => 0; + } + + // check our file with stat + struct lfs_info info; + lfsr_stat(&lfs, "hello", &info) => 0; + assert(strcmp(info.name, "hello") == 0); + assert(info.type == LFS_TYPE_REG); + assert(info.size == size); + + // and with dir read + 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, "hello") == 0); + assert(info.type == LFS_TYPE_REG); + assert(info.size == size); + lfsr_dir_read(&lfs, &dir, &info) => LFS_ERR_NOENT; + lfsr_dir_close(&lfs, &dir) => 0; + + // try reading our file + lfsr_file_open(&lfs, &file, "hello", LFS_O_RDONLY) => 0; + // is size correct? + lfsr_file_size(&lfs, &file) => size; + // try reading + uint8_t rbuf[2*SIZE]; + memset(rbuf, 0xaa, 2*SIZE); + lfsr_file_read(&lfs, &file, rbuf, 2*SIZE) => size; + // does our file match our simulation? + assert(memcmp(rbuf, sim, size) == 0); + lfsr_file_close(&lfs, &file) => 0; + } + + lfsr_unmount(&lfs) => 0; +''' + + # heavy fuzz test with rw seeks, truncate, and fruncate [cases.test_fwrite_rwtf_fuzz] defines.N = 20