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;
        }