diff mbox

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

Message ID 1468383486-108169-1-git-send-email-guangrong.xiao@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Xiao Guangrong July 13, 2016, 4:18 a.m. UTC
'info memdev' crashes QEMU:
   (qemu) info memdev
   Unexpected error in parse_str() at qapi/string-input-visitor.c:111:
   Parameter 'null' expects an int64 value or range
It is caused by null uint16List is returned if 'host-nodes' is the default
value

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(-)

Comments

Paolo Bonzini July 13, 2016, 10:45 a.m. UTC | #1
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
Markus Armbruster July 13, 2016, 11:29 a.m. UTC | #2
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?
Paolo Bonzini July 13, 2016, 11:37 a.m. UTC | #3
On 13/07/2016 13:29, Markus Armbruster wrote:
>> > 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?

Yes, definitely!  I'm queueing Guangrong's patch because it fixes a
crash and the leak existed before, but without next_list we can indeed
visit a "virtual" list and fix the leak.  It can be done during the -rc
period.

Paolo
Xiao Guangrong July 15, 2016, 6:56 a.m. UTC | #4
On 07/13/2016 07:37 PM, Paolo Bonzini wrote:
>
>
> On 13/07/2016 13:29, Markus Armbruster wrote:
>>>> 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?
>
> Yes, definitely!  I'm queueing Guangrong's patch because it fixes a
> crash and the leak existed before, but without next_list we can indeed
> visit a "virtual" list and fix the leak.  It can be done during the -rc
> period.

So you want to build uint16List list and save it as a "virtual" list in
host_memory_backend_get_host_nodes(), then its caller can directly fetch
this 'virtual' list from the visit?
Eric Blake July 15, 2016, 5:16 p.m. UTC | #5
On 07/15/2016 12:56 AM, Xiao Guangrong wrote:

>>> 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?
>>
>> Yes, definitely!  I'm queueing Guangrong's patch because it fixes a
>> crash and the leak existed before, but without next_list we can indeed
>> visit a "virtual" list and fix the leak.  It can be done during the -rc
>> period.
> 
> So you want to build uint16List list and save it as a "virtual" list in
> host_memory_backend_get_host_nodes(), then its caller can directly fetch
> this 'virtual' list from the visit?

With a virtual list visit, you don't even need a uint16List object.
Merely call visit_start_list(NULL) to start the list with no matching
uint16List, then visit_type_int16() for each list element (note no
visit_next_list() calls), then visit_end_list().
diff mbox

Patch

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