From 31c59c913e4c1fc2c4fd5fcf9d0f811e68eedd99 Mon Sep 17 00:00:00 2001 From: Peter Andersson Date: Mon, 17 Jul 2017 09:19:08 +0200 Subject: [PATCH] fixes #158 --- heap_overwrite.spiffs | Bin 0 -> 84 bytes makefile | 11 ++++++----- memcpy_overlap.spiffs | Bin 0 -> 143 bytes src/default/spiffs_config.h | 3 +++ src/spiffs.h | 3 +++ src/spiffs_cache.c | 8 ++++---- src/spiffs_hydrogen.c | 33 +++++++++++++++++++++++---------- src/spiffs_nucleus.c | 8 ++++---- src/spiffs_nucleus.h | 20 ++++++++++++++++++++ src/test/test_hydrogen.c | 28 ++++++++++++++++++++++++++++ src/test/testrunner.h | 2 +- src/test/testsuites.c | 2 +- 12 files changed, 93 insertions(+), 25 deletions(-) create mode 100644 heap_overwrite.spiffs create mode 100644 memcpy_overlap.spiffs diff --git a/heap_overwrite.spiffs b/heap_overwrite.spiffs new file mode 100644 index 0000000000000000000000000000000000000000..1aa268981c961e7fb4976d9a71946baddae6fbe5 GIT binary patch literal 84 zcmZQj`2U~FKRVX9(I&CcD^I2~31 literal 0 HcmV?d00001 diff --git a/makefile b/makefile index b1755fc..355b467 100644 --- a/makefile +++ b/makefile @@ -17,11 +17,11 @@ builddir = build ############# CC ?= gcc -LD = ld -GDB = gdb -OBJCOPY = objcopy -OBJDUMP = objdump -MKDIR = mkdir -p +LD ?= ld +GDB ?= gdb +OBJCOPY ?= objcopy +OBJDUMP ?= objdump +MKDIR ?= mkdir -p ############### # @@ -43,6 +43,7 @@ CFILES_TEST = main.c \ test_bugreports.c \ testsuites.c \ testrunner.c +CFLAGS += -D_SPIFFS_TEST endif include files.mk INCLUDE_DIRECTIVES = -I./${sourcedir} -I./${sourcedir}/default -I./${sourcedir}/test diff --git a/memcpy_overlap.spiffs b/memcpy_overlap.spiffs new file mode 100644 index 0000000000000000000000000000000000000000..413620dfed325fc3a090971a00af4a107c897515 GIT binary patch literal 143 zcmeA)4c2uwVNftF%HZ #include #include +#ifdef _SPIFFS_TEST +#include "testrunner.h" +#endif // ----------- >8 ------------ // compile time switches diff --git a/src/spiffs.h b/src/spiffs.h index d87422d..534c3df 100644 --- a/src/spiffs.h +++ b/src/spiffs.h @@ -60,6 +60,9 @@ extern "C" { #define SPIFFS_ERR_IX_MAP_MAPPED -10038 #define SPIFFS_ERR_IX_MAP_BAD_RANGE -10039 +#define SPIFFS_ERR_SEEK_BOUNDS -10040 + + #define SPIFFS_ERR_INTERNAL -10050 #define SPIFFS_ERR_TEST -10100 diff --git a/src/spiffs_cache.c b/src/spiffs_cache.c index 0c5b5b9..37c9d64 100644 --- a/src/spiffs_cache.c +++ b/src/spiffs_cache.c @@ -137,7 +137,7 @@ s32_t spiffs_phys_rd( #endif cp->last_access = cache->last_access; u8_t *mem = spiffs_get_cache_page(fs, cache, cp->ix); - memcpy(dst, &mem[SPIFFS_PADDR_TO_PAGE_OFFSET(fs, addr)], len); + _SPIFFS_MEMCPY(dst, &mem[SPIFFS_PADDR_TO_PAGE_OFFSET(fs, addr)], len); } else { if ((op & SPIFFS_OP_TYPE_MASK) == SPIFFS_OP_T_OBJ_LU2) { // for second layer lookup functions, we do not cache in order to prevent shredding @@ -165,7 +165,7 @@ s32_t spiffs_phys_rd( res = res2; } u8_t *mem = spiffs_get_cache_page(fs, cache, cp->ix); - memcpy(dst, &mem[SPIFFS_PADDR_TO_PAGE_OFFSET(fs, addr)], len); + _SPIFFS_MEMCPY(dst, &mem[SPIFFS_PADDR_TO_PAGE_OFFSET(fs, addr)], len); } else { // this will never happen, last resort for sake of symmetry s32_t res2 = SPIFFS_HAL_READ(fs, addr, len, dst); @@ -203,7 +203,7 @@ s32_t spiffs_phys_wr( } u8_t *mem = spiffs_get_cache_page(fs, cache, cp->ix); - memcpy(&mem[SPIFFS_PADDR_TO_PAGE_OFFSET(fs, addr)], src, len); + _SPIFFS_MEMCPY(&mem[SPIFFS_PADDR_TO_PAGE_OFFSET(fs, addr)], src, len); cache->last_access++; cp->last_access = cache->last_access; @@ -302,7 +302,7 @@ void spiffs_cache_init(spiffs *fs) { cache.cpage_use_map = 0xffffffff; cache.cpage_use_mask = cache_mask; - memcpy(fs->cache, &cache, sizeof(spiffs_cache)); + _SPIFFS_MEMCPY(fs->cache, &cache, sizeof(spiffs_cache)); spiffs_cache *c = spiffs_get_cache(fs); diff --git a/src/spiffs_hydrogen.c b/src/spiffs_hydrogen.c index b6fd093..69e0c22 100644 --- a/src/spiffs_hydrogen.c +++ b/src/spiffs_hydrogen.c @@ -86,7 +86,7 @@ s32_t SPIFFS_mount(spiffs *fs, spiffs_config *config, u8_t *work, SPIFFS_LOCK(fs); user_data = fs->user_data; memset(fs, 0, sizeof(spiffs)); - memcpy(&fs->cfg, config, sizeof(spiffs_config)); + _SPIFFS_MEMCPY(&fs->cfg, config, sizeof(spiffs_config)); fs->user_data = user_data; fs->block_count = SPIFFS_CFG_PHYS_SZ(fs) / SPIFFS_CFG_LOG_BLOCK_SZ(fs); fs->work = &work[0]; @@ -543,7 +543,18 @@ s32_t SPIFFS_write(spiffs *fs, spiffs_file fh, void *buf, s32_t len) { offset, offset_in_cpage, len); spiffs_cache *cache = spiffs_get_cache(fs); u8_t *cpage_data = spiffs_get_cache_page(fs, cache, fd->cache_page->ix); - memcpy(&cpage_data[offset_in_cpage], buf, len); +#ifdef _SPIFFS_TEST + { + intptr_t __a1 = (u8_t*)&cpage_data[offset_in_cpage]-(u8_t*)cache; + intptr_t __a2 = (u8_t*)&cpage_data[offset_in_cpage]+len-(u8_t*)cache; + intptr_t __b = sizeof(spiffs_cache) + cache->cpage_count * (sizeof(spiffs_cache_page) + SPIFFS_CFG_LOG_PAGE_SZ(fs)); + if (__a1 > __b || __a2 > __b) { + printf("FATAL OOB: CACHE_WR: memcpy to cache buffer ixs:%4ld..%4ld of %4ld\n", __a1, __a2, __b); + ERREXIT(); + } + } +#endif + _SPIFFS_MEMCPY(&cpage_data[offset_in_cpage], buf, len); fd->cache_page->size = MAX(fd->cache_page->size, offset_in_cpage + len); fd->fdoffset += len; SPIFFS_UNLOCK(fs); @@ -583,7 +594,7 @@ s32_t SPIFFS_write(spiffs *fs, spiffs_file fh, void *buf, s32_t len) { } s32_t SPIFFS_lseek(spiffs *fs, spiffs_file fh, s32_t offs, int whence) { - SPIFFS_API_DBG("%s "_SPIPRIfd " "_SPIPRIi " "_SPIPRIi "\n", __func__, fh, offs, whence); + SPIFFS_API_DBG("%s "_SPIPRIfd " "_SPIPRIi " %s\n", __func__, fh, offs, (const char* []){"SET","CUR","END","???"}[MIN(whence,3)]); SPIFFS_API_CHECK_CFG(fs); SPIFFS_API_CHECK_MOUNT(fs); SPIFFS_LOCK(fs); @@ -598,19 +609,21 @@ s32_t SPIFFS_lseek(spiffs *fs, spiffs_file fh, s32_t offs, int whence) { spiffs_fflush_cache(fs, fh); #endif - s32_t fileSize = fd->size == SPIFFS_UNDEFINED_LEN ? 0 : fd->size; + s32_t file_size = fd->size == SPIFFS_UNDEFINED_LEN ? 0 : fd->size; switch (whence) { case SPIFFS_SEEK_CUR: offs = fd->fdoffset+offs; break; case SPIFFS_SEEK_END: - offs = fileSize + offs; + offs = file_size + offs; break; } - - if (offs > fileSize) { - fd->fdoffset = fileSize; + if (offs < 0) { + SPIFFS_API_CHECK_RES_UNLOCK(fs, SPIFFS_ERR_SEEK_BOUNDS); + } + if (offs > file_size) { + fd->fdoffset = file_size; res = SPIFFS_ERR_END_OF_OBJECT; } SPIFFS_API_CHECK_RES_UNLOCK(fs, res); @@ -730,7 +743,7 @@ static s32_t spiffs_stat_pix(spiffs *fs, spiffs_page_ix pix, spiffs_file fh, spi s->pix = pix; strncpy((char *)s->name, (char *)objix_hdr.name, SPIFFS_OBJ_NAME_LEN); #if SPIFFS_OBJ_META_LEN - memcpy(s->meta, objix_hdr.meta, SPIFFS_OBJ_META_LEN); + _SPIFFS_MEMCPY(s->meta, objix_hdr.meta, SPIFFS_OBJ_META_LEN); #endif return res; @@ -1033,7 +1046,7 @@ static s32_t spiffs_read_dir_v( e->size = objix_hdr.size == SPIFFS_UNDEFINED_LEN ? 0 : objix_hdr.size; e->pix = pix; #if SPIFFS_OBJ_META_LEN - memcpy(e->meta, objix_hdr.meta, SPIFFS_OBJ_META_LEN); + _SPIFFS_MEMCPY(e->meta, objix_hdr.meta, SPIFFS_OBJ_META_LEN); #endif return SPIFFS_OK; } diff --git a/src/spiffs_nucleus.c b/src/spiffs_nucleus.c index f9ef5fe..12c9de8 100644 --- a/src/spiffs_nucleus.c +++ b/src/spiffs_nucleus.c @@ -270,7 +270,7 @@ s32_t spiffs_probe( s32_t res; u32_t paddr; spiffs dummy_fs; // create a dummy fs struct just to be able to use macros - memcpy(&dummy_fs.cfg, cfg, sizeof(spiffs_config)); + _SPIFFS_MEMCPY(&dummy_fs.cfg, cfg, sizeof(spiffs_config)); dummy_fs.block_count = 0; // Read three magics, as one block may be in an aborted erase state. @@ -942,7 +942,7 @@ s32_t spiffs_object_create( strncpy((char*)oix_hdr.name, (const char*)name, SPIFFS_OBJ_NAME_LEN); #if SPIFFS_OBJ_META_LEN if (meta) { - memcpy(oix_hdr.meta, meta, SPIFFS_OBJ_META_LEN); + _SPIFFS_MEMCPY(oix_hdr.meta, meta, SPIFFS_OBJ_META_LEN); } else { memset(oix_hdr.meta, 0xff, SPIFFS_OBJ_META_LEN); } @@ -1006,7 +1006,7 @@ s32_t spiffs_object_update_index_hdr( } #if SPIFFS_OBJ_META_LEN if (meta) { - memcpy(objix_hdr->meta, meta, SPIFFS_OBJ_META_LEN); + _SPIFFS_MEMCPY(objix_hdr->meta, meta, SPIFFS_OBJ_META_LEN); } #else (void) meta; @@ -1298,7 +1298,7 @@ s32_t spiffs_object_append(spiffs_fd *fd, u32_t offset, u8_t *data, u32_t len) { SPIFFS_CHECK_RES(res); // quick "load" of new object index page memset(fs->work, 0xff, SPIFFS_CFG_LOG_PAGE_SZ(fs)); - memcpy(fs->work, &p_hdr, sizeof(spiffs_page_header)); + _SPIFFS_MEMCPY(fs->work, &p_hdr, sizeof(spiffs_page_header)); spiffs_cb_object_event(fs, (spiffs_page_object_ix *)fs->work, SPIFFS_EV_IX_NEW, fd->obj_id, cur_objix_spix, cur_objix_pix, 0); SPIFFS_DBG("append: "_SPIPRIid" create objix page, "_SPIPRIpg":"_SPIPRIsp", written "_SPIPRIi"\n", fd->obj_id diff --git a/src/spiffs_nucleus.h b/src/spiffs_nucleus.h index 4404d8d..3d77d50 100644 --- a/src/spiffs_nucleus.h +++ b/src/spiffs_nucleus.h @@ -803,4 +803,24 @@ s32_t spiffs_page_consistency_check( s32_t spiffs_object_index_consistency_check( spiffs *fs); +// memcpy macro, +// checked in test builds, otherwise plain memcpy (unless already defined) +#ifdef _SPIFFS_TEST +#define _SPIFFS_MEMCPY(__d, __s, __l) do { \ + intptr_t __a1 = (intptr_t)((u8_t*)(__s)); \ + intptr_t __a2 = (intptr_t)((u8_t*)(__s)+(__l)); \ + intptr_t __b1 = (intptr_t)((u8_t*)(__d)); \ + intptr_t __b2 = (intptr_t)((u8_t*)(__d)+(__l)); \ + if (__a1 <= __b2 && __b1 <= __a2) { \ + printf("FATAL OVERLAP: memcpy from %lx..%lx to %lx..%lx\n", __a1, __a2, __b1, __b2); \ + ERREXIT(); \ + } \ + memcpy((__d),(__s),(__l)); \ +} while (0) +#else +#ifndef _SPIFFS_MEMCPY +#define _SPIFFS_MEMCPY(__d, __s, __l) do{memcpy((__d),(__s),(__l));}while(0) +#endif +#endif //_SPIFFS_TEST + #endif /* SPIFFS_NUCLEUS_H_ */ diff --git a/src/test/test_hydrogen.c b/src/test/test_hydrogen.c index 2245025..52126b2 100644 --- a/src/test/test_hydrogen.c +++ b/src/test/test_hydrogen.c @@ -1589,6 +1589,33 @@ TEST(lseek_read) { TEST_END + +TEST(lseek_oob) { + int res; + spiffs_file fd; + char *fname = "seekfile"; + int len = (FS_PURE_DATA_PAGES(FS) / 2) * SPIFFS_DATA_PAGE_SIZE(FS); + + fd = SPIFFS_open(FS, fname, SPIFFS_TRUNC | SPIFFS_CREAT | SPIFFS_RDWR, 0); + TEST_CHECK(fd > 0); + u8_t *refbuf = malloc(len); + memrand(refbuf, len); + res = SPIFFS_write(FS, fd, refbuf, len); + TEST_CHECK(res >= 0); + + int offs = 0; + res = SPIFFS_lseek(FS, fd, -1, SPIFFS_SEEK_SET); + TEST_CHECK_EQ(res, SPIFFS_ERR_SEEK_BOUNDS); + res = SPIFFS_lseek(FS, fd, len+1, SPIFFS_SEEK_SET); + TEST_CHECK_EQ(res, SPIFFS_ERR_END_OF_OBJECT); + free(refbuf); + SPIFFS_close(FS, fd); + + return TEST_RES_OK; +} +TEST_END + + TEST(gc_quick) { char name[32]; @@ -2459,6 +2486,7 @@ SUITE_TESTS(hydrogen_tests) ADD_TEST(lseek_modification_append) ADD_TEST(lseek_modification_append_multi) ADD_TEST(lseek_read) + ADD_TEST(lseek_oob) ADD_TEST(gc_quick) ADD_TEST(write_small_file_chunks_1) ADD_TEST(write_small_files_chunks_1) diff --git a/src/test/testrunner.h b/src/test/testrunner.h index 55babca..697fb09 100644 --- a/src/test/testrunner.h +++ b/src/test/testrunner.h @@ -155,7 +155,7 @@ int get_abort_on_error(void); int get_error_count(void); void inc_error_count(void); -void add_suites(); +void add_suites(void); void test_init(void (*on_stop)(test *t)); // returns 0 if all tests ok, -1 if any test failed, -2 on badness int run_tests(int argc, char **args); diff --git a/src/test/testsuites.c b/src/test/testsuites.c index f572287..cce5cd9 100644 --- a/src/test/testsuites.c +++ b/src/test/testsuites.c @@ -7,7 +7,7 @@ #include "testrunner.h" -void add_suites() { +void add_suites(void) { //ADD_SUITE(dev_tests); ADD_SUITE(check_tests); ADD_SUITE(hydrogen_tests);