[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v4 5/8] block: Fix potential Null pointer derefe
From: |
Max Reitz |
Subject: |
Re: [Qemu-block] [PATCH v4 5/8] block: Fix potential Null pointer dereferences in vvfat.c |
Date: |
Mon, 5 Nov 2018 01:19:48 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 |
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
- Re: [Qemu-block] [PATCH v4 5/8] block: Fix potential Null pointer dereferences in vvfat.c,
Max Reitz <=