Re: [PATCH 1/2] hostmem: fix QEMU crash by 'info memdev'

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 13/07/2016 06:18, Xiao Guangrong wrote:
>> 
>> Return MAX_NODES under this case to fix this bug
>> 
>> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
>> ---
>>  backends/hostmem.c | 22 ++++++++++++++--------
>>  1 file changed, 14 insertions(+), 8 deletions(-)
>> 
>> diff --git a/backends/hostmem.c b/backends/hostmem.c
>> index 6e28be1..8dede4d 100644
>> --- a/backends/hostmem.c
>> +++ b/backends/hostmem.c
>> @@ -64,6 +64,14 @@ out:
>>      error_propagate(errp, local_err);
>>  }
>>  
>> +static uint16List **host_memory_append_node(uint16List **node,
>> +                                            unsigned long value)
>> +{
>> +     *node = g_malloc0(sizeof(**node));
>> +     (*node)->value = value;
>> +     return &(*node)->next;
>> +}
>> +
>>  static void
>>  host_memory_backend_get_host_nodes(Object *obj, Visitor *v, const char *name,
>>                                     void *opaque, Error **errp)
>> @@ -74,25 +82,23 @@ host_memory_backend_get_host_nodes(Object *obj, Visitor *v, const char *name,
>>      unsigned long value;
>>  
>>      value = find_first_bit(backend->host_nodes, MAX_NODES);
>> +
>> +    node = host_memory_append_node(node, value);
>> +
>>      if (value == MAX_NODES) {
>> -        return;
>> +        goto out;
>>      }
>>  
>> -    *node = g_malloc0(sizeof(**node));
>> -    (*node)->value = value;
>> -    node = &(*node)->next;
>> -
>>      do {
>>          value = find_next_bit(backend->host_nodes, MAX_NODES, value + 1);
>>          if (value == MAX_NODES) {
>>              break;
>>          }
>>  
>> -        *node = g_malloc0(sizeof(**node));
>> -        (*node)->value = value;
>> -        node = &(*node)->next;
>> +        node = host_memory_append_node(node, value);
>>      } while (true);
>>  
>> +out:
>>      visit_type_uint16List(v, name, &host_nodes, errp);
>
> This function is leaking host_nodes, so you need a
>
> qapi_free_uint16List(head);
>
> here (and saving the head pointer on the first call to
> host_memory_append_node).  The bug is preexisting.
>
> I'm curious about one thing.  Eric/Markus, it would be nice to open code
> the visit of the list with
>
>     visit_start_list(v, name, NULL, 0, &err);
>     if (err) {
>         goto out;
>     }
>     ...
>     visit_type_uint16(v, name, &value, &err);
>     visit_next_list(v, NULL, 0);
>     ...
>     visit_end_list(v, NULL);
>
> We know here that on the other side there is an output visitor.
> However, it doesn't work because visit_next_list asserts that tail ==
> NULL.  Would it be easy to support this idiom, and would it make sense
> to extend it to other kinds of visitor?

visit_next_list() asserts tail != NULL because to protect the
next_list() method.  qmp_output_next_list() dereferences tail.

Note that you don't have to call visit_next_list() in a virtual visit.
For an example, see prop_get_fdt().  Good enough already?