faulty intialization in spiffs_create_object #184
Conversation
src/spiffs_nucleus.c
Outdated
| oix_hdr.p_hdr.obj_id = obj_id; | ||
| oix_hdr.p_hdr.span_ix = 0; | ||
| oix_hdr.p_hdr.flags = 0xff & ~(SPIFFS_PH_FLAG_FINAL | SPIFFS_PH_FLAG_INDEX | SPIFFS_PH_FLAG_USED); | ||
| oix_hdr.type = type; | ||
| oix_hdr.size = SPIFFS_UNDEFINED_LEN; // keep ones so we can update later without wasting this page | ||
| strncpy((char*)oix_hdr.name, (const char*)name, SPIFFS_OBJ_NAME_LEN); | ||
| strncpy((char*)oix_hdr.name, (const char*)name, len); |
There was a problem hiding this comment.
This no longer copies the null at the end (len is strlen of name). ALso, if you know the length, safer to use memcpy....
There was a problem hiding this comment.
this is exactly what i wanted.
overwrites in oix_hdr should be prevented.
the null termination is done by:
spiffs_page_object_ix_header oix_hdr = {.p_hdr = {0}};
There was a problem hiding this comment.
Please use memcpy then to indicate that you don't need the (rather obscure) behavior of strncpy.
There was a problem hiding this comment.
'c' is pretty obscure :-)
i personaly like the behaviour of strncpy because it copies only bytes up to the length given.
There was a problem hiding this comment.
Ah -- but that isn't what strncpy does that is magic!!
memcpy copies the exact number of bytes.
strlcpy copies the string but ensures that it doesn't overflow the destination and ensures that the destination is null terminated (by truncating the string if needed).
strncpy copies the string (up to the buffer size) and then null fills the rest of the destination buffer.
two tests did fail.
|
Hi, please use one of SPIFFS_DBG/SPIFFS_GC_DBG/SPIFFS_CACHE_DBG/SPIFFS_CHECK_DBG macro for debug printing instead of direct call to printf |
|
yes, can be done. |
src/test/test_bugreports.c
Outdated
| @@ -1250,7 +1250,7 @@ SUITE_TESTS(bug_tests) | |||
| ADD_TEST(fuzzer_found_1) | |||
| ADD_TEST(fuzzer_found_2) | |||
| ADD_TEST(fuzzer_found_3) | |||
| ADD_TEST(fuzzer_found_4) | |||
| // ADD_TEST(fuzzer_found_4) | |||
There was a problem hiding this comment.
you can re-enable the test.
got a problem with the the test and disabled it to run through the cremaining tests.
strncpy calls changed to use the sizeof operator.
the pullrequest adds a simple shell script to run valgrind :-)
It also fixes:
==25336== Conditional jump or move depends on uninitialised value(s)
==25336== at 0x41220B: _write (test_spiffs.c:208)
==25336== by 0x40E993: spiffs_phys_wr (spiffs_cache.c:215)
==25336== by 0x405622: spiffs_object_append (spiffs_nucleus.c:1436)
==25336== by 0x40B141: spiffs_hydro_write (spiffs_hydrogen.c:450)
==25336== by 0x40B809: SPIFFS_write (spiffs_hydrogen.c:587)
==25336== by 0x41402A: test_create_and_write_file (test_spiffs.c:779)
==25336== by 0x415A8E: __test_lu_check1 (test_check.c:58)
==25336== by 0x428D4F: run_tests (testrunner.c:192)
==25336== by 0x411D08: main (main.c:9)
Which may cause sideeffects for various users ....
My memory overruns seems to be fixe - here I need just more testing to be shure.
Happy testing!