| Message ID | 1539981546-10596-6-git-send-email-Liam.Merwick@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | off-by-one and NULL pointer accesses detected by static analysis | expand |
On 19.10.18 22:39, Liam Merwick wrote: > The calls to find_mapping_for_cluster() may return NULL but it > isn't always checked for before dereferencing the value returned. > Additionally, add some asserts to cover cases where NULL can't > be returned but which might not be obvious at first glance. > > Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com> > --- > block/vvfat.c | 33 ++++++++++++++++++++++++++++----- > 1 file changed, 28 insertions(+), 5 deletions(-) > > diff --git a/block/vvfat.c b/block/vvfat.c > index fc41841a5c3c..19f6725054a0 100644 > --- a/block/vvfat.c > +++ b/block/vvfat.c > @@ -100,6 +100,7 @@ static inline void array_free(array_t* array) > /* does not automatically grow */ > static inline void* array_get(array_t* array,unsigned int index) { > assert(index < array->next); > + assert(array->pointer); > return array->pointer + index * array->item_size; > } > > @@ -108,8 +109,7 @@ static inline int array_ensure_allocated(array_t* array, int index) > if((index + 1) * array->item_size > array->size) { > int new_size = (index + 32) * array->item_size; > array->pointer = g_realloc(array->pointer, new_size); > - if (!array->pointer) > - return -1; > + assert(array->pointer); It would make sense to make this function not return any value (because it just always returns 0 now), but I fully understand if you don't want to mess around with vvfat more than you have to. (Neither do I.) > memset(array->pointer + array->size, 0, new_size - array->size); > array->size = new_size; > array->next = index + 1; > @@ -2261,6 +2261,9 @@ static mapping_t* insert_mapping(BDRVVVFATState* s, > } > if (index >= s->mapping.next || mapping->begin > begin) { > mapping = array_insert(&(s->mapping), index, 1); > + if (mapping == NULL) { > + return NULL; > + } array_insert() will never return NULL. > mapping->path = NULL; > adjust_mapping_indices(s, index, +1); > } > @@ -2428,6 +2431,9 @@ static int commit_direntries(BDRVVVFATState* s, > direntry_t* direntry = array_get(&(s->directory), dir_index); > uint32_t first_cluster = dir_index == 0 ? 0 : begin_of_direntry(direntry); > mapping_t* mapping = find_mapping_for_cluster(s, first_cluster); > + if (mapping == NULL) { > + return -1; > + } This should be moved below the declarations that still follow here. > > int factor = 0x10 * s->sectors_per_cluster; > int old_cluster_count, new_cluster_count; [...] > @@ -3193,6 +3215,7 @@ static int enable_write_target(BlockDriverState *bs, Error **errp) > > backing = bdrv_new_open_driver(&vvfat_write_target, NULL, BDRV_O_ALLOW_RDWR, > &error_abort); > + assert(backing); > *(void**) backing->opaque = s; I personally wouldn't use an assert() here because it's clear that the value is dereferenced immediately, so that is the assertion that it is non-NULL, but I won't give too much of a fight. The thing is, I believe we should write code for humans, not machines. Fixing machines to understand what we produce is possible -- fixing humans is more difficult. On top of that, it would be a bug if NULL is returned and it would be good if a static analyzer could catch that case. Just fully silencing it with assert() is not ideal. Ideal would be if it would know that setting &error_abort to any value crashes the program, and could thus infer whether this function will actually ever get to return NULL when &error_abort has been passed to it. Max > > bdrv_set_backing_hd(s->bs, backing, &error_abort); >
On 05/11/18 00:19, Max Reitz wrote: > On 19.10.18 22:39, Liam Merwick wrote: >> The calls to find_mapping_for_cluster() may return NULL but it >> isn't always checked for before dereferencing the value returned. >> Additionally, add some asserts to cover cases where NULL can't >> be returned but which might not be obvious at first glance. >> >> Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com> >> --- >> block/vvfat.c | 33 ++++++++++++++++++++++++++++----- >> 1 file changed, 28 insertions(+), 5 deletions(-) >> >> diff --git a/block/vvfat.c b/block/vvfat.c >> index fc41841a5c3c..19f6725054a0 100644 >> --- a/block/vvfat.c >> +++ b/block/vvfat.c >> @@ -100,6 +100,7 @@ static inline void array_free(array_t* array) >> /* does not automatically grow */ >> static inline void* array_get(array_t* array,unsigned int index) { >> assert(index < array->next); >> + assert(array->pointer); >> return array->pointer + index * array->item_size; >> } >> >> @@ -108,8 +109,7 @@ static inline int array_ensure_allocated(array_t* array, int index) >> if((index + 1) * array->item_size > array->size) { >> int new_size = (index + 32) * array->item_size; >> array->pointer = g_realloc(array->pointer, new_size); >> - if (!array->pointer) >> - return -1; >> + assert(array->pointer); > > It would make sense to make this function not return any value (because > it just always returns 0 now), but I fully understand if you don't want > to mess around with vvfat more than you have to. (Neither do I.) It had occurred to me too but wasn't sure if it'd be preferred to roll that change in. 3 of the 4 callers ignored the return value already, so I bit the bullet and made the change. > >> memset(array->pointer + array->size, 0, new_size - array->size); >> array->size = new_size; >> array->next = index + 1; >> @@ -2261,6 +2261,9 @@ static mapping_t* insert_mapping(BDRVVVFATState* s, >> } >> if (index >= s->mapping.next || mapping->begin > begin) { >> mapping = array_insert(&(s->mapping), index, 1); >> + if (mapping == NULL) { >> + return NULL; >> + } > > array_insert() will never return NULL. Removed. > >> mapping->path = NULL; >> adjust_mapping_indices(s, index, +1); >> } >> @@ -2428,6 +2431,9 @@ static int commit_direntries(BDRVVVFATState* s, >> direntry_t* direntry = array_get(&(s->directory), dir_index); >> uint32_t first_cluster = dir_index == 0 ? 0 : begin_of_direntry(direntry); >> mapping_t* mapping = find_mapping_for_cluster(s, first_cluster); >> + if (mapping == NULL) { >> + return -1; >> + } > > This should be moved below the declarations that still follow here. Done. (It resulted in a bit more code rearranging and I had to fix two checkpatch warnings in existing code) > >> >> int factor = 0x10 * s->sectors_per_cluster; >> int old_cluster_count, new_cluster_count; > > [...] > >> @@ -3193,6 +3215,7 @@ static int enable_write_target(BlockDriverState *bs, Error **errp) >> >> backing = bdrv_new_open_driver(&vvfat_write_target, NULL, BDRV_O_ALLOW_RDWR, >> &error_abort); >> + assert(backing); >> *(void**) backing->opaque = s; > > I personally wouldn't use an assert() here because it's clear that the > value is dereferenced immediately, so that is the assertion that it is > non-NULL, but I won't give too much of a fight. > > The thing is, I believe we should write code for humans, not machines. > Fixing machines to understand what we produce is possible -- fixing > humans is more difficult. > > On top of that, it would be a bug if NULL is returned and it would be > good if a static analyzer could catch that case. Just fully silencing > it with assert() is not ideal. Ideal would be if it would know that > setting &error_abort to any value crashes the program, and could thus > infer whether this function will actually ever get to return NULL when > &error_abort has been passed to it. > I'm investigating if the tool's config file syntax can describe that error_handle_fatal() exits when particular error_xxx parameters are passed. I'll drop that assert in any case. Regards, Liam > Max > >> >> bdrv_set_backing_hd(s->bs, backing, &error_abort); >> > >
diff --git a/block/vvfat.c b/block/vvfat.c index fc41841a5c3c..19f6725054a0 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -100,6 +100,7 @@ static inline void array_free(array_t* array) /* does not automatically grow */ static inline void* array_get(array_t* array,unsigned int index) { assert(index < array->next); + assert(array->pointer); return array->pointer + index * array->item_size; } @@ -108,8 +109,7 @@ static inline int array_ensure_allocated(array_t* array, int index) if((index + 1) * array->item_size > array->size) { int new_size = (index + 32) * array->item_size; array->pointer = g_realloc(array->pointer, new_size); - if (!array->pointer) - return -1; + assert(array->pointer); memset(array->pointer + array->size, 0, new_size - array->size); array->size = new_size; array->next = index + 1; @@ -2261,6 +2261,9 @@ static mapping_t* insert_mapping(BDRVVVFATState* s, } if (index >= s->mapping.next || mapping->begin > begin) { mapping = array_insert(&(s->mapping), index, 1); + if (mapping == NULL) { + return NULL; + } mapping->path = NULL; adjust_mapping_indices(s, index, +1); } @@ -2428,6 +2431,9 @@ static int commit_direntries(BDRVVVFATState* s, direntry_t* direntry = array_get(&(s->directory), dir_index); uint32_t first_cluster = dir_index == 0 ? 0 : begin_of_direntry(direntry); mapping_t* mapping = find_mapping_for_cluster(s, first_cluster); + if (mapping == NULL) { + return -1; + } int factor = 0x10 * s->sectors_per_cluster; int old_cluster_count, new_cluster_count; @@ -2494,6 +2500,9 @@ DLOG(fprintf(stderr, "commit_direntries for %s, parent_mapping_index %d\n", mapp direntry = array_get(&(s->directory), first_dir_index + i); if (is_directory(direntry) && !is_dot(direntry)) { mapping = find_mapping_for_cluster(s, first_cluster); + if (mapping == NULL) { + return -1; + } assert(mapping->mode & MODE_DIRECTORY); ret = commit_direntries(s, first_dir_index + i, array_index(&(s->mapping), mapping)); @@ -2522,6 +2531,10 @@ static int commit_one_file(BDRVVVFATState* s, assert(offset < size); assert((offset % s->cluster_size) == 0); + if (mapping == NULL) { + return -1; + } + for (i = s->cluster_size; i < offset; i += s->cluster_size) c = modified_fat_get(s, c); @@ -2668,8 +2681,12 @@ static int handle_renames_and_mkdirs(BDRVVVFATState* s) if (commit->action == ACTION_RENAME) { mapping_t* mapping = find_mapping_for_cluster(s, commit->param.rename.cluster); - char* old_path = mapping->path; + char *old_path; + if (mapping == NULL) { + return -1; + } + old_path = mapping->path; assert(commit->path); mapping->path = commit->path; if (rename(old_path, mapping->path)) @@ -2690,10 +2707,15 @@ static int handle_renames_and_mkdirs(BDRVVVFATState* s) direntry_t* d = direntry + i; if (is_file(d) || (is_directory(d) && !is_dot(d))) { + int l; + char *new_path; mapping_t* m = find_mapping_for_cluster(s, begin_of_direntry(d)); - int l = strlen(m->path); - char* new_path = g_malloc(l + diff + 1); + if (m == NULL) { + return -1; + } + l = strlen(m->path); + new_path = g_malloc(l + diff + 1); assert(!strncmp(m->path, mapping->path, l2)); @@ -3193,6 +3215,7 @@ static int enable_write_target(BlockDriverState *bs, Error **errp) backing = bdrv_new_open_driver(&vvfat_write_target, NULL, BDRV_O_ALLOW_RDWR, &error_abort); + assert(backing); *(void**) backing->opaque = s; bdrv_set_backing_hd(s->bs, backing, &error_abort);
The calls to find_mapping_for_cluster() may return NULL but it isn't always checked for before dereferencing the value returned. Additionally, add some asserts to cover cases where NULL can't be returned but which might not be obvious at first glance. Signed-off-by: Liam Merwick <Liam.Merwick@oracle.com> --- block/vvfat.c | 33 ++++++++++++++++++++++++++++----- 1 file changed, 28 insertions(+), 5 deletions(-)