Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

faulty intialization in spiffs_create_object #184

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

faulty intialization in spiffs_create_object #184

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8cc82cf
Te function "spiffs_create_object" did use unitialized varaibles acco…
Oct 30, 2017
c04d9ec
add shell-script to run valground wit hsome common options.
Oct 31, 2017
e17a749
cleaning up the error handling.
Dec 1, 2017
a5d3feb
stupid test for the erase block size.
sensslen Dec 4, 2017
a282626
faulte erase address calculation.
sensslen Dec 4, 2017
9088f67
show spiffs result.
sensslen Dec 4, 2017
90c3bd9
re-enabling two test-cases.
sensslen Dec 5, 2017
c6b3c33
last changes. mainly initialisation and sanity checks.
sensslen Jan 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions build/run_valgrind.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
#!/bin/bash
valgrind --show-reachable=yes --track-origins=yes --leak-check=full ./linux_spiffs_test &> valgrind_output.txt

8 changes: 5 additions & 3 deletions src/spiffs_gc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,9 +107,11 @@ s32_t spiffs_gc_quick(
// Checks if garbage collecting is necessary. If so a candidate block is found,
// cleansed and erased
s32_t spiffs_gc_check(
spiffs *fs,
u32_t len) {
s32_t res;
spiffs *fs,
u32_t len
)
{
s32_t res = SPIFFS_OK;
s32_t free_pages =
(SPIFFS_PAGES_PER_BLOCK(fs) - SPIFFS_OBJ_LOOKUP_PAGES(fs)) * (fs->block_count-2)
- fs->stats_p_allocated - fs->stats_p_deleted;
Expand Down
32 changes: 18 additions & 14 deletions src/spiffs_nucleus.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -451,8 +451,9 @@ s32_t spiffs_obj_lu_find_free(
spiffs_block_ix starting_block,
int starting_lu_entry,
spiffs_block_ix *block_ix,
int *lu_entry) {
s32_t res;
int *lu_entry)
{
s32_t res = SPIFFS_OK;
if (!fs->cleaning && fs->free_blocks < 2) {
res = spiffs_gc_quick(fs, 0);
if (res == SPIFFS_ERR_NO_DELETED_BLOCKS) {
Expand Down Expand Up @@ -910,16 +911,18 @@ s32_t spiffs_page_delete(
#if !SPIFFS_READ_ONLY
// Create an object index header page with empty index and undefined length
s32_t spiffs_object_create(
spiffs *fs,
spiffs_obj_id obj_id,
const u8_t name[],
const u8_t meta[],
spiffs_obj_type type,
spiffs_page_ix *objix_hdr_pix) {
s32_t res = SPIFFS_OK;
spiffs_block_ix bix;
spiffs_page_object_ix_header oix_hdr;
int entry;
spiffs * fs,
spiffs_obj_id obj_id,
const u8_t name[],
const u8_t meta[],
spiffs_obj_type type,
spiffs_page_ix * objix_hdr_pix
)
{
s32_t res = SPIFFS_OK;
spiffs_block_ix bix = 0;
spiffs_page_object_ix_header oix_hdr = {.p_hdr = {0}};
int entry = 0;

res = spiffs_gc_check(fs, SPIFFS_DATA_PAGE_SIZE(fs));
SPIFFS_CHECK_RES(res);
Expand All @@ -939,12 +942,13 @@ s32_t spiffs_object_create(
fs->stats_p_allocated++;

// write empty object index page
size_t const len = MIN( strlen((const char *)name), sizeof(oix_hdr.name));
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This no longer copies the null at the end (len is strlen of name). ALso, if you know the length, safer to use memcpy....

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}};

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use memcpy then to indicate that you don't need the (rather obscure) behavior of strncpy.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'c' is pretty obscure :-)
i personaly like the behaviour of strncpy because it copies only bytes up to the length given.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

#if SPIFFS_OBJ_META_LEN
if (meta) {
_SPIFFS_MEMCPY(oix_hdr.meta, meta, SPIFFS_OBJ_META_LEN);
Expand Down Expand Up @@ -1051,7 +1055,7 @@ void spiffs_cb_object_event(
#endif
// update index caches in all file descriptors
spiffs_obj_id obj_id = obj_id_raw & ~SPIFFS_OBJ_ID_IX_FLAG;
u32_t i;
u32_t i = 0;
spiffs_fd *fds = (spiffs_fd *)fs->fd_space;
SPIFFS_DBG(" CALLBACK %s obj_id:"_SPIPRIid" spix:"_SPIPRIsp" npix:"_SPIPRIpg" nsz:"_SPIPRIi"\n", (const char *[]){"UPD", "NEW", "DEL", "MOV", "HUP","???"}[MIN(ev,5)],
obj_id_raw, spix, new_pix, new_size);
Expand Down
2 changes: 1 addition & 1 deletion src/test/test_bugreports.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -628,7 +628,7 @@ static int run_fuzz_test(FILE *f, int maxfds, int debuglog) {

for (i = 0; i < 8; i++) {
char buff[128];
sprintf(buff, "%dfile%d.xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxasdasdasdadxxxxxxxxxxxxxxxxxxx", i, i);
snprintf(buff, sizeof(buff), "%dfile%d.xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxasdasdasdadxxxxxxxxxxxxxxxxxxx", i, i);
buff[9 + 2 * i] = 0;
filename[i] = strdup(buff);
}
Expand Down
4 changes: 2 additions & 2 deletions src/test/test_spiffs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ static int mkpath(const char *path, mode_t mode) {
//
//
char *make_test_fname(const char *name) {
sprintf(_path, "%s/%s", TEST_PATH, name);
snprintf(_path, sizeof(_path), "%s/%s", TEST_PATH, name);
return _path;
}

Expand All @@ -132,7 +132,7 @@ void clear_test_path() {
if (dp != NULL) {
while ((ep = readdir(dp))) {
if (ep->d_name[0] != '.') {
sprintf(_path, "%s/%s", TEST_PATH, ep->d_name);
snprintf(_path, sizeof(_path), "%s/%s", TEST_PATH, ep->d_name);
remove(_path);
}
}
Expand Down