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

[Paolo Bonzini <pbonzini@redhat.com>]

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?

Thanks,

Paolo