In recent commit of bf8bd93df0 (and friends) we switched the way
we process queried command line arguments: from string lists to
virJSONValue stored in a hash table. To achieve this
qemuMonitorJSONGetCommandLineOptions() helper was introduced
which executes the "query-command-line-options" monitor command
and then calls virJSONValueArrayForeachSteal() to process the
output. The array process function is also given
qemuMonitorJSONGetCommandLineOptionsWorker() as the callback
which is called over each item of the returned array. This
callback then steals "parameters" attribute of each array iteam
storing it in the hash table, but it leaves behind "option"
attribute (because it's g_strdup()-ed). After all of this, the
callback returns 0 which is a signal to the array processing
function that the callback took ownership of the array item. But
this is not true. While it removed "parameters" it did not take
the rest ("option" for instance). And therefore, it leads to a
memory leak:

 5,347 (1,656 direct, 3,691 indirect) bytes in 69 blocks are definitely lost in loss record 2,752 of 2,794
 at 0x483BEC5: calloc (vg_replace_malloc.c:760)
 by 0x4E25A10: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.6400.5)
 by 0x4943317: virJSONValueNewObject (virjson.c:569)
 by 0x4945692: virJSONParserHandleStartMap (virjson.c:1768)
 by 0x5825A86: yajl_do_parse (in /usr/lib64/libyajl.so.2.1.0)
 by 0x4945BFA: virJSONValueFromString (virjson.c:1896)
 by 0xAF5C115: qemuMonitorJSONIOProcessLine (qemu_monitor_json.c:224)
 by 0xAF5C45E: qemuMonitorJSONIOProcess (qemu_monitor_json.c:279)
 by 0xAF4BB6C: qemuMonitorIOProcess (qemu_monitor.c:342)
 by 0xAF4C444: qemuMonitorIO (qemu_monitor.c:574)
 by 0x4FEF846: socket_source_dispatch (in /usr/lib64/libgio-2.0.so.0.6400.5)
 by 0x4E1F727: g_main_context_dispatch (in /usr/lib64/libglib-2.0.so.0.6400.5)

The callback must return 1 so that the array item is properly
freed.

Fixes: ebeff6cd57d07c89d42e191ed0085a9dd89835c5
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/qemu/qemu_monitor_json.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 4db00e284a..8480338f46 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -6357,7 +6357,7 @@ qemuMonitorJSONGetCommandLineOptionsWorker(size_t pos G_GNUC_UNUSED,
     g_hash_table_insert(options, g_strdup(name), parameters);
     parameters = NULL;
 
-    return 0;
+    return 1;
 }
 
 
-- 
2.26.2

On Mon, Dec 07, 2020 at 09:49:32 +0100, Michal Privoznik wrote:
> In recent commit of bf8bd93df0 (and friends) we switched the way
> we process queried command line arguments: from string lists to
> virJSONValue stored in a hash table. To achieve this
> qemuMonitorJSONGetCommandLineOptions() helper was introduced
> which executes the "query-command-line-options" monitor command
> and then calls virJSONValueArrayForeachSteal() to process the
> output. The array process function is also given
> qemuMonitorJSONGetCommandLineOptionsWorker() as the callback
> which is called over each item of the returned array. This
> callback then steals "parameters" attribute of each array iteam
> storing it in the hash table, but it leaves behind "option"
> attribute (because it's g_strdup()-ed). After all of this, the
> callback returns 0 which is a signal to the array processing
> function that the callback took ownership of the array item. But
> this is not true. While it removed "parameters" it did not take
> the rest ("option" for instance). And therefore, it leads to a
> memory leak:
> 
>  5,347 (1,656 direct, 3,691 indirect) bytes in 69 blocks are definitely lost in loss record 2,752 of 2,794
>  at 0x483BEC5: calloc (vg_replace_malloc.c:760)
>  by 0x4E25A10: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.6400.5)
>  by 0x4943317: virJSONValueNewObject (virjson.c:569)
>  by 0x4945692: virJSONParserHandleStartMap (virjson.c:1768)
>  by 0x5825A86: yajl_do_parse (in /usr/lib64/libyajl.so.2.1.0)
>  by 0x4945BFA: virJSONValueFromString (virjson.c:1896)
>  by 0xAF5C115: qemuMonitorJSONIOProcessLine (qemu_monitor_json.c:224)
>  by 0xAF5C45E: qemuMonitorJSONIOProcess (qemu_monitor_json.c:279)
>  by 0xAF4BB6C: qemuMonitorIOProcess (qemu_monitor.c:342)
>  by 0xAF4C444: qemuMonitorIO (qemu_monitor.c:574)
>  by 0x4FEF846: socket_source_dispatch (in /usr/lib64/libgio-2.0.so.0.6400.5)
>  by 0x4E1F727: g_main_context_dispatch (in /usr/lib64/libglib-2.0.so.0.6400.5)
> 
> The callback must return 1 so that the array item is properly
> freed.
> 
> Fixes: ebeff6cd57d07c89d42e191ed0085a9dd89835c5
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/qemu/qemu_monitor_json.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Oops, yes. In this case we definitely don't want to claim ownership of
the array element since we've just take a part of the object.

I probably misremembered the semantics of virJSONValueArrayForeachSteal
since it's a bit backwards.

Reviewed-by: Peter Krempa <pkrempa@redhat.com>