From mboxrd@z Thu Jan 1 00:00:00 1970 From: Markus Armbruster Subject: Re: [PATCH 1/2] hostmem: fix QEMU crash by 'info memdev' Date: Wed, 13 Jul 2016 13:29:34 +0200 Message-ID: <87r3ax6ajl.fsf@dusky.pond.sub.org> References: <1468383486-108169-1-git-send-email-guangrong.xiao@linux.intel.com> <945b233e-286f-fc22-8837-761e1ac2e522@redhat.com> Mime-Version: 1.0 Content-Type: text/plain Cc: Xiao Guangrong , ehabkost@redhat.com, kvm@vger.kernel.org, mst@redhat.com, gleb@kernel.org, mtosatti@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, imammedo@redhat.com, rth@twiddle.net To: Paolo Bonzini Return-path: In-Reply-To: <945b233e-286f-fc22-8837-761e1ac2e522@redhat.com> (Paolo Bonzini's message of "Wed, 13 Jul 2016 12:45:26 +0200") List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org Sender: "Qemu-devel" List-Id: kvm.vger.kernel.org Paolo Bonzini 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 >> --- >> 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? From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53036) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bNIMI-0001jV-I0 for qemu-devel@nongnu.org; Wed, 13 Jul 2016 07:29:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bNIME-0003DK-6Q for qemu-devel@nongnu.org; Wed, 13 Jul 2016 07:29:41 -0400 Received: from mx1.redhat.com ([209.132.183.28]:54287) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bNIMD-0003DE-UM for qemu-devel@nongnu.org; Wed, 13 Jul 2016 07:29:38 -0400 From: Markus Armbruster References: <1468383486-108169-1-git-send-email-guangrong.xiao@linux.intel.com> <945b233e-286f-fc22-8837-761e1ac2e522@redhat.com> Date: Wed, 13 Jul 2016 13:29:34 +0200 In-Reply-To: <945b233e-286f-fc22-8837-761e1ac2e522@redhat.com> (Paolo Bonzini's message of "Wed, 13 Jul 2016 12:45:26 +0200") Message-ID: <87r3ax6ajl.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH 1/2] hostmem: fix QEMU crash by 'info memdev' List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: Xiao Guangrong , imammedo@redhat.com, ehabkost@redhat.com, kvm@vger.kernel.org, mst@redhat.com, gleb@kernel.org, mtosatti@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, rth@twiddle.net Paolo Bonzini 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 >> --- >> 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?