Commit e8c9e65816 "qom: Make "info qom-tree" show children sorted"
created a memory leak, because I didn't realize
object_get_canonical_path_component()'s value needs to be freed.

Reproducer:

    $ qemu-system-x86_64 -nodefaults -display none -S -monitor stdio
    QEMU 5.0.50 monitor - type 'help' for more information
    (qemu) info qom-tree

This leaks some 4500 path components, 12-13 characters on average,
i.e. roughly 100kBytes depending on the allocator.  A couple of
hundred "info qom-tree" here, a couple of hundred there, and soon
enough we're talking about real memory.

Plug the leak.

Fixes: e8c9e65816f5dbfe18ad3b2be938d0d8192d459a
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qom/qom-hmp-cmds.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/qom/qom-hmp-cmds.c b/qom/qom-hmp-cmds.c
index 9ed8bb1c9f..aaacadacca 100644
--- a/qom/qom-hmp-cmds.c
+++ b/qom/qom-hmp-cmds.c
@@ -96,8 +96,10 @@ static void print_qom_composition(Monitor *mon, Object *obj, int indent);
 
 static int qom_composition_compare(const void *a, const void *b, void *ignore)
 {
-    return g_strcmp0(a ? object_get_canonical_path_component(a) : NULL,
-                     b ? object_get_canonical_path_component(b) : NULL);
+    g_autofree char *ac = object_get_canonical_path_component(a);
+    g_autofree char *bc = object_get_canonical_path_component(b);
+
+    return g_strcmp0(ac, bc);
 }
 
 static int insert_qom_composition_child(Object *obj, void *opaque)
-- 
2.26.2

On 7/14/20 6:01 PM, Markus Armbruster wrote:
> Commit e8c9e65816 "qom: Make "info qom-tree" show children sorted"
> created a memory leak, because I didn't realize
> object_get_canonical_path_component()'s value needs to be freed.
> 
> Reproducer:
> 
>     $ qemu-system-x86_64 -nodefaults -display none -S -monitor stdio
>     QEMU 5.0.50 monitor - type 'help' for more information
>     (qemu) info qom-tree
> 
> This leaks some 4500 path components, 12-13 characters on average,
> i.e. roughly 100kBytes depending on the allocator.  A couple of
> hundred "info qom-tree" here, a couple of hundred there, and soon
> enough we're talking about real memory.
> 
> Plug the leak.
> 
> Fixes: e8c9e65816f5dbfe18ad3b2be938d0d8192d459a
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qom/qom-hmp-cmds.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/qom/qom-hmp-cmds.c b/qom/qom-hmp-cmds.c
> index 9ed8bb1c9f..aaacadacca 100644
> --- a/qom/qom-hmp-cmds.c
> +++ b/qom/qom-hmp-cmds.c
> @@ -96,8 +96,10 @@ static void print_qom_composition(Monitor *mon, Object *obj, int indent);
>  
>  static int qom_composition_compare(const void *a, const void *b, void *ignore)
>  {
> -    return g_strcmp0(a ? object_get_canonical_path_component(a) : NULL,
> -                     b ? object_get_canonical_path_component(b) : NULL);
> +    g_autofree char *ac = object_get_canonical_path_component(a);
> +    g_autofree char *bc = object_get_canonical_path_component(b);
> +
> +    return g_strcmp0(ac, bc);
>  }
>  
>  static int insert_qom_composition_child(Object *obj, void *opaque)
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

On 7/14/20 6:01 PM, Markus Armbruster wrote:
> Commit e8c9e65816 "qom: Make "info qom-tree" show children sorted"
> created a memory leak, because I didn't realize
> object_get_canonical_path_component()'s value needs to be freed.
> 
> Reproducer:
> 
>     $ qemu-system-x86_64 -nodefaults -display none -S -monitor stdio
>     QEMU 5.0.50 monitor - type 'help' for more information
>     (qemu) info qom-tree
> 
> This leaks some 4500 path components, 12-13 characters on average,
> i.e. roughly 100kBytes depending on the allocator.  A couple of
> hundred "info qom-tree" here, a couple of hundred there, and soon
> enough we're talking about real memory.
> 
> Plug the leak.
> 
> Fixes: e8c9e65816f5dbfe18ad3b2be938d0d8192d459a
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  qom/qom-hmp-cmds.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/qom/qom-hmp-cmds.c b/qom/qom-hmp-cmds.c
> index 9ed8bb1c9f..aaacadacca 100644
> --- a/qom/qom-hmp-cmds.c
> +++ b/qom/qom-hmp-cmds.c
> @@ -96,8 +96,10 @@ static void print_qom_composition(Monitor *mon, Object *obj, int indent);
>  
>  static int qom_composition_compare(const void *a, const void *b, void *ignore)
>  {
> -    return g_strcmp0(a ? object_get_canonical_path_component(a) : NULL,
> -                     b ? object_get_canonical_path_component(b) : NULL);
> +    g_autofree char *ac = object_get_canonical_path_component(a);
> +    g_autofree char *bc = object_get_canonical_path_component(b);
> +
> +    return g_strcmp0(ac, bc);
>  }
>  
>  static int insert_qom_composition_child(Object *obj, void *opaque)
> 

Li Qiang set this too:
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg04732.html

Markus Armbruster <armbru@redhat.com> 于2020年7月15日周三 上午12:05写道：
>
> Commit e8c9e65816 "qom: Make "info qom-tree" show children sorted"
> created a memory leak, because I didn't realize
> object_get_canonical_path_component()'s value needs to be freed.
>
> Reproducer:
>
>     $ qemu-system-x86_64 -nodefaults -display none -S -monitor stdio
>     QEMU 5.0.50 monitor - type 'help' for more information
>     (qemu) info qom-tree
>
> This leaks some 4500 path components, 12-13 characters on average,
> i.e. roughly 100kBytes depending on the allocator.  A couple of
> hundred "info qom-tree" here, a couple of hundred there, and soon
> enough we're talking about real memory.
>
> Plug the leak.
>
> Fixes: e8c9e65816f5dbfe18ad3b2be938d0d8192d459a
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Li Qiang <liq3ea@gmail.com>

I have also send out this patch.
I hope the maintainer will pick your patch.

Thanks,
Li Qiang

> ---
>  qom/qom-hmp-cmds.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/qom/qom-hmp-cmds.c b/qom/qom-hmp-cmds.c
> index 9ed8bb1c9f..aaacadacca 100644
> --- a/qom/qom-hmp-cmds.c
> +++ b/qom/qom-hmp-cmds.c
> @@ -96,8 +96,10 @@ static void print_qom_composition(Monitor *mon, Object *obj, int indent);
>
>  static int qom_composition_compare(const void *a, const void *b, void *ignore)
>  {
> -    return g_strcmp0(a ? object_get_canonical_path_component(a) : NULL,
> -                     b ? object_get_canonical_path_component(b) : NULL);
> +    g_autofree char *ac = object_get_canonical_path_component(a);
> +    g_autofree char *bc = object_get_canonical_path_component(b);
> +
> +    return g_strcmp0(ac, bc);
>  }
>
>  static int insert_qom_composition_child(Object *obj, void *opaque)
> --
> 2.26.2
>
>

On 15/07/2020 17.19, Li Qiang wrote:
> Markus Armbruster <armbru@redhat.com> 于2020年7月15日周三 上午12:05写道：
>>
>> Commit e8c9e65816 "qom: Make "info qom-tree" show children sorted"
>> created a memory leak, because I didn't realize
>> object_get_canonical_path_component()'s value needs to be freed.
>>
>> Reproducer:
>>
>>     $ qemu-system-x86_64 -nodefaults -display none -S -monitor stdio
>>     QEMU 5.0.50 monitor - type 'help' for more information
>>     (qemu) info qom-tree
>>
>> This leaks some 4500 path components, 12-13 characters on average,
>> i.e. roughly 100kBytes depending on the allocator.  A couple of
>> hundred "info qom-tree" here, a couple of hundred there, and soon
>> enough we're talking about real memory.
>>
>> Plug the leak.
>>
>> Fixes: e8c9e65816f5dbfe18ad3b2be938d0d8192d459a
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Tested-by: Thomas Huth <thuth@redhat.com>


> Reviewed-by: Li Qiang <liq3ea@gmail.com>
> 
> I have also send out this patch.
> I hope the maintainer will pick your patch.

Thanks a lot for your patch, too! Normally, I'd say "first come, first
serve" and suggest that we use your patch, but since Markus' patch
series has some more patches which would break due to the contextual
differences, I think it's slightly better this time to use Markus'
version of the patch.

 Regards,
  Thomas