Re: [Qemu-devel] [PATCH v4 5/8] block: Fix potential Null pointer dereferences in vvfat.c

[Max Reitz]

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 <address@hidden>
> ---
>  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