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 <[email protected]> > --- > 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
