* [Qemu-devel] [PATCH] qmp: Fix memory leak in migrate_params_test_apply
@ 2019-08-27 8:05 Ying Fang
2019-08-27 8:30 ` no-reply
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Ying Fang @ 2019-08-27 8:05 UTC (permalink / raw)
To: qemu-devel, dgilbert, quintela
Cc: lcf.lichaofeng, Ying Fang, zhanghailiang, zhouyibo3
Address Sanitizer shows memory leak in migrate_params_test_apply
migration/migration.c:1253 and the stack is as bellow:
Direct leak of 45 byte(s) in 9 object(s) allocated from:
#0 0xffffbd7fc1db in __interceptor_malloc (/lib64/libasan.so.4+0xd31db)
#1 0xffffbd514163 in g_malloc (/lib64/libglib-2.0.so.0+0x57163)
#2 0xffffbd52f43b in g_strdup (/lib64/libglib-2.0.so.0+0x7243b)
#3 0xaaaadfa4d623 in migrate_params_test_apply migration/migration.c:1253
#4 0xaaaadfa4d623 in qmp_migrate_set_parameters migration/migration.c:1422
#5 0xaaaadfa963f3 in hmp_migrate_set_parameter monitor/hmp-cmds.c:1867
#6 0xaaaadfa8afe3 in handle_hmp_command monitor/hmp.c:1082
#7 0xaaaadf479c57 in qmp_human_monitor_command monitor/misc.c:140
#8 0xaaaadfadf87b in qmp_marshal_human_monitor_command qapi/qapi-commands-misc.c:1024
#9 0xaaaadfc7797b in do_qmp_dispatch qapi/qmp-dispatch.c:131
#10 0xaaaadfc7797b in qmp_dispatch qapi/qmp-dispatch.c:174
#11 0xaaaadfa84fff in monitor_qmp_dispatch monitor/qmp.c:120
#12 0xaaaadfa85bbf in monitor_qmp_bh_dispatcher monitor/qmp.c:209
#13 0xaaaadfd2228f in aio_bh_call util/async.c:89
#14 0xaaaadfd2228f in aio_bh_poll util/async.c:117
#15 0xaaaadfd29bc3 in aio_dispatch util/aio-posix.c:459
#16 0xaaaadfd21ff7 in aio_ctx_dispatch util/async.c:260
#17 0xffffbd50e2f7 in g_main_context_dispatch (/lib64/libglib-2.0.so.0+0x512f7)
#18 0xaaaadfd278d7 in glib_pollfds_poll util/main-loop.c:218
#19 0xaaaadfd278d7 in os_host_main_loop_wait util/main-loop.c:241
#20 0xaaaadfd278d7 in main_loop_wait util/main-loop.c:517
#21 0xaaaadf67b5e7 in main_loop vl.c:1806
#22 0xaaaadf15d453 in main vl.c:4488
Cc: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Ying Fang <fangying1@huawei.com>
---
migration/migration.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/migration/migration.c b/migration/migration.c
index 8b9f2fe30a..05e44ff7cc 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1250,11 +1250,17 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
if (params->has_tls_creds) {
assert(params->tls_creds->type == QTYPE_QSTRING);
+ if (dest->tls_creds) {
+ g_free(dest->tls_creds);
+ }
dest->tls_creds = g_strdup(params->tls_creds->u.s);
}
if (params->has_tls_hostname) {
assert(params->tls_hostname->type == QTYPE_QSTRING);
+ if (dest->tls_hostname) {
+ g_free(dest->tls_hostname);
+ }
dest->tls_hostname = g_strdup(params->tls_hostname->u.s);
}
--
2.19.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] qmp: Fix memory leak in migrate_params_test_apply
2019-08-27 8:05 [Qemu-devel] [PATCH] qmp: Fix memory leak in migrate_params_test_apply Ying Fang
@ 2019-08-27 8:30 ` no-reply
2019-08-27 8:38 ` Li Qiang
2019-09-03 16:46 ` Dr. David Alan Gilbert
2 siblings, 0 replies; 8+ messages in thread
From: no-reply @ 2019-08-27 8:30 UTC (permalink / raw)
To: fangying1
Cc: zhang.zhanghailiang, quintela, qemu-devel, zhouyibo3, dgilbert,
lcf.lichaofeng, fangying1
Patchew URL: https://patchew.org/QEMU/20190827080512.2417-1-fangying1@huawei.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Message-id: 20190827080512.2417-1-fangying1@huawei.com
Type: series
Subject: [Qemu-devel] [PATCH] qmp: Fix memory leak in migrate_params_test_apply
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
6e5df31 qmp: Fix memory leak in migrate_params_test_apply
=== OUTPUT BEGIN ===
ERROR: g_free(NULL) is safe this check is probably not required
#48: FILE: migration/migration.c:1254:
+ if (dest->tls_creds) {
+ g_free(dest->tls_creds);
ERROR: g_free(NULL) is safe this check is probably not required
#56: FILE: migration/migration.c:1262:
+ if (dest->tls_hostname) {
+ g_free(dest->tls_hostname);
total: 2 errors, 0 warnings, 17 lines checked
Commit 6e5df312feff (qmp: Fix memory leak in migrate_params_test_apply) has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20190827080512.2417-1-fangying1@huawei.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] qmp: Fix memory leak in migrate_params_test_apply
2019-08-27 8:05 [Qemu-devel] [PATCH] qmp: Fix memory leak in migrate_params_test_apply Ying Fang
2019-08-27 8:30 ` no-reply
@ 2019-08-27 8:38 ` Li Qiang
2019-08-27 9:13 ` fangying
2019-09-03 16:46 ` Dr. David Alan Gilbert
2 siblings, 1 reply; 8+ messages in thread
From: Li Qiang @ 2019-08-27 8:38 UTC (permalink / raw)
To: Ying Fang
Cc: zhanghailiang, Juan Quintela, Qemu Developers, zhouyibo3,
Dr. David Alan Gilbert, lcf.lichaofeng
Ying Fang <fangying1@huawei.com> 于2019年8月27日周二 下午4:06写道:
> Address Sanitizer shows memory leak in migrate_params_test_apply
> migration/migration.c:1253 and the stack is as bellow:
>
> Direct leak of 45 byte(s) in 9 object(s) allocated from:
> #0 0xffffbd7fc1db in __interceptor_malloc (/lib64/libasan.so.4+0xd31db)
> #1 0xffffbd514163 in g_malloc (/lib64/libglib-2.0.so.0+0x57163)
> #2 0xffffbd52f43b in g_strdup (/lib64/libglib-2.0.so.0+0x7243b)
> #3 0xaaaadfa4d623 in migrate_params_test_apply
> migration/migration.c:1253
> #4 0xaaaadfa4d623 in qmp_migrate_set_parameters
> migration/migration.c:1422
> #5 0xaaaadfa963f3 in hmp_migrate_set_parameter monitor/hmp-cmds.c:1867
> #6 0xaaaadfa8afe3 in handle_hmp_command monitor/hmp.c:1082
> #7 0xaaaadf479c57 in qmp_human_monitor_command monitor/misc.c:140
> #8 0xaaaadfadf87b in qmp_marshal_human_monitor_command
> qapi/qapi-commands-misc.c:1024
> #9 0xaaaadfc7797b in do_qmp_dispatch qapi/qmp-dispatch.c:131
> #10 0xaaaadfc7797b in qmp_dispatch qapi/qmp-dispatch.c:174
> #11 0xaaaadfa84fff in monitor_qmp_dispatch monitor/qmp.c:120
> #12 0xaaaadfa85bbf in monitor_qmp_bh_dispatcher monitor/qmp.c:209
> #13 0xaaaadfd2228f in aio_bh_call util/async.c:89
> #14 0xaaaadfd2228f in aio_bh_poll util/async.c:117
> #15 0xaaaadfd29bc3 in aio_dispatch util/aio-posix.c:459
> #16 0xaaaadfd21ff7 in aio_ctx_dispatch util/async.c:260
> #17 0xffffbd50e2f7 in g_main_context_dispatch
> (/lib64/libglib-2.0.so.0+0x512f7)
> #18 0xaaaadfd278d7 in glib_pollfds_poll util/main-loop.c:218
> #19 0xaaaadfd278d7 in os_host_main_loop_wait util/main-loop.c:241
> #20 0xaaaadfd278d7 in main_loop_wait util/main-loop.c:517
> #21 0xaaaadf67b5e7 in main_loop vl.c:1806
> #22 0xaaaadf15d453 in main vl.c:4488
>
> Cc: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Ying Fang <fangying1@huawei.com>
> ---
> migration/migration.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 8b9f2fe30a..05e44ff7cc 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1250,11 +1250,17 @@ static void
> migrate_params_test_apply(MigrateSetParameters *params,
>
> if (params->has_tls_creds) {
> assert(params->tls_creds->type == QTYPE_QSTRING);
> + if (dest->tls_creds) {
> + g_free(dest->tls_creds);
> + }
>
g_free can handle NULL, no need to do the NULL check.
Thanks,
Li Qiang
> dest->tls_creds = g_strdup(params->tls_creds->u.s);
> }
>
> if (params->has_tls_hostname) {
> assert(params->tls_hostname->type == QTYPE_QSTRING);
> + if (dest->tls_hostname) {
> + g_free(dest->tls_hostname);
> + }
> dest->tls_hostname = g_strdup(params->tls_hostname->u.s);
> }
>
> --
> 2.19.1
>
>
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] qmp: Fix memory leak in migrate_params_test_apply
2019-08-27 8:38 ` Li Qiang
@ 2019-08-27 9:13 ` fangying
0 siblings, 0 replies; 8+ messages in thread
From: fangying @ 2019-08-27 9:13 UTC (permalink / raw)
To: Li Qiang
Cc: zhanghailiang, Juan Quintela, Qemu Developers, zhouyibo3,
Dr. David Alan Gilbert, lcf.lichaofeng
On 2019/8/27 16:38, Li Qiang wrote:
>
>
> Ying Fang <fangying1@huawei.com <mailto:fangying1@huawei.com>> 于2019年8月27日周
> 二 下午4:06写道:
>
> Address Sanitizer shows memory leak in migrate_params_test_apply
> migration/migration.c:1253 and the stack is as bellow:
>
> Direct leak of 45 byte(s) in 9 object(s) allocated from:
> #0 0xffffbd7fc1db in __interceptor_malloc (/lib64/libasan.so.4+0xd31db)
> #1 0xffffbd514163 in g_malloc (/lib64/libglib-2.0.so.0+0x57163)
> #2 0xffffbd52f43b in g_strdup (/lib64/libglib-2.0.so.0+0x7243b)
> #3 0xaaaadfa4d623 in migrate_params_test_apply migration/migration.c:1253
> #4 0xaaaadfa4d623 in qmp_migrate_set_parameters migration/migration.c:1422
> #5 0xaaaadfa963f3 in hmp_migrate_set_parameter monitor/hmp-cmds.c:1867
> #6 0xaaaadfa8afe3 in handle_hmp_command monitor/hmp.c:1082
> #7 0xaaaadf479c57 in qmp_human_monitor_command monitor/misc.c:140
> #8 0xaaaadfadf87b in qmp_marshal_human_monitor_command
> qapi/qapi-commands-misc.c:1024
> #9 0xaaaadfc7797b in do_qmp_dispatch qapi/qmp-dispatch.c:131
> #10 0xaaaadfc7797b in qmp_dispatch qapi/qmp-dispatch.c:174
> #11 0xaaaadfa84fff in monitor_qmp_dispatch monitor/qmp.c:120
> #12 0xaaaadfa85bbf in monitor_qmp_bh_dispatcher monitor/qmp.c:209
> #13 0xaaaadfd2228f in aio_bh_call util/async.c:89
> #14 0xaaaadfd2228f in aio_bh_poll util/async.c:117
> #15 0xaaaadfd29bc3 in aio_dispatch util/aio-posix.c:459
> #16 0xaaaadfd21ff7 in aio_ctx_dispatch util/async.c:260
> #17 0xffffbd50e2f7 in g_main_context_dispatch
> (/lib64/libglib-2.0.so.0+0x512f7)
> #18 0xaaaadfd278d7 in glib_pollfds_poll util/main-loop.c:218
> #19 0xaaaadfd278d7 in os_host_main_loop_wait util/main-loop.c:241
> #20 0xaaaadfd278d7 in main_loop_wait util/main-loop.c:517
> #21 0xaaaadf67b5e7 in main_loop vl.c:1806
> #22 0xaaaadf15d453 in main vl.c:4488
>
> Cc: zhanghailiang <zhang.zhanghailiang@huawei.com
> <mailto:zhang.zhanghailiang@huawei.com>>
> Signed-off-by: Ying Fang <fangying1@huawei.com <mailto:fangying1@huawei.com>>
> ---
> migration/migration.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 8b9f2fe30a..05e44ff7cc 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1250,11 +1250,17 @@ static void
> migrate_params_test_apply(MigrateSetParameters *params,
>
> if (params->has_tls_creds) {
> assert(params->tls_creds->type == QTYPE_QSTRING);
> + if (dest->tls_creds) {
> + g_free(dest->tls_creds);
> + }
>
>
> g_free can handle NULL, no need to do the NULL check.
Thanks for your remind, we can call g_free here directly.
I'll send a v2 later.
>
> Thanks,
> Li Qiang
>
> dest->tls_creds = g_strdup(params->tls_creds->u.s);
> }
>
> if (params->has_tls_hostname) {
> assert(params->tls_hostname->type == QTYPE_QSTRING);
> + if (dest->tls_hostname) {
> + g_free(dest->tls_hostname);
> + }
> dest->tls_hostname = g_strdup(params->tls_hostname->u.s);
> }
>
> --
> 2.19.1
>
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] qmp: Fix memory leak in migrate_params_test_apply
2019-08-27 8:05 [Qemu-devel] [PATCH] qmp: Fix memory leak in migrate_params_test_apply Ying Fang
2019-08-27 8:30 ` no-reply
2019-08-27 8:38 ` Li Qiang
@ 2019-09-03 16:46 ` Dr. David Alan Gilbert
2019-09-04 7:24 ` fangying
2 siblings, 1 reply; 8+ messages in thread
From: Dr. David Alan Gilbert @ 2019-09-03 16:46 UTC (permalink / raw)
To: Ying Fang; +Cc: lcf.lichaofeng, zhanghailiang, qemu-devel, zhouyibo3, quintela
* Ying Fang (fangying1@huawei.com) wrote:
> Address Sanitizer shows memory leak in migrate_params_test_apply
> migration/migration.c:1253 and the stack is as bellow:
>
> Direct leak of 45 byte(s) in 9 object(s) allocated from:
> #0 0xffffbd7fc1db in __interceptor_malloc (/lib64/libasan.so.4+0xd31db)
> #1 0xffffbd514163 in g_malloc (/lib64/libglib-2.0.so.0+0x57163)
> #2 0xffffbd52f43b in g_strdup (/lib64/libglib-2.0.so.0+0x7243b)
> #3 0xaaaadfa4d623 in migrate_params_test_apply migration/migration.c:1253
> #4 0xaaaadfa4d623 in qmp_migrate_set_parameters migration/migration.c:1422
> #5 0xaaaadfa963f3 in hmp_migrate_set_parameter monitor/hmp-cmds.c:1867
> #6 0xaaaadfa8afe3 in handle_hmp_command monitor/hmp.c:1082
> #7 0xaaaadf479c57 in qmp_human_monitor_command monitor/misc.c:140
> #8 0xaaaadfadf87b in qmp_marshal_human_monitor_command qapi/qapi-commands-misc.c:1024
> #9 0xaaaadfc7797b in do_qmp_dispatch qapi/qmp-dispatch.c:131
> #10 0xaaaadfc7797b in qmp_dispatch qapi/qmp-dispatch.c:174
> #11 0xaaaadfa84fff in monitor_qmp_dispatch monitor/qmp.c:120
> #12 0xaaaadfa85bbf in monitor_qmp_bh_dispatcher monitor/qmp.c:209
> #13 0xaaaadfd2228f in aio_bh_call util/async.c:89
> #14 0xaaaadfd2228f in aio_bh_poll util/async.c:117
> #15 0xaaaadfd29bc3 in aio_dispatch util/aio-posix.c:459
> #16 0xaaaadfd21ff7 in aio_ctx_dispatch util/async.c:260
> #17 0xffffbd50e2f7 in g_main_context_dispatch (/lib64/libglib-2.0.so.0+0x512f7)
> #18 0xaaaadfd278d7 in glib_pollfds_poll util/main-loop.c:218
> #19 0xaaaadfd278d7 in os_host_main_loop_wait util/main-loop.c:241
> #20 0xaaaadfd278d7 in main_loop_wait util/main-loop.c:517
> #21 0xaaaadf67b5e7 in main_loop vl.c:1806
> #22 0xaaaadf15d453 in main vl.c:4488
>
> Cc: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Ying Fang <fangying1@huawei.com>
> ---
> migration/migration.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 8b9f2fe30a..05e44ff7cc 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1250,11 +1250,17 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
>
> if (params->has_tls_creds) {
> assert(params->tls_creds->type == QTYPE_QSTRING);
> + if (dest->tls_creds) {
> + g_free(dest->tls_creds);
> + }
> dest->tls_creds = g_strdup(params->tls_creds->u.s);
> }
>
> if (params->has_tls_hostname) {
> assert(params->tls_hostname->type == QTYPE_QSTRING);
> + if (dest->tls_hostname) {
> + g_free(dest->tls_hostname);
> + }
> dest->tls_hostname = g_strdup(params->tls_hostname->u.s);
> }
Thanks for reporting the leak, but I don't think this is the right fix:
In the call chain we have, qmp_migrate_set_parameters calls:
migrate_params_test_apply(params, &tmp);
tmp is a stack allocated variable that becomes the 'dest'
we see here. Then at the top of migrate_params_test_apply
we have:
*dest = migrate_get_current()->parameters;
so that's probably bad; that's a shallow copy, so dest->tls_authz
points to the same storage as the real current migration parameters.
whne the code does:
if (params->has_tls_creds) {
assert(params->tls_creds->type == QTYPE_QSTRING);
dest->tls_creds = g_strdup(params->tls_creds->u.s);
}
it's only changing the pointer in the 'tmp' not the main copy
because of migrate_params_check fails then the parameters get entirely
unchanged. So if you do a free on dest->tls_hostname you end up
freeing the real parameter that's still getting used, not the tmp.
So I think we need to:
a) change migrate_params_test_apply so that it returns a
MigrationParameters * rather than taking &tmp
b) Make migrate_params_test use QAPI_CLONE to clone instead of doing:
*dest = migrate_get_current()->parameters;
c) Then do a qapi_free_MigrateParameters in qmp_migrate_set_parameters
on both the true and false paths.
Does that make sense?
Dave
>
> --
> 2.19.1
>
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] qmp: Fix memory leak in migrate_params_test_apply
2019-09-03 16:46 ` Dr. David Alan Gilbert
@ 2019-09-04 7:24 ` fangying
0 siblings, 0 replies; 8+ messages in thread
From: fangying @ 2019-09-04 7:24 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: lcf.lichaofeng, zhanghailiang, qemu-devel, zhouyibo3, quintela
On 2019/9/4 0:46, Dr. David Alan Gilbert wrote:
> * Ying Fang (fangying1@huawei.com) wrote:
>> Address Sanitizer shows memory leak in migrate_params_test_apply
>> migration/migration.c:1253 and the stack is as bellow:
>>
>> Direct leak of 45 byte(s) in 9 object(s) allocated from:
>> #0 0xffffbd7fc1db in __interceptor_malloc (/lib64/libasan.so.4+0xd31db)
>> #1 0xffffbd514163 in g_malloc (/lib64/libglib-2.0.so.0+0x57163)
>> #2 0xffffbd52f43b in g_strdup (/lib64/libglib-2.0.so.0+0x7243b)
>> #3 0xaaaadfa4d623 in migrate_params_test_apply migration/migration.c:1253
>> #4 0xaaaadfa4d623 in qmp_migrate_set_parameters migration/migration.c:1422
>> #5 0xaaaadfa963f3 in hmp_migrate_set_parameter monitor/hmp-cmds.c:1867
>> #6 0xaaaadfa8afe3 in handle_hmp_command monitor/hmp.c:1082
>> #7 0xaaaadf479c57 in qmp_human_monitor_command monitor/misc.c:140
>> #8 0xaaaadfadf87b in qmp_marshal_human_monitor_command qapi/qapi-commands-misc.c:1024
>> #9 0xaaaadfc7797b in do_qmp_dispatch qapi/qmp-dispatch.c:131
>> #10 0xaaaadfc7797b in qmp_dispatch qapi/qmp-dispatch.c:174
>> #11 0xaaaadfa84fff in monitor_qmp_dispatch monitor/qmp.c:120
>> #12 0xaaaadfa85bbf in monitor_qmp_bh_dispatcher monitor/qmp.c:209
>> #13 0xaaaadfd2228f in aio_bh_call util/async.c:89
>> #14 0xaaaadfd2228f in aio_bh_poll util/async.c:117
>> #15 0xaaaadfd29bc3 in aio_dispatch util/aio-posix.c:459
>> #16 0xaaaadfd21ff7 in aio_ctx_dispatch util/async.c:260
>> #17 0xffffbd50e2f7 in g_main_context_dispatch (/lib64/libglib-2.0.so.0+0x512f7)
>> #18 0xaaaadfd278d7 in glib_pollfds_poll util/main-loop.c:218
>> #19 0xaaaadfd278d7 in os_host_main_loop_wait util/main-loop.c:241
>> #20 0xaaaadfd278d7 in main_loop_wait util/main-loop.c:517
>> #21 0xaaaadf67b5e7 in main_loop vl.c:1806
>> #22 0xaaaadf15d453 in main vl.c:4488
>>
>> Cc: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Signed-off-by: Ying Fang <fangying1@huawei.com>
>> ---
>> migration/migration.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 8b9f2fe30a..05e44ff7cc 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -1250,11 +1250,17 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
>>
>> if (params->has_tls_creds) {
>> assert(params->tls_creds->type == QTYPE_QSTRING);
>> + if (dest->tls_creds) {
>> + g_free(dest->tls_creds);
>> + }
>> dest->tls_creds = g_strdup(params->tls_creds->u.s);
>> }
>>
>> if (params->has_tls_hostname) {
>> assert(params->tls_hostname->type == QTYPE_QSTRING);
>> + if (dest->tls_hostname) {
>> + g_free(dest->tls_hostname);
>> + }
>> dest->tls_hostname = g_strdup(params->tls_hostname->u.s);
>> }
>
> Thanks for reporting the leak, but I don't think this is the right fix:
>
> In the call chain we have, qmp_migrate_set_parameters calls:
>
> migrate_params_test_apply(params, &tmp);
>
> tmp is a stack allocated variable that becomes the 'dest'
> we see here. Then at the top of migrate_params_test_apply
> we have:
>
> *dest = migrate_get_current()->parameters;
>
> so that's probably bad; that's a shallow copy, so dest->tls_authz
> points to the same storage as the real current migration parameters.
>
> whne the code does:
> if (params->has_tls_creds) {
> assert(params->tls_creds->type == QTYPE_QSTRING);
> dest->tls_creds = g_strdup(params->tls_creds->u.s);
> }
>
Yes, you are right, this patch will not fix this issue.
'tmp' is just a copy of 'dest' here, it 's used to do parameter sanity check.
We should either free tmp.tls_creds/tmp.tls_hostname after migrate_params_check
or change migrate_params_test_apply .
> it's only changing the pointer in the 'tmp' not the main copy
> because of migrate_params_check fails then the parameters get entirely
> unchanged. So if you do a free on dest->tls_hostname you end up
> freeing the real parameter that's still getting used, not the tmp.
>
> So I think we need to:
> a) change migrate_params_test_apply so that it returns a
> MigrationParameters * rather than taking &tmp
> b) Make migrate_params_test use QAPI_CLONE to clone instead of doing:
> *dest = migrate_get_current()->parameters;
> c) Then do a qapi_free_MigrateParameters in qmp_migrate_set_parameters
> on both the true and false paths.
>
> Does that make sense?
>
> Dave
>
>
>>
>> --
>> 2.19.1
>>
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
> .
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] qmp: Fix memory leak in migrate_params_test_apply
2019-08-27 11:16 Ying Fang
@ 2019-08-28 7:58 ` Stefano Garzarella
0 siblings, 0 replies; 8+ messages in thread
From: Stefano Garzarella @ 2019-08-28 7:58 UTC (permalink / raw)
To: Ying Fang; +Cc: lcf.lichaofeng, zhanghailiang, qemu-devel, zhouyibo3, quintela
On Tue, Aug 27, 2019 at 07:16:50PM +0800, Ying Fang wrote:
> Address Sanitizer shows memory leak in migrate_params_test_apply
> migration/migration.c:1253 and the stack is as bellow:
>
> Direct leak of 45 byte(s) in 9 object(s) allocated from:
> #0 0xffffbd7fc1db in __interceptor_malloc (/lib64/libasan.so.4+0xd31db)
> #1 0xffffbd514163 in g_malloc (/lib64/libglib-2.0.so.0+0x57163)
> #2 0xffffbd52f43b in g_strdup (/lib64/libglib-2.0.so.0+0x7243b)
> #3 0xaaaadfa4d623 in migrate_params_test_apply migration/migration.c:1253
> #4 0xaaaadfa4d623 in qmp_migrate_set_parameters migration/migration.c:1422
> #5 0xaaaadfa963f3 in hmp_migrate_set_parameter monitor/hmp-cmds.c:1867
> #6 0xaaaadfa8afe3 in handle_hmp_command monitor/hmp.c:1082
> #7 0xaaaadf479c57 in qmp_human_monitor_command monitor/misc.c:140
> #8 0xaaaadfadf87b in qmp_marshal_human_monitor_command qapi/qapi-commands-misc.c:1024
> #9 0xaaaadfc7797b in do_qmp_dispatch qapi/qmp-dispatch.c:131
> #10 0xaaaadfc7797b in qmp_dispatch qapi/qmp-dispatch.c:174
> #11 0xaaaadfa84fff in monitor_qmp_dispatch monitor/qmp.c:120
> #12 0xaaaadfa85bbf in monitor_qmp_bh_dispatcher monitor/qmp.c:209
> #13 0xaaaadfd2228f in aio_bh_call util/async.c:89
> #14 0xaaaadfd2228f in aio_bh_poll util/async.c:117
> #15 0xaaaadfd29bc3 in aio_dispatch util/aio-posix.c:459
> #16 0xaaaadfd21ff7 in aio_ctx_dispatch util/async.c:260
> #17 0xffffbd50e2f7 in g_main_context_dispatch (/lib64/libglib-2.0.so.0+0x512f7)
> #18 0xaaaadfd278d7 in glib_pollfds_poll util/main-loop.c:218
> #19 0xaaaadfd278d7 in os_host_main_loop_wait util/main-loop.c:241
> #20 0xaaaadfd278d7 in main_loop_wait util/main-loop.c:517
> #21 0xaaaadf67b5e7 in main_loop vl.c:1806
> #22 0xaaaadf15d453 in main vl.c:4488
>
> Cc: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Ying Fang <fangying1@huawei.com>
> ---
> migration/migration.c | 2 ++
> 1 file changed, 2 insertions(+)
LGTM:
Acked-by: Stefano Garzarella <sgarzare@redhat.com>
Just a note, please use the version tag [PATCH vX] next time:
https://wiki.qemu.org/Contribute/SubmitAPatch#When_resending_patches_add_a_version_tag
In this case it had to be [PATCH v2].
Thanks,
Stefano
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH] qmp: Fix memory leak in migrate_params_test_apply
@ 2019-08-27 11:16 Ying Fang
2019-08-28 7:58 ` Stefano Garzarella
0 siblings, 1 reply; 8+ messages in thread
From: Ying Fang @ 2019-08-27 11:16 UTC (permalink / raw)
To: qemu-devel, quintela; +Cc: lcf.lichaofeng, Ying Fang, zhanghailiang, zhouyibo3
Address Sanitizer shows memory leak in migrate_params_test_apply
migration/migration.c:1253 and the stack is as bellow:
Direct leak of 45 byte(s) in 9 object(s) allocated from:
#0 0xffffbd7fc1db in __interceptor_malloc (/lib64/libasan.so.4+0xd31db)
#1 0xffffbd514163 in g_malloc (/lib64/libglib-2.0.so.0+0x57163)
#2 0xffffbd52f43b in g_strdup (/lib64/libglib-2.0.so.0+0x7243b)
#3 0xaaaadfa4d623 in migrate_params_test_apply migration/migration.c:1253
#4 0xaaaadfa4d623 in qmp_migrate_set_parameters migration/migration.c:1422
#5 0xaaaadfa963f3 in hmp_migrate_set_parameter monitor/hmp-cmds.c:1867
#6 0xaaaadfa8afe3 in handle_hmp_command monitor/hmp.c:1082
#7 0xaaaadf479c57 in qmp_human_monitor_command monitor/misc.c:140
#8 0xaaaadfadf87b in qmp_marshal_human_monitor_command qapi/qapi-commands-misc.c:1024
#9 0xaaaadfc7797b in do_qmp_dispatch qapi/qmp-dispatch.c:131
#10 0xaaaadfc7797b in qmp_dispatch qapi/qmp-dispatch.c:174
#11 0xaaaadfa84fff in monitor_qmp_dispatch monitor/qmp.c:120
#12 0xaaaadfa85bbf in monitor_qmp_bh_dispatcher monitor/qmp.c:209
#13 0xaaaadfd2228f in aio_bh_call util/async.c:89
#14 0xaaaadfd2228f in aio_bh_poll util/async.c:117
#15 0xaaaadfd29bc3 in aio_dispatch util/aio-posix.c:459
#16 0xaaaadfd21ff7 in aio_ctx_dispatch util/async.c:260
#17 0xffffbd50e2f7 in g_main_context_dispatch (/lib64/libglib-2.0.so.0+0x512f7)
#18 0xaaaadfd278d7 in glib_pollfds_poll util/main-loop.c:218
#19 0xaaaadfd278d7 in os_host_main_loop_wait util/main-loop.c:241
#20 0xaaaadfd278d7 in main_loop_wait util/main-loop.c:517
#21 0xaaaadf67b5e7 in main_loop vl.c:1806
#22 0xaaaadf15d453 in main vl.c:4488
Cc: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Ying Fang <fangying1@huawei.com>
---
migration/migration.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/migration/migration.c b/migration/migration.c
index 8b9f2fe30a..d60dda2b9c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1250,11 +1250,13 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
if (params->has_tls_creds) {
assert(params->tls_creds->type == QTYPE_QSTRING);
+ g_free(dest->tls_creds);
dest->tls_creds = g_strdup(params->tls_creds->u.s);
}
if (params->has_tls_hostname) {
assert(params->tls_hostname->type == QTYPE_QSTRING);
+ g_free(dest->tls_hostname);
dest->tls_hostname = g_strdup(params->tls_hostname->u.s);
}
--
2.19.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-09-04 7:25 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-27 8:05 [Qemu-devel] [PATCH] qmp: Fix memory leak in migrate_params_test_apply Ying Fang
2019-08-27 8:30 ` no-reply
2019-08-27 8:38 ` Li Qiang
2019-08-27 9:13 ` fangying
2019-09-03 16:46 ` Dr. David Alan Gilbert
2019-09-04 7:24 ` fangying
2019-08-27 11:16 Ying Fang
2019-08-28 7:58 ` Stefano Garzarella
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.