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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
