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.merw...@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); >
signature.asc
Description: OpenPGP digital signature