[PATCH] hw/core: define stack variable to NULL to fix qtest with sanitizers

[PATCH] hw/core: define stack variable to NULL to fix qtest with sanitizers

[Daniel Hoffman]

This was the only failure preventing `make check` from passing with sanitizers
enabled on my configuration.

Signed-off-by: Daniel Hoffman <dhoff749@gmail.com>
---
 hw/core/qdev-properties.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 91632f7be9f..4caa78b7bc5 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -690,7 +690,7 @@ static void get_prop_array(Object *obj, Visitor *v, const char *name,
     uint32_t *alenptr = object_field_prop_ptr(obj, prop);
     void **arrayptr = (void *)obj + prop->arrayoffset;
     char *elem = *arrayptr;
-    GenericList *list;
+    GenericList *list = NULL;
     const size_t list_elem_size = sizeof(*list) + prop->arrayfieldsize;
     int i;
     bool ok;
-- 
2.40.1

Re: [PATCH] hw/core: define stack variable to NULL to fix qtest with sanitizers

[Philippe Mathieu-Daudé]

(Cc'ing QAPI maintainer)

On 24/11/23 02:53, Daniel Hoffman wrote:
> This was the only failure preventing `make check` from passing with sanitizers
> enabled on my configuration.

IIUC this is due to visit_start_list() which expects a NULL list,
see qapi/qapi-visit-core.c:

bool visit_start_list(Visitor *v, const char *name, GenericList **list,
                       size_t size, Error **errp)
{
     bool ok;

     assert(!list || size >= sizeof(GenericList));

which is well defined in its declaration:

/*
  * Start visiting a list.
  *
  * @name expresses the relationship of this list to its parent
  * container; see the general description of @name above.
  *
  * @list must be non-NULL for a real walk, in which case @size
  * determines how much memory an input or clone visitor will allocate
  * into *@list (at least sizeof(GenericList)).  Some visitors also
  * allow @list to be NULL for a virtual walk, in which case @size is
  * ignored.
  ...

With the patch description improved:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> Signed-off-by: Daniel Hoffman <dhoff749@gmail.com>
> ---
>   hw/core/qdev-properties.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 91632f7be9f..4caa78b7bc5 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -690,7 +690,7 @@ static void get_prop_array(Object *obj, Visitor *v, const char *name,
>       uint32_t *alenptr = object_field_prop_ptr(obj, prop);
>       void **arrayptr = (void *)obj + prop->arrayoffset;
>       char *elem = *arrayptr;
> -    GenericList *list;
> +    GenericList *list = NULL;
>       const size_t list_elem_size = sizeof(*list) + prop->arrayfieldsize;
>       int i;
>       bool ok;

Re: [PATCH] hw/core: define stack variable to NULL to fix qtest with sanitizers

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> (Cc'ing QAPI maintainer)
>
> On 24/11/23 02:53, Daniel Hoffman wrote:
>> This was the only failure preventing `make check` from passing with sanitizers
>> enabled on my configuration.
>
> IIUC this is due to visit_start_list() which expects a NULL list,
> see qapi/qapi-visit-core.c:
>
> bool visit_start_list(Visitor *v, const char *name, GenericList **list,
>                       size_t size, Error **errp)
> {
>     bool ok;
>
>     assert(!list || size >= sizeof(GenericList));

This asserts either "if real walk, then size is sane".

>
> which is well defined in its declaration:
>
> /*
>  * Start visiting a list.
>  *
>  * @name expresses the relationship of this list to its parent
>  * container; see the general description of @name above.
>  *
>  * @list must be non-NULL for a real walk, in which case @size
>  * determines how much memory an input or clone visitor will allocate
>  * into *@list (at least sizeof(GenericList)).  Some visitors also
>  * allow @list to be NULL for a virtual walk, in which case @size is
>  * ignored.
>  ...

Mind the number of *!

The function contract talks about @list, which is GenericList **.

get_prop_array() passes &list, where list is GenericList *.  &list is
non-null even before the patch.

The patch initializes @list to empty.

> With the patch description improved:

Specifically, show how things fail under sanitation.

> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
>> Signed-off-by: Daniel Hoffman <dhoff749@gmail.com>
>> ---
>>   hw/core/qdev-properties.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
>> index 91632f7be9f..4caa78b7bc5 100644
>> --- a/hw/core/qdev-properties.c
>> +++ b/hw/core/qdev-properties.c
>> @@ -690,7 +690,7 @@ static void get_prop_array(Object *obj, Visitor *v, const char *name,
>>      uint32_t *alenptr = object_field_prop_ptr(obj, prop);
>>      void **arrayptr = (void *)obj + prop->arrayoffset;
>>      char *elem = *arrayptr;
>> -    GenericList *list;
>> +    GenericList *list = NULL;
>>      const size_t list_elem_size = sizeof(*list) + prop->arrayfieldsize;
>>      int i;
>>      bool ok;

        if (!visit_start_list(v, name, &list, list_elem_size, errp)) {
            return;
        }

Re: [PATCH] hw/core: define stack variable to NULL to fix qtest with sanitizers

[Markus Armbruster]

Daniel, please have a look at Kevin's patch:

    Subject: [PATCH for-8.2 1/2] qdev: Fix crash in array property getter
    Date: Tue, 21 Nov 2023 18:34:15 +0100 (2 days, 20 hours, 26 minutes ago)
    Message-ID: <20231121173416.346610-2-kwolf@redhat.com>
    https://lore.kernel.org/qemu-devel/20231121173416.346610-2-kwolf@redhat.com/

Does it fix your sanitizer run?

Re: [PATCH] hw/core: define stack variable to NULL to fix qtest with sanitizers

[Dan Hoffman]

Yes, that fixes my issue. I was receiving two errors with the sanitizers:
1. UBsan complaining that the (garbage) value didn't have the required
alignment of the type
2. ASan complaining about some memory failure by read/write/accessing it

On Fri, Nov 24, 2023 at 8:02 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> Daniel, please have a look at Kevin's patch:
>
>     Subject: [PATCH for-8.2 1/2] qdev: Fix crash in array property getter
>     Date: Tue, 21 Nov 2023 18:34:15 +0100 (2 days, 20 hours, 26 minutes ago)
>     Message-ID: <20231121173416.346610-2-kwolf@redhat.com>
>     https://lore.kernel.org/qemu-devel/20231121173416.346610-2-kwolf@redhat.com/
>
> Does it fix your sanitizer run?
>