Re: [PATCH v2 00/10] fix some error memleaks
ping! Hello, Some patches have been reviewed a few weeks ago but missed to queue. The rest patches are the following: https://patchwork.kernel.org/patch/11745621/ https://patchwork.kernel.org/patch/11745633/ https://patchwork.kernel.org/patch/11745629/ https://patchwork.kernel.org/patch/11745625/ https://patchwork.kernel.org/patch/11745627/ https://patchwork.kernel.org/patch/11745635/ On 2020/9/21 10:12, Pan Nengyuan wrote: > ping! > > and cc: qemu-triv...@nongnu.org > > On 2020/9/17 20:49, Pan Nengyuan wrote: >> ping! >> >> Anyone queued the rest(patch 01/02/03/07/08/09)? >> >> On 2020/8/31 21:43, Pan Nengyuan wrote: >>> This series fix some Error/GError memleaks. >>> >>> V2: >>> 1. remove two patches.(One has aleardy applied. The other has fixed.) >>> 2. change patch 5/10 and 7/10. >>> >>> Pan Nengyuan (10): >>> qga/channel-posix: Plug memory leak in ga_channel_write_all() >>> elf2dmp/qemu_elf: Plug memleak in QEMU_Elf_init >>> elf2dmp/pdb: Plug memleak in pdb_init_from_file >>> target/i386/sev: Plug memleak in sev_read_file_base64 >>> ui/gtk-gl-area: Plug memleak in gd_gl_area_create_context() >>> target/i386/cpu: Fix memleak in x86_cpu_class_check_missing_features >>> migration/colo: Plug memleaks in colo_process_incoming_thread >>> blockdev: Fix a memleak in drive_backup_prepare() >>> block/file-posix: fix a possible undefined behavior >>> vnc-auth-sasl: Plug memleak in vnc_socket_ip_addr_string >>> >>> block/file-posix.c | 2 +- >>> blockdev.c | 1 + >>> contrib/elf2dmp/pdb.c | 1 + >>> contrib/elf2dmp/qemu_elf.c | 1 + >>> migration/colo.c | 5 - >>> qga/channel-posix.c| 6 +- >>> target/i386/cpu.c | 1 + >>> target/i386/sev.c | 1 + >>> ui/gtk-gl-area.c | 11 +++ >>> ui/vnc-auth-sasl.c | 1 + >>> 10 files changed, 27 insertions(+), 3 deletions(-) >>>
Re: [PATCH] target/i386/cpu: add return value verification and ignore Error objects
ping!
Maybe missed to queue?
On 2020/9/4 21:20, Philippe Mathieu-Daudé wrote:
> On 9/4/20 3:45 PM, Pan Nengyuan wrote:
>> 'err' is unnecessary in x86_cpu_class_check_missing_features(), we can
>> change x86_cpu_expand_features()
>> to return true on success, false on failure, then pass NULL here to remove
>> it.
>>
>> Signed-off-by: Pan Nengyuan
>> Suggested-by: Markus Armbruster
>
> Reviewed-by: Philippe Mathieu-Daudé
>
>> ---
>> target/i386/cpu.c | 15 +++
>> 1 file changed, 7 insertions(+), 8 deletions(-)
>>
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index 49d8958528..c3d3766133 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -4883,7 +4883,7 @@ static void x86_cpu_parse_featurestr(const char
>> *typename, char *features,
>> }
>> }
>>
>> -static void x86_cpu_expand_features(X86CPU *cpu, Error **errp);
>> +static bool x86_cpu_expand_features(X86CPU *cpu, Error **errp);
>> static void x86_cpu_filter_features(X86CPU *cpu, bool verbose);
>>
>> /* Build a list with the name of all features on a feature word array */
>> @@ -4925,7 +4925,6 @@ static void
>> x86_cpu_class_check_missing_features(X86CPUClass *xcc,
>> strList **missing_feats)
>> {
>> X86CPU *xc;
>> -Error *err = NULL;
>> strList **next = missing_feats;
>>
>> if (xcc->host_cpuid_required && !accel_uses_host_cpuid()) {
>> @@ -4937,8 +4936,7 @@ static void
>> x86_cpu_class_check_missing_features(X86CPUClass *xcc,
>>
>> xc = X86_CPU(object_new_with_class(OBJECT_CLASS(xcc)));
>>
>> -x86_cpu_expand_features(xc, );
>> -if (err) {
>> +if (!x86_cpu_expand_features(xc, NULL)) {
>> /* Errors at x86_cpu_expand_features should never happen,
>> * but in case it does, just report the model as not
>> * runnable at all using the "type" property.
>> @@ -4947,7 +4945,6 @@ static void
>> x86_cpu_class_check_missing_features(X86CPUClass *xcc,
>> new->value = g_strdup("type");
>> *next = new;
>> next = >next;
>> -error_free(err);
>> }
>>
>> x86_cpu_filter_features(xc, false);
>> @@ -6426,7 +6423,7 @@ static void x86_cpu_enable_xsave_components(X86CPU
>> *cpu)
>> /* Expand CPU configuration data, based on configured features
>> * and host/accelerator capabilities when appropriate.
>> */
>> -static void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
>> +static bool x86_cpu_expand_features(X86CPU *cpu, Error **errp)
>> {
>> CPUX86State *env = >env;
>> FeatureWord w;
>> @@ -6436,14 +6433,14 @@ static void x86_cpu_expand_features(X86CPU *cpu,
>> Error **errp)
>> for (l = plus_features; l; l = l->next) {
>> const char *prop = l->data;
>> if (!object_property_set_bool(OBJECT(cpu), prop, true, errp)) {
>> -return;
>> +return false;
>> }
>> }
>>
>> for (l = minus_features; l; l = l->next) {
>> const char *prop = l->data;
>> if (!object_property_set_bool(OBJECT(cpu), prop, false, errp)) {
>> -return;
>> +return false;
>> }
>> }
>>
>> @@ -6540,6 +6537,8 @@ static void x86_cpu_expand_features(X86CPU *cpu, Error
>> **errp)
>> if (env->cpuid_xlevel2 == UINT32_MAX) {
>> env->cpuid_xlevel2 = env->cpuid_min_xlevel2;
>> }
>> +
>> +return true;
>> }
>>
>> /*
>>
>
Re: [PATCH] net/filter-rewriter: destroy g_hash_table in colo_rewriter_cleanup
ping! Maybe missed to queue? On 2020/9/5 8:44, Li Qiang wrote: > Pan Nengyuan 于2020年9月4日周五 下午3:23写道： >> >> s->connection_track_table forgot to destroy in colo_rewriter_cleanup. Fix it. >> >> Reported-by: Euler Robot >> Signed-off-by: Pan Nengyuan > > Reviewed-by: Li Qiang > >> --- >> net/filter-rewriter.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/net/filter-rewriter.c b/net/filter-rewriter.c >> index 1aaad101b6..9ff366d44f 100644 >> --- a/net/filter-rewriter.c >> +++ b/net/filter-rewriter.c >> @@ -376,6 +376,8 @@ static void colo_rewriter_cleanup(NetFilterState *nf) >> filter_rewriter_flush(nf); >> g_free(s->incoming_queue); >> } >> + >> +g_hash_table_destroy(s->connection_track_table); >> } >> >> static void colo_rewriter_setup(NetFilterState *nf, Error **errp) >> -- >> 2.18.2 >> >>
Re: [RFC PATCH 2/2] .mailmap: Fix more contributor entries
On 2020/10/5 1:26, Philippe Mathieu-Daudé wrote: > These authors have some incorrect author email field. > For each of them, there is one commit with the replaced > entry. > > Cc: Anup Patel > Cc: Andrew Melnychenko > Cc: David Carlier > Cc: Erik Smit > Cc: Marcel Apfelbaum > Cc: Pan Nengyuan > Cc: Stefan Berger > Signed-off-by: Philippe Mathieu-Daudé > --- Acked-by: Pan Nengyuan
Re: [PATCH v2 00/10] fix some error memleaks
ping! and cc: qemu-triv...@nongnu.org On 2020/9/17 20:49, Pan Nengyuan wrote: > ping! > > Anyone queued the rest(patch 01/02/03/07/08/09)? > > On 2020/8/31 21:43, Pan Nengyuan wrote: >> This series fix some Error/GError memleaks. >> >> V2: >> 1. remove two patches.(One has aleardy applied. The other has fixed.) >> 2. change patch 5/10 and 7/10. >> >> Pan Nengyuan (10): >> qga/channel-posix: Plug memory leak in ga_channel_write_all() >> elf2dmp/qemu_elf: Plug memleak in QEMU_Elf_init >> elf2dmp/pdb: Plug memleak in pdb_init_from_file >> target/i386/sev: Plug memleak in sev_read_file_base64 >> ui/gtk-gl-area: Plug memleak in gd_gl_area_create_context() >> target/i386/cpu: Fix memleak in x86_cpu_class_check_missing_features >> migration/colo: Plug memleaks in colo_process_incoming_thread >> blockdev: Fix a memleak in drive_backup_prepare() >> block/file-posix: fix a possible undefined behavior >> vnc-auth-sasl: Plug memleak in vnc_socket_ip_addr_string >> >> block/file-posix.c | 2 +- >> blockdev.c | 1 + >> contrib/elf2dmp/pdb.c | 1 + >> contrib/elf2dmp/qemu_elf.c | 1 + >> migration/colo.c | 5 - >> qga/channel-posix.c| 6 +- >> target/i386/cpu.c | 1 + >> target/i386/sev.c | 1 + >> ui/gtk-gl-area.c | 11 +++ >> ui/vnc-auth-sasl.c | 1 + >> 10 files changed, 27 insertions(+), 3 deletions(-) >>
Re: [PATCH v2 00/10] fix some error memleaks
ping! Anyone queued the rest(patch 01/02/03/07/08/09)? On 2020/8/31 21:43, Pan Nengyuan wrote: > This series fix some Error/GError memleaks. > > V2: > 1. remove two patches.(One has aleardy applied. The other has fixed.) > 2. change patch 5/10 and 7/10. > > Pan Nengyuan (10): > qga/channel-posix: Plug memory leak in ga_channel_write_all() > elf2dmp/qemu_elf: Plug memleak in QEMU_Elf_init > elf2dmp/pdb: Plug memleak in pdb_init_from_file > target/i386/sev: Plug memleak in sev_read_file_base64 > ui/gtk-gl-area: Plug memleak in gd_gl_area_create_context() > target/i386/cpu: Fix memleak in x86_cpu_class_check_missing_features > migration/colo: Plug memleaks in colo_process_incoming_thread > blockdev: Fix a memleak in drive_backup_prepare() > block/file-posix: fix a possible undefined behavior > vnc-auth-sasl: Plug memleak in vnc_socket_ip_addr_string > > block/file-posix.c | 2 +- > blockdev.c | 1 + > contrib/elf2dmp/pdb.c | 1 + > contrib/elf2dmp/qemu_elf.c | 1 + > migration/colo.c | 5 - > qga/channel-posix.c| 6 +- > target/i386/cpu.c | 1 + > target/i386/sev.c | 1 + > ui/gtk-gl-area.c | 11 +++ > ui/vnc-auth-sasl.c | 1 + > 10 files changed, 27 insertions(+), 3 deletions(-) >
[PATCH] test-vmstate: remove unnecessary code in match_interval_mapping_node
'str' is not used in match_interval_mapping_node(), remove it.
Signed-off-by: Pan Nengyuan
---
tests/test-vmstate.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/tests/test-vmstate.c b/tests/test-vmstate.c
index f8de709a0b..1c763015d0 100644
--- a/tests/test-vmstate.c
+++ b/tests/test-vmstate.c
@@ -1055,9 +1055,6 @@ static gboolean match_interval_mapping_node(gpointer key,
TestGTreeMapping *map_a, *map_b;
TestGTreeInterval *a, *b;
struct match_node_data *d = (struct match_node_data *)data;
-char *str = g_strdup_printf("dest");
-
-g_free(str);
a = (TestGTreeInterval *)key;
b = (TestGTreeInterval *)d->key;
--
2.21.0.windows.1
[PATCH] net/filter-rewriter: destroy g_hash_table in colo_rewriter_cleanup
s->connection_track_table forgot to destroy in colo_rewriter_cleanup. Fix it. Reported-by: Euler Robot Signed-off-by: Pan Nengyuan --- net/filter-rewriter.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/net/filter-rewriter.c b/net/filter-rewriter.c index 1aaad101b6..9ff366d44f 100644 --- a/net/filter-rewriter.c +++ b/net/filter-rewriter.c @@ -376,6 +376,8 @@ static void colo_rewriter_cleanup(NetFilterState *nf) filter_rewriter_flush(nf); g_free(s->incoming_queue); } + +g_hash_table_destroy(s->connection_track_table); } static void colo_rewriter_setup(NetFilterState *nf, Error **errp) -- 2.18.2
[PATCH] target/i386/cpu: add return value verification and ignore Error objects
'err' is unnecessary in x86_cpu_class_check_missing_features(), we can change
x86_cpu_expand_features()
to return true on success, false on failure, then pass NULL here to remove it.
Signed-off-by: Pan Nengyuan
Suggested-by: Markus Armbruster
---
target/i386/cpu.c | 15 +++
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 49d8958528..c3d3766133 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -4883,7 +4883,7 @@ static void x86_cpu_parse_featurestr(const char
*typename, char *features,
}
}
-static void x86_cpu_expand_features(X86CPU *cpu, Error **errp);
+static bool x86_cpu_expand_features(X86CPU *cpu, Error **errp);
static void x86_cpu_filter_features(X86CPU *cpu, bool verbose);
/* Build a list with the name of all features on a feature word array */
@@ -4925,7 +4925,6 @@ static void
x86_cpu_class_check_missing_features(X86CPUClass *xcc,
strList **missing_feats)
{
X86CPU *xc;
-Error *err = NULL;
strList **next = missing_feats;
if (xcc->host_cpuid_required && !accel_uses_host_cpuid()) {
@@ -4937,8 +4936,7 @@ static void
x86_cpu_class_check_missing_features(X86CPUClass *xcc,
xc = X86_CPU(object_new_with_class(OBJECT_CLASS(xcc)));
-x86_cpu_expand_features(xc, );
-if (err) {
+if (!x86_cpu_expand_features(xc, NULL)) {
/* Errors at x86_cpu_expand_features should never happen,
* but in case it does, just report the model as not
* runnable at all using the "type" property.
@@ -4947,7 +4945,6 @@ static void
x86_cpu_class_check_missing_features(X86CPUClass *xcc,
new->value = g_strdup("type");
*next = new;
next = >next;
-error_free(err);
}
x86_cpu_filter_features(xc, false);
@@ -6426,7 +6423,7 @@ static void x86_cpu_enable_xsave_components(X86CPU *cpu)
/* Expand CPU configuration data, based on configured features
* and host/accelerator capabilities when appropriate.
*/
-static void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
+static bool x86_cpu_expand_features(X86CPU *cpu, Error **errp)
{
CPUX86State *env = >env;
FeatureWord w;
@@ -6436,14 +6433,14 @@ static void x86_cpu_expand_features(X86CPU *cpu, Error
**errp)
for (l = plus_features; l; l = l->next) {
const char *prop = l->data;
if (!object_property_set_bool(OBJECT(cpu), prop, true, errp)) {
-return;
+return false;
}
}
for (l = minus_features; l; l = l->next) {
const char *prop = l->data;
if (!object_property_set_bool(OBJECT(cpu), prop, false, errp)) {
-return;
+return false;
}
}
@@ -6540,6 +6537,8 @@ static void x86_cpu_expand_features(X86CPU *cpu, Error
**errp)
if (env->cpuid_xlevel2 == UINT32_MAX) {
env->cpuid_xlevel2 = env->cpuid_min_xlevel2;
}
+
+return true;
}
/*
--
2.18.2
Re: [PATCH v2 06/10] target/i386/cpu: Fix memleak in x86_cpu_class_check_missing_features
On 2020/9/1 20:03, Markus Armbruster wrote:
> Pan Nengyuan writes:
>
>> 'err' forgot to free in x86_cpu_class_check_missing_features error path.
>> Fix that.
>>
>> Reported-by: Euler Robot
>> Signed-off-by: Pan Nengyuan
>> Reviewed-by: Li Qiang
>> ---
>> Cc: Paolo Bonzini
>> Cc: Richard Henderson
>> Cc: Eduardo Habkost
>> ---
>> - V2: no changes in v2.
>> ---
>> target/i386/cpu.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index 588f32e136..4678aac0b4 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -4872,6 +4872,7 @@ static void
>> x86_cpu_class_check_missing_features(X86CPUClass *xcc,
>x86_cpu_expand_features(xc, );
>if (err) {
>/* Errors at x86_cpu_expand_features should never happen,
> * but in case it does, just report the model as not
> * runnable at all using the "type" property.
> */
>strList *new = g_new0(strList, 1);
>> new->value = g_strdup("type");
>> *next = new;
>> next = >next;
>> +error_free(err);
>> }
>>
>> x86_cpu_filter_features(xc, false);
>
> Reviewed-by: Markus Armbruster
>
> Recommended cleanup: change x86_cpu_filter_features() to return true on
> success, false on failure, then pass NULL here and check the return
> value. Can be done on top.
>
Agree with you, 'err' is not used, we can pass NULL here.
BTW, I think the func you mentioned shoule be x86_cpu_expand_features(), not
x86_cpu_filter_features()?
Thanks.
> .
>
[PATCH v2 10/10] vnc-auth-sasl: Plug memleak in vnc_socket_ip_addr_string
'addr' is forgot to free in vnc_socket_ip_addr_string error path. Fix that.
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
Reviewed-by: Li Qiang
---
Cc: Gerd Hoffmann
---
- V2: no changes in v2.
---
ui/vnc-auth-sasl.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/ui/vnc-auth-sasl.c b/ui/vnc-auth-sasl.c
index 7b2b09f242..0517b2ead9 100644
--- a/ui/vnc-auth-sasl.c
+++ b/ui/vnc-auth-sasl.c
@@ -522,6 +522,7 @@ vnc_socket_ip_addr_string(QIOChannelSocket *ioc,
if (addr->type != SOCKET_ADDRESS_TYPE_INET) {
error_setg(errp, "Not an inet socket type");
+qapi_free_SocketAddress(addr);
return NULL;
}
ret = g_strdup_printf("%s;%s", addr->u.inet.host, addr->u.inet.port);
--
2.18.2
[PATCH v2 05/10] ui/gtk-gl-area: Plug memleak in gd_gl_area_create_context()
Receiving error in local variable err, and forgot to free it.
This patch check the return value of 'gdk_window_create_gl_context'
and 'gdk_gl_context_realize', then free err to fix it.
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
---
Cc: Gerd Hoffmann
---
V2->V1: check the return value of 'gdk_window_create_gl_context'
and 'gdk_gl_context_realize' instead of omitting it(Suggested by Li Qiang)
---
ui/gtk-gl-area.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/ui/gtk-gl-area.c b/ui/gtk-gl-area.c
index 85f9d14c51..98c22d23f5 100644
--- a/ui/gtk-gl-area.c
+++ b/ui/gtk-gl-area.c
@@ -147,10 +147,21 @@ QEMUGLContext
gd_gl_area_create_context(DisplayChangeListener *dcl,
gtk_gl_area_make_current(GTK_GL_AREA(vc->gfx.drawing_area));
window = gtk_widget_get_window(vc->gfx.drawing_area);
ctx = gdk_window_create_gl_context(window, );
+if (err) {
+g_printerr("Create gdk gl context failed: %s\n", err->message);
+g_error_free(err);
+return NULL;
+}
gdk_gl_context_set_required_version(ctx,
params->major_ver,
params->minor_ver);
gdk_gl_context_realize(ctx, );
+if (err) {
+g_printerr("Realize gdk gl context failed: %s\n", err->message);
+g_error_free(err);
+g_clear_object();
+return NULL;
+}
return ctx;
}
--
2.18.2
[PATCH v2 09/10] block/file-posix: fix a possible undefined behavior
local_err is not initialized to NULL, it will cause a assert error as below:
qemu/util/error.c:59: error_setv: Assertion `*errp == NULL' failed.
Fixes: c6447510690
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
Reviewed-by: Stefano Garzarella
---
Cc: Kevin Wolf
Cc: Max Reitz
Cc: Aarushi Mehta
Cc: qemu-bl...@nongnu.org
---
- V2: no changes in v2.
---
block/file-posix.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index 9a00d4190a..697a7d9eea 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2113,7 +2113,7 @@ static void raw_aio_attach_aio_context(BlockDriverState
*bs,
#endif
#ifdef CONFIG_LINUX_IO_URING
if (s->use_linux_io_uring) {
-Error *local_err;
+Error *local_err = NULL;
if (!aio_setup_linux_io_uring(new_context, _err)) {
error_reportf_err(local_err, "Unable to use linux io_uring, "
"falling back to thread pool: ");
--
2.18.2
[PATCH v2 06/10] target/i386/cpu: Fix memleak in x86_cpu_class_check_missing_features
'err' forgot to free in x86_cpu_class_check_missing_features error path.
Fix that.
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
Reviewed-by: Li Qiang
---
Cc: Paolo Bonzini
Cc: Richard Henderson
Cc: Eduardo Habkost
---
- V2: no changes in v2.
---
target/i386/cpu.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 588f32e136..4678aac0b4 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -4872,6 +4872,7 @@ static void
x86_cpu_class_check_missing_features(X86CPUClass *xcc,
new->value = g_strdup("type");
*next = new;
next = >next;
+error_free(err);
}
x86_cpu_filter_features(xc, false);
--
2.18.2
[PATCH v2 00/10] fix some error memleaks
This series fix some Error/GError memleaks. V2: 1. remove two patches.(One has aleardy applied. The other has fixed.) 2. change patch 5/10 and 7/10. Pan Nengyuan (10): qga/channel-posix: Plug memory leak in ga_channel_write_all() elf2dmp/qemu_elf: Plug memleak in QEMU_Elf_init elf2dmp/pdb: Plug memleak in pdb_init_from_file target/i386/sev: Plug memleak in sev_read_file_base64 ui/gtk-gl-area: Plug memleak in gd_gl_area_create_context() target/i386/cpu: Fix memleak in x86_cpu_class_check_missing_features migration/colo: Plug memleaks in colo_process_incoming_thread blockdev: Fix a memleak in drive_backup_prepare() block/file-posix: fix a possible undefined behavior vnc-auth-sasl: Plug memleak in vnc_socket_ip_addr_string block/file-posix.c | 2 +- blockdev.c | 1 + contrib/elf2dmp/pdb.c | 1 + contrib/elf2dmp/qemu_elf.c | 1 + migration/colo.c | 5 - qga/channel-posix.c| 6 +- target/i386/cpu.c | 1 + target/i386/sev.c | 1 + ui/gtk-gl-area.c | 11 +++ ui/vnc-auth-sasl.c | 1 + 10 files changed, 27 insertions(+), 3 deletions(-) -- 2.18.2
[PATCH v2 03/10] elf2dmp/pdb: Plug memleak in pdb_init_from_file
Missing g_error_free in pdb_init_from_file() error path. Fix that.
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
Reviewed-by: Viktor Prutyanov
Reviewed-by: Li Qiang
---
Cc: Viktor Prutyanov
---
- v2: no changes in v2
---
contrib/elf2dmp/pdb.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/contrib/elf2dmp/pdb.c b/contrib/elf2dmp/pdb.c
index a5bd40c99d..b3a6547068 100644
--- a/contrib/elf2dmp/pdb.c
+++ b/contrib/elf2dmp/pdb.c
@@ -285,6 +285,7 @@ int pdb_init_from_file(const char *name, struct pdb_reader
*reader)
reader->gmf = g_mapped_file_new(name, TRUE, );
if (gerr) {
eprintf("Failed to map PDB file \'%s\'\n", name);
+g_error_free(gerr);
return 1;
}
--
2.18.2
[PATCH v2 02/10] elf2dmp/qemu_elf: Plug memleak in QEMU_Elf_init
Missing g_error_free in QEMU_Elf_init() error path. Fix that.
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
Reviewed-by: Viktor Prutyanov
Reviewed-by: Li Qiang
---
Cc: Viktor Prutyanov
---
- v2: no changes in v2
---
contrib/elf2dmp/qemu_elf.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/contrib/elf2dmp/qemu_elf.c b/contrib/elf2dmp/qemu_elf.c
index 0db7816586..b601b6d7ba 100644
--- a/contrib/elf2dmp/qemu_elf.c
+++ b/contrib/elf2dmp/qemu_elf.c
@@ -126,6 +126,7 @@ int QEMU_Elf_init(QEMU_Elf *qe, const char *filename)
qe->gmf = g_mapped_file_new(filename, TRUE, );
if (gerr) {
eprintf("Failed to map ELF dump file \'%s\'\n", filename);
+g_error_free(gerr);
return 1;
}
--
2.18.2
[PATCH v2 04/10] target/i386/sev: Plug memleak in sev_read_file_base64
Missing g_error_free() in sev_read_file_base64() error path.
Fix that.
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
Reviewed-by: Li Qiang
---
Cc: Paolo Bonzini
Cc: Richard Henderson
Cc: Eduardo Habkost
---
- v2: no changes in v2
---
target/i386/sev.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/target/i386/sev.c b/target/i386/sev.c
index c3ecf86704..de4818da6d 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -500,6 +500,7 @@ sev_read_file_base64(const char *filename, guchar **data,
gsize *len)
if (!g_file_get_contents(filename, , , )) {
error_report("failed to read '%s' (%s)", filename, error->message);
+g_error_free(error);
return -1;
}
--
2.18.2
[PATCH v2 01/10] qga/channel-posix: Plug memory leak in ga_channel_write_all()
Missing g_error_free on error path in ga_channel_write_all(). Fix that.
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
Reviewed-by: Li Qiang
---
Cc: Michael Roth
---
- V2: no changes in v2
---
qga/channel-posix.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/qga/channel-posix.c b/qga/channel-posix.c
index 8fc205ad21..0b151cb87b 100644
--- a/qga/channel-posix.c
+++ b/qga/channel-posix.c
@@ -249,7 +249,7 @@ GIOStatus ga_channel_write_all(GAChannel *c, const gchar
*buf, gsize size)
buf += written;
} else if (status != G_IO_STATUS_AGAIN) {
g_warning("error writing to channel: %s", err->message);
-return status;
+goto out;
}
}
@@ -261,6 +261,10 @@ GIOStatus ga_channel_write_all(GAChannel *c, const gchar
*buf, gsize size)
g_warning("error flushing channel: %s", err->message);
}
+out:
+if (err) {
+g_error_free(err);
+}
return status;
}
--
2.18.2
[PATCH v2 08/10] blockdev: Fix a memleak in drive_backup_prepare()
'local_err' seems forgot to propagate in error path, it'll cause
a memleak. Fix it.
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
Reviewed-by: Kevin Wolf
Reviewed-by: Li Qiang
---
Cc: Kevin Wolf
Cc: Max Reitz
Cc: Markus Armbruster
Cc: qemu-bl...@nongnu.org
---
- V2: no changes in v2.
---
blockdev.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/blockdev.c b/blockdev.c
index 3848a9c8ab..842ac289c1 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1801,6 +1801,7 @@ static void drive_backup_prepare(BlkActionState *common,
Error **errp)
if (set_backing_hd) {
bdrv_set_backing_hd(target_bs, source, _err);
if (local_err) {
+error_propagate(errp, local_err);
goto unref;
}
}
--
2.18.2
[PATCH v2 07/10] migration/colo: Plug memleaks in colo_process_incoming_thread
'local_err' forgot to free in colo_process_incoming_thread error path.
Fix that.
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
---
Cc: Hailiang Zhang
Cc: Juan Quintela
Cc: "Dr. David Alan Gilbert"
---
- V2: Arrange all 'error_report_err' in 'out' label(suggested by Li Qiang).
---
migration/colo.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/migration/colo.c b/migration/colo.c
index ea7d1e9d4e..2288188fe2 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -888,7 +888,6 @@ void *colo_process_incoming_thread(void *opaque)
while (mis->state == MIGRATION_STATUS_COLO) {
colo_wait_handle_message(mis, fb, bioc, _err);
if (local_err) {
-error_report_err(local_err);
break;
}
@@ -924,6 +923,10 @@ out:
qemu_fclose(fb);
}
+if (local_err) {
+error_report_err(local_err);
+}
+
/* Hope this not to be too long to loop here */
qemu_sem_wait(>colo_incoming_sem);
qemu_sem_destroy(>colo_incoming_sem);
--
2.18.2
Re: [PATCH 06/12] ui/gtk-gl-area: Plug memleak in gd_gl_area_create_context()
On 2020/8/26 20:20, Li Qiang wrote:
> Pan Nengyuan 于2020年8月14日周五 下午6:15写道：
>>
>> Receiving error in local variable err, and forgot to free it.
>> Considering that there is no place to deal with it. Clean up.
>>
>> Reported-by: Euler Robot
>> Signed-off-by: Pan Nengyuan
>> ---
>> Cc: Gerd Hoffmann
>> ---
>> ui/gtk-gl-area.c | 5 ++---
>> 1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/ui/gtk-gl-area.c b/ui/gtk-gl-area.c
>> index 85f9d14c51..c740a7eb14 100644
>> --- a/ui/gtk-gl-area.c
>> +++ b/ui/gtk-gl-area.c
>> @@ -142,15 +142,14 @@ QEMUGLContext
>> gd_gl_area_create_context(DisplayChangeListener *dcl,
>> VirtualConsole *vc = container_of(dcl, VirtualConsole, gfx.dcl);
>> GdkWindow *window;
>> GdkGLContext *ctx;
>> -GError *err = NULL;
>>
>> gtk_gl_area_make_current(GTK_GL_AREA(vc->gfx.drawing_area));
>> window = gtk_widget_get_window(vc->gfx.drawing_area);
>> -ctx = gdk_window_create_gl_context(window, );
>> +ctx = gdk_window_create_gl_context(window, NULL);
>> gdk_gl_context_set_required_version(ctx,
>> params->major_ver,
>> params->minor_ver);
>> -gdk_gl_context_realize(ctx, );
>> +gdk_gl_context_realize(ctx, NULL);
>> return ctx;
>> }
>
> Maybe we should check the return value of 'gdk_window_create_gl_context'
> and 'gdk_gl_context_realize' instead of omitting it?
OK, Agree with you.
How about check the value like the below?
(Return NULL when error happens in gdk_gl_context_realize. It's different from
the original.)
Thanks.
diff --git a/ui/gtk-gl-area.c b/ui/gtk-gl-area.c
index 85f9d14c51..98c22d23f5 100644
--- a/ui/gtk-gl-area.c
+++ b/ui/gtk-gl-area.c
@@ -147,10 +147,21 @@ QEMUGLContext
gd_gl_area_create_context(DisplayChangeListener *dcl,
gtk_gl_area_make_current(GTK_GL_AREA(vc->gfx.drawing_area));
window = gtk_widget_get_window(vc->gfx.drawing_area);
ctx = gdk_window_create_gl_context(window, );
+if (err) {
+g_printerr("Create gdk gl context failed: %s\n", err->message);
+g_error_free(err);
+return NULL;
+}
gdk_gl_context_set_required_version(ctx,
params->major_ver,
params->minor_ver);
gdk_gl_context_realize(ctx, );
+if (err) {
+g_printerr("Realize gdk gl context failed: %s\n", err->message);
+g_error_free(err);
+g_clear_object();
+return NULL;
+}
return ctx;
}
>
> Thanks,
> Li Qiang
>
>>
>
>> --
>> 2.18.2
>>
>>
> .
>
Re: [PATCH 08/12] migration/colo: Plug memleaks in colo_process_incoming_thread
On 2020/8/26 20:37, Li Qiang wrote:
> Pan Nengyuan 于2020年8月14日周五 下午6:52写道：
>>
>> 'local_err' forgot to free in colo_process_incoming_thread error path.
>> Fix that.
>>
>> Reported-by: Euler Robot
>> Signed-off-by: Pan Nengyuan
>> ---
>> Cc: Hailiang Zhang
>> Cc: Juan Quintela
>> Cc: "Dr. David Alan Gilbert"
>> ---
>> migration/colo.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/migration/colo.c b/migration/colo.c
>> index ea7d1e9d4e..17b5afc6b5 100644
>> --- a/migration/colo.c
>> +++ b/migration/colo.c
>> @@ -870,6 +870,7 @@ void *colo_process_incoming_thread(void *opaque)
>> replication_start_all(REPLICATION_MODE_SECONDARY, _err);
>> if (local_err) {
>> qemu_mutex_unlock_iothread();
>> +error_report_err(local_err);
>> goto out;
>> }
>> #else
>> @@ -882,6 +883,7 @@ void *colo_process_incoming_thread(void *opaque)
>> colo_send_message(mis->to_src_file, COLO_MESSAGE_CHECKPOINT_READY,
>>_err);
>> if (local_err) {
>> +error_report_err(local_err);
>> goto out;
>> }
>>
>
> Could we arrange 'error_report_err' in 'out' label?
> Like this:
>
> if (local_err) {
> error_report_err(local_err);
> }
Similar to the other place in the same function, I didn't arrange them in 'out'
label:
while (mis->state == MIGRATION_STATUS_COLO) {
colo_wait_handle_message(mis, fb, bioc, _err);
if (local_err) {
error_report_err(local_err);
break;
}
But I think it's a good idea to arrange them in 'out' label. I will change it.
Thanks.
>
> Thanks,
> Li Qiang
>
>
>
>> --
>> 2.18.2
>>
>>
> .
>
Re: [PATCH 00/12] fix some error memleaks
ping! On 2020/8/15 0:02, Pan Nengyuan wrote: > This series fix some Error/GError memleaks. > > Pan Nengyuan (12): > qga/channel-posix: Plug memory leak in ga_channel_write_all() > hw/vfio/ap: Plug memleak in vfio_ap_get_group() > elf2dmp/qemu_elf: Plug memleak in QEMU_Elf_init > elf2dmp/pdb: Plug memleak in pdb_init_from_file > target/i386/sev: Plug memleak in sev_read_file_base64 > ui/gtk-gl-area: Plug memleak in gd_gl_area_create_context() > target/i386/cpu: Fix memleak in x86_cpu_class_check_missing_features > migration/colo: Plug memleaks in colo_process_incoming_thread > blockdev: Fix a memleak in drive_backup_prepare() > block/file-posix: fix a possible undefined behavior > vnc-auth-sasl: Plug memleak in vnc_socket_ip_addr_string > test-util-sockets: Fix a memleak in test_socket_unix_abstract_good > > block/file-posix.c | 2 +- > blockdev.c | 1 + > contrib/elf2dmp/pdb.c | 1 + > contrib/elf2dmp/qemu_elf.c | 1 + > hw/vfio/ap.c | 1 + > migration/colo.c | 2 ++ > qga/channel-posix.c| 6 +- > target/i386/cpu.c | 1 + > target/i386/sev.c | 1 + > tests/test-util-sockets.c | 1 + > ui/gtk-gl-area.c | 5 ++--- > ui/vnc-auth-sasl.c | 1 + > 12 files changed, 18 insertions(+), 5 deletions(-) >
Re: [PATCH 12/12] test-util-sockets: Fix a memleak in test_socket_unix_abstract_good
On 2020/8/14 22:50, Li Qiang wrote: > Pan Nengyuan 于2020年8月14日周五 下午6:18写道： >> >> Fix a memleak in test_socket_unix_abstract_good(). >> >> Reported-by: Euler Robot >> Signed-off-by: Pan Nengyuan >> --- > > Hi Nengyuan, > I have sent this two month ago: > -->https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg00809.html > > seems the maintainer forget to push it to upstream. Yes, it's the same. Let's ignore this one. Thanks. > > Thanks, > Li Qiang > > >> tests/test-util-sockets.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c >> index 261dc48c03..5c4204a130 100644 >> --- a/tests/test-util-sockets.c >> +++ b/tests/test-util-sockets.c >> @@ -312,6 +312,7 @@ static void test_socket_unix_abstract_good(void) >> g_thread_join(cli); >> g_thread_join(serv); >> >> +g_rand_free(r); >> g_free(abstract_sock_name); >> } >> #endif >> -- >> 2.18.2 >> >>
[PATCH 09/12] blockdev: Fix a memleak in drive_backup_prepare()
'local_err' seems forgot to propagate in error path, it'll cause
a memleak. Fix it.
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
---
Cc: Kevin Wolf
Cc: Max Reitz
Cc: Markus Armbruster
Cc: qemu-bl...@nongnu.org
---
blockdev.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/blockdev.c b/blockdev.c
index 3848a9c8ab..842ac289c1 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1801,6 +1801,7 @@ static void drive_backup_prepare(BlkActionState *common,
Error **errp)
if (set_backing_hd) {
bdrv_set_backing_hd(target_bs, source, _err);
if (local_err) {
+error_propagate(errp, local_err);
goto unref;
}
}
--
2.18.2
[PATCH 08/12] migration/colo: Plug memleaks in colo_process_incoming_thread
'local_err' forgot to free in colo_process_incoming_thread error path.
Fix that.
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
---
Cc: Hailiang Zhang
Cc: Juan Quintela
Cc: "Dr. David Alan Gilbert"
---
migration/colo.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/migration/colo.c b/migration/colo.c
index ea7d1e9d4e..17b5afc6b5 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -870,6 +870,7 @@ void *colo_process_incoming_thread(void *opaque)
replication_start_all(REPLICATION_MODE_SECONDARY, _err);
if (local_err) {
qemu_mutex_unlock_iothread();
+error_report_err(local_err);
goto out;
}
#else
@@ -882,6 +883,7 @@ void *colo_process_incoming_thread(void *opaque)
colo_send_message(mis->to_src_file, COLO_MESSAGE_CHECKPOINT_READY,
_err);
if (local_err) {
+error_report_err(local_err);
goto out;
}
--
2.18.2
[PATCH 04/12] elf2dmp/pdb: Plug memleak in pdb_init_from_file
Missing g_error_free in pdb_init_from_file() error path. Fix that.
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
---
Cc: Viktor Prutyanov
---
contrib/elf2dmp/pdb.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/contrib/elf2dmp/pdb.c b/contrib/elf2dmp/pdb.c
index a5bd40c99d..b3a6547068 100644
--- a/contrib/elf2dmp/pdb.c
+++ b/contrib/elf2dmp/pdb.c
@@ -285,6 +285,7 @@ int pdb_init_from_file(const char *name, struct pdb_reader
*reader)
reader->gmf = g_mapped_file_new(name, TRUE, );
if (gerr) {
eprintf("Failed to map PDB file \'%s\'\n", name);
+g_error_free(gerr);
return 1;
}
--
2.18.2
[PATCH 00/12] fix some error memleaks
This series fix some Error/GError memleaks. Pan Nengyuan (12): qga/channel-posix: Plug memory leak in ga_channel_write_all() hw/vfio/ap: Plug memleak in vfio_ap_get_group() elf2dmp/qemu_elf: Plug memleak in QEMU_Elf_init elf2dmp/pdb: Plug memleak in pdb_init_from_file target/i386/sev: Plug memleak in sev_read_file_base64 ui/gtk-gl-area: Plug memleak in gd_gl_area_create_context() target/i386/cpu: Fix memleak in x86_cpu_class_check_missing_features migration/colo: Plug memleaks in colo_process_incoming_thread blockdev: Fix a memleak in drive_backup_prepare() block/file-posix: fix a possible undefined behavior vnc-auth-sasl: Plug memleak in vnc_socket_ip_addr_string test-util-sockets: Fix a memleak in test_socket_unix_abstract_good block/file-posix.c | 2 +- blockdev.c | 1 + contrib/elf2dmp/pdb.c | 1 + contrib/elf2dmp/qemu_elf.c | 1 + hw/vfio/ap.c | 1 + migration/colo.c | 2 ++ qga/channel-posix.c| 6 +- target/i386/cpu.c | 1 + target/i386/sev.c | 1 + tests/test-util-sockets.c | 1 + ui/gtk-gl-area.c | 5 ++--- ui/vnc-auth-sasl.c | 1 + 12 files changed, 18 insertions(+), 5 deletions(-) -- 2.18.2
[PATCH 11/12] vnc-auth-sasl: Plug memleak in vnc_socket_ip_addr_string
'addr' forgot to free in vnc_socket_ip_addr_string error path. Fix that.
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
---
Cc: Gerd Hoffmann
---
ui/vnc-auth-sasl.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/ui/vnc-auth-sasl.c b/ui/vnc-auth-sasl.c
index 7b2b09f242..0517b2ead9 100644
--- a/ui/vnc-auth-sasl.c
+++ b/ui/vnc-auth-sasl.c
@@ -522,6 +522,7 @@ vnc_socket_ip_addr_string(QIOChannelSocket *ioc,
if (addr->type != SOCKET_ADDRESS_TYPE_INET) {
error_setg(errp, "Not an inet socket type");
+qapi_free_SocketAddress(addr);
return NULL;
}
ret = g_strdup_printf("%s;%s", addr->u.inet.host, addr->u.inet.port);
--
2.18.2
[PATCH 07/12] target/i386/cpu: Fix memleak in x86_cpu_class_check_missing_features
'err' forgot to free in x86_cpu_class_check_missing_features error path.
Fix that.
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
---
Cc: Paolo Bonzini
Cc: Richard Henderson
Cc: Eduardo Habkost
---
target/i386/cpu.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 588f32e136..4678aac0b4 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -4872,6 +4872,7 @@ static void
x86_cpu_class_check_missing_features(X86CPUClass *xcc,
new->value = g_strdup("type");
*next = new;
next = >next;
+error_free(err);
}
x86_cpu_filter_features(xc, false);
--
2.18.2
[PATCH 05/12] target/i386/sev: Plug memleak in sev_read_file_base64
Missing g_error_free() in sev_read_file_base64() error path.
Fix that.
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
---
Cc: Paolo Bonzini
Cc: Richard Henderson
Cc: Eduardo Habkost
---
target/i386/sev.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/target/i386/sev.c b/target/i386/sev.c
index c3ecf86704..de4818da6d 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -500,6 +500,7 @@ sev_read_file_base64(const char *filename, guchar **data,
gsize *len)
if (!g_file_get_contents(filename, , , )) {
error_report("failed to read '%s' (%s)", filename, error->message);
+g_error_free(error);
return -1;
}
--
2.18.2
[PATCH 10/12] block/file-posix: fix a possible undefined behavior
local_err is not initialized to NULL, it will cause a assert error as below:
qemu/util/error.c:59: error_setv: Assertion `*errp == NULL' failed.
Fixes: c6447510690
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
---
Cc: Kevin Wolf
Cc: Max Reitz
Cc: Aarushi Mehta
Cc: qemu-bl...@nongnu.org
---
block/file-posix.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index 9a00d4190a..697a7d9eea 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2113,7 +2113,7 @@ static void raw_aio_attach_aio_context(BlockDriverState
*bs,
#endif
#ifdef CONFIG_LINUX_IO_URING
if (s->use_linux_io_uring) {
-Error *local_err;
+Error *local_err = NULL;
if (!aio_setup_linux_io_uring(new_context, _err)) {
error_reportf_err(local_err, "Unable to use linux io_uring, "
"falling back to thread pool: ");
--
2.18.2
[PATCH 01/12] qga/channel-posix: Plug memory leak in ga_channel_write_all()
Missing g_error_free on error path in ga_channel_write_all(). Fix that.
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
---
Cc: Michael Roth
---
qga/channel-posix.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/qga/channel-posix.c b/qga/channel-posix.c
index 8fc205ad21..0b151cb87b 100644
--- a/qga/channel-posix.c
+++ b/qga/channel-posix.c
@@ -249,7 +249,7 @@ GIOStatus ga_channel_write_all(GAChannel *c, const gchar
*buf, gsize size)
buf += written;
} else if (status != G_IO_STATUS_AGAIN) {
g_warning("error writing to channel: %s", err->message);
-return status;
+goto out;
}
}
@@ -261,6 +261,10 @@ GIOStatus ga_channel_write_all(GAChannel *c, const gchar
*buf, gsize size)
g_warning("error flushing channel: %s", err->message);
}
+out:
+if (err) {
+g_error_free(err);
+}
return status;
}
--
2.18.2
[PATCH 02/12] hw/vfio/ap: Plug memleak in vfio_ap_get_group()
Missing g_error_free() in vfio_ap_get_group() error path. Fix that.
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
---
Cc: Cornelia Huck
Cc: Thomas Huth
Cc: Christian Borntraeger
Cc: Tony Krowiak
Cc: Halil Pasic
Cc: Pierre Morel
Cc: Alex Williamson
Cc: qemu-s3...@nongnu.org
---
hw/vfio/ap.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
index b9330a8e6f..cec6fe1599 100644
--- a/hw/vfio/ap.c
+++ b/hw/vfio/ap.c
@@ -71,6 +71,7 @@ static VFIOGroup *vfio_ap_get_group(VFIOAPDevice *vapdev,
Error **errp)
if (!group_path) {
error_setg(errp, "%s: no iommu_group found for %s: %s",
VFIO_AP_DEVICE_TYPE, vapdev->vdev.sysfsdev,
gerror->message);
+g_error_free(gerror);
return NULL;
}
--
2.18.2
[PATCH 03/12] elf2dmp/qemu_elf: Plug memleak in QEMU_Elf_init
Missing g_error_free in QEMU_Elf_init() error path. Fix that.
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
---
Cc: Viktor Prutyanov
---
contrib/elf2dmp/qemu_elf.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/contrib/elf2dmp/qemu_elf.c b/contrib/elf2dmp/qemu_elf.c
index 0db7816586..b601b6d7ba 100644
--- a/contrib/elf2dmp/qemu_elf.c
+++ b/contrib/elf2dmp/qemu_elf.c
@@ -126,6 +126,7 @@ int QEMU_Elf_init(QEMU_Elf *qe, const char *filename)
qe->gmf = g_mapped_file_new(filename, TRUE, );
if (gerr) {
eprintf("Failed to map ELF dump file \'%s\'\n", filename);
+g_error_free(gerr);
return 1;
}
--
2.18.2
[PATCH 12/12] test-util-sockets: Fix a memleak in test_socket_unix_abstract_good
Fix a memleak in test_socket_unix_abstract_good(). Reported-by: Euler Robot Signed-off-by: Pan Nengyuan --- tests/test-util-sockets.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test-util-sockets.c b/tests/test-util-sockets.c index 261dc48c03..5c4204a130 100644 --- a/tests/test-util-sockets.c +++ b/tests/test-util-sockets.c @@ -312,6 +312,7 @@ static void test_socket_unix_abstract_good(void) g_thread_join(cli); g_thread_join(serv); +g_rand_free(r); g_free(abstract_sock_name); } #endif -- 2.18.2
[PATCH 06/12] ui/gtk-gl-area: Plug memleak in gd_gl_area_create_context()
Receiving error in local variable err, and forgot to free it. Considering that there is no place to deal with it. Clean up. Reported-by: Euler Robot Signed-off-by: Pan Nengyuan --- Cc: Gerd Hoffmann --- ui/gtk-gl-area.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/ui/gtk-gl-area.c b/ui/gtk-gl-area.c index 85f9d14c51..c740a7eb14 100644 --- a/ui/gtk-gl-area.c +++ b/ui/gtk-gl-area.c @@ -142,15 +142,14 @@ QEMUGLContext gd_gl_area_create_context(DisplayChangeListener *dcl, VirtualConsole *vc = container_of(dcl, VirtualConsole, gfx.dcl); GdkWindow *window; GdkGLContext *ctx; -GError *err = NULL; gtk_gl_area_make_current(GTK_GL_AREA(vc->gfx.drawing_area)); window = gtk_widget_get_window(vc->gfx.drawing_area); -ctx = gdk_window_create_gl_context(window, ); +ctx = gdk_window_create_gl_context(window, NULL); gdk_gl_context_set_required_version(ctx, params->major_ver, params->minor_ver); -gdk_gl_context_realize(ctx, ); +gdk_gl_context_realize(ctx, NULL); return ctx; } -- 2.18.2
Re: [PATCH] qom-hmp-cmds: fix a memleak in hmp_qom_get
On 6/3/2020 2:51 PM, Philippe Mathieu-Daudé wrote: > Hi Pan, > > On 6/3/20 9:03 AM, Pan Nengyuan wrote: >> 'obj' forgot to free at the end of hmp_qom_get(). Fix that. >> >> The leak stack: >> Direct leak of 40 byte(s) in 1 object(s) allocated from: >> #0 0x7f4e3a779ae8 in __interceptor_malloc (/lib64/libasan.so.5+0xefae8) >> #1 0x7f4e398f91d5 in g_malloc (/lib64/libglib-2.0.so.0+0x531d5) >> #2 0x55c9fd9a3999 in qstring_from_substr >> /build/qemu/src/qobject/qstring.c:45 >> #3 0x55c9fd894bd3 in qobject_output_type_str >> /build/qemu/src/qapi/qobject-output-visitor.c:175 >> #4 0x55c9fd894bd3 in qobject_output_type_str >> /build/qemu/src/qapi/qobject-output-visitor.c:168 >> #5 0x55c9fd88b34d in visit_type_str >> /build/qemu/src/qapi/qapi-visit-core.c:308 >> #6 0x55c9fd59aa6b in property_get_str /build/qemu/src/qom/object.c:2064 >> #7 0x55c9fd5adb8a in object_property_get_qobject >> /build/qemu/src/qom/qom-qobject.c:38 >> #8 0x55c9fd4a029d in hmp_qom_get /build/qemu/src/qom/qom-hmp-cmds.c:66 >> >> Fixes: 89cf4fe34f4 > > When you fix a bug in a specific commit, please Cc the commit author and > the reviewers, so they have a chance to 1/ review your fix and 2/ learn > from their mistake. Ok, I will do next time. > >> Reported-by: Euler Robot >> Signed-off-by: Pan Nengyuan >> --- >> qom/qom-hmp-cmds.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/qom/qom-hmp-cmds.c b/qom/qom-hmp-cmds.c >> index f704b6949a..3d2a23292d 100644 >> --- a/qom/qom-hmp-cmds.c >> +++ b/qom/qom-hmp-cmds.c >> @@ -71,6 +71,7 @@ void hmp_qom_get(Monitor *mon, const QDict *qdict) >> qobject_unref(str); >> } >> >> +qobject_unref(obj); > > The object_resolve_path() documentation is not clear about that... > Can we get the documentation clarified? I'm not sure why is object_resolve_path(). In this case, 'obj' is returned from object_property_get_qobject() in qmp_qom_get(). Do you mean document object_property_get_qobject() to make it more clear? Like: - The returned object has a reference count of 1, and will be freed when the last reference is dropped. Thanks. > >> hmp_handle_error(mon, err); >> } >> >> >
[PATCH] qom-hmp-cmds: fix a memleak in hmp_qom_get
'obj' forgot to free at the end of hmp_qom_get(). Fix that. The leak stack: Direct leak of 40 byte(s) in 1 object(s) allocated from: #0 0x7f4e3a779ae8 in __interceptor_malloc (/lib64/libasan.so.5+0xefae8) #1 0x7f4e398f91d5 in g_malloc (/lib64/libglib-2.0.so.0+0x531d5) #2 0x55c9fd9a3999 in qstring_from_substr /build/qemu/src/qobject/qstring.c:45 #3 0x55c9fd894bd3 in qobject_output_type_str /build/qemu/src/qapi/qobject-output-visitor.c:175 #4 0x55c9fd894bd3 in qobject_output_type_str /build/qemu/src/qapi/qobject-output-visitor.c:168 #5 0x55c9fd88b34d in visit_type_str /build/qemu/src/qapi/qapi-visit-core.c:308 #6 0x55c9fd59aa6b in property_get_str /build/qemu/src/qom/object.c:2064 #7 0x55c9fd5adb8a in object_property_get_qobject /build/qemu/src/qom/qom-qobject.c:38 #8 0x55c9fd4a029d in hmp_qom_get /build/qemu/src/qom/qom-hmp-cmds.c:66 Fixes: 89cf4fe34f4 Reported-by: Euler Robot Signed-off-by: Pan Nengyuan --- qom/qom-hmp-cmds.c | 1 + 1 file changed, 1 insertion(+) diff --git a/qom/qom-hmp-cmds.c b/qom/qom-hmp-cmds.c index f704b6949a..3d2a23292d 100644 --- a/qom/qom-hmp-cmds.c +++ b/qom/qom-hmp-cmds.c @@ -71,6 +71,7 @@ void hmp_qom_get(Monitor *mon, const QDict *qdict) qobject_unref(str); } +qobject_unref(obj); hmp_handle_error(mon, err); } -- 2.18.2
[PATCH v2] i386/kvm: fix a use-after-free when vcpu plug/unplug
When we hotplug vcpus, cpu_update_state is added to vm_change_state_head
in kvm_arch_init_vcpu(). But it forgot to delete in kvm_arch_destroy_vcpu()
after
unplug. Then it will cause a use-after-free access. This patch delete it in
kvm_arch_destroy_vcpu() to fix that.
Reproducer:
virsh setvcpus vm1 4 --live
virsh setvcpus vm1 2 --live
virsh suspend vm1
virsh resume vm1
The UAF stack:
==qemu-system-x86_64==28233==ERROR: AddressSanitizer: heap-use-after-free on
address 0x62e2e798 at pc 0x5573c6917d9e bp 0x7fff07139e50 sp 0x7fff07139e40
WRITE of size 1 at 0x62e2e798 thread T0
#0 0x5573c6917d9d in cpu_update_state /mnt/sdb/qemu/target/i386/kvm.c:742
#1 0x5573c699121a in vm_state_notify /mnt/sdb/qemu/vl.c:1290
#2 0x5573c636287e in vm_prepare_start /mnt/sdb/qemu/cpus.c:2144
#3 0x5573c6362927 in vm_start /mnt/sdb/qemu/cpus.c:2150
#4 0x5573c71e8304 in qmp_cont /mnt/sdb/qemu/monitor/qmp-cmds.c:173
#5 0x5573c727cb1e in qmp_marshal_cont qapi/qapi-commands-misc.c:835
#6 0x5573c7694c7a in do_qmp_dispatch /mnt/sdb/qemu/qapi/qmp-dispatch.c:132
#7 0x5573c7694c7a in qmp_dispatch /mnt/sdb/qemu/qapi/qmp-dispatch.c:175
#8 0x5573c71d9110 in monitor_qmp_dispatch /mnt/sdb/qemu/monitor/qmp.c:145
#9 0x5573c71dad4f in monitor_qmp_bh_dispatcher
/mnt/sdb/qemu/monitor/qmp.c:234
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
Reviewed-by: Philippe Mathieu-Daudé
---
- v2: remove unnecessary set vmsentry to null(there is no non-null check).
---
target/i386/cpu.h | 1 +
target/i386/kvm.c | 4 +++-
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index e818fc712a..afbd11b7a3 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1631,6 +1631,7 @@ struct X86CPU {
CPUNegativeOffsetState neg;
CPUX86State env;
+VMChangeStateEntry *vmsentry;
uint64_t ucode_rev;
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index 4901c6dd74..0a4eca5a85 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -1770,7 +1770,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
}
}
-qemu_add_vm_change_state_handler(cpu_update_state, env);
+cpu->vmsentry = qemu_add_vm_change_state_handler(cpu_update_state, env);
c = cpuid_find_entry(_data.cpuid, 1, 0);
if (c) {
@@ -1883,6 +1883,8 @@ int kvm_arch_destroy_vcpu(CPUState *cs)
env->nested_state = NULL;
}
+qemu_del_vm_change_state_handler(cpu->vmsentry);
+
return 0;
}
--
2.18.2
Re: [PATCH] i386/kvm: fix a use-after-free when vcpu plug/unplug
On 5/12/2020 3:54 PM, Philippe Mathieu-Daudé wrote:
> On 5/12/20 3:39 PM, Pan Nengyuan wrote:
>> When we hotplug vcpus, cpu_update_state is added to vm_change_state_head
>> in kvm_arch_init_vcpu(). But it forgot to delete in kvm_arch_destroy_vcpu()
>> after
>> unplug. Then it will cause a use-after-free access. This patch delete it in
>> kvm_arch_destroy_vcpu() to fix that.
>>
>> Reproducer:
>> virsh setvcpus vm1 4 --live
>> virsh setvcpus vm1 2 --live
>> virsh suspend vm1
>> virsh resume vm1
>>
>> The UAF stack:
>> ==qemu-system-x86_64==28233==ERROR: AddressSanitizer: heap-use-after-free on
>> address 0x62e2e798 at pc 0x5573c6917d9e bp 0x7fff07139e50 sp
>> 0x7fff07139e40
>> WRITE of size 1 at 0x62e2e798 thread T0
>> #0 0x5573c6917d9d in cpu_update_state
>> /mnt/sdb/qemu/target/i386/kvm.c:742
>> #1 0x5573c699121a in vm_state_notify /mnt/sdb/qemu/vl.c:1290
>> #2 0x5573c636287e in vm_prepare_start /mnt/sdb/qemu/cpus.c:2144
>> #3 0x5573c6362927 in vm_start /mnt/sdb/qemu/cpus.c:2150
>> #4 0x5573c71e8304 in qmp_cont /mnt/sdb/qemu/monitor/qmp-cmds.c:173
>> #5 0x5573c727cb1e in qmp_marshal_cont qapi/qapi-commands-misc.c:835
>> #6 0x5573c7694c7a in do_qmp_dispatch
>> /mnt/sdb/qemu/qapi/qmp-dispatch.c:132
>> #7 0x5573c7694c7a in qmp_dispatch /mnt/sdb/qemu/qapi/qmp-dispatch.c:175
>> #8 0x5573c71d9110 in monitor_qmp_dispatch
>> /mnt/sdb/qemu/monitor/qmp.c:145
>> #9 0x5573c71dad4f in monitor_qmp_bh_dispatcher
>> /mnt/sdb/qemu/monitor/qmp.c:234
>>
>> Reported-by: Euler Robot
>> Signed-off-by: Pan Nengyuan
>> ---
>> target/i386/cpu.h | 1 +
>> target/i386/kvm.c | 5 -
>> 2 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
>> index e818fc712a..afbd11b7a3 100644
>> --- a/target/i386/cpu.h
>> +++ b/target/i386/cpu.h
>> @@ -1631,6 +1631,7 @@ struct X86CPU {
>> CPUNegativeOffsetState neg;
>> CPUX86State env;
>> + VMChangeStateEntry *vmsentry;
>> uint64_t ucode_rev;
>> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
>> index 4901c6dd74..ff2848357e 100644
>> --- a/target/i386/kvm.c
>> +++ b/target/i386/kvm.c
>> @@ -1770,7 +1770,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
>> }
>> }
>> - qemu_add_vm_change_state_handler(cpu_update_state, env);
>> + cpu->vmsentry = qemu_add_vm_change_state_handler(cpu_update_state, env);
>> c = cpuid_find_entry(_data.cpuid, 1, 0);
>> if (c) {
>> @@ -1883,6 +1883,9 @@ int kvm_arch_destroy_vcpu(CPUState *cs)
>> env->nested_state = NULL;
>> }
>> + qemu_del_vm_change_state_handler(cpu->vmsentry);
>> + cpu->vmsentry = NULL;
>
> Why set it to NULL? there is no non-NULL check.
>
> Anyway if it matters to you, why not do it in
> qemu_del_vm_change_state_handler()?
Yes, there is no non-NULL check and it will not be NULL at all.
Actually it doesn't matters to me, just habitually set to null after free.
I will remove it.
Thanks.
>
> Otherwise:
> Reviewed-by: Philippe Mathieu-Daudé
>
>> +
>> return 0;
>> }
>>
>
> .
[PATCH] i386/kvm: fix a use-after-free when vcpu plug/unplug
When we hotplug vcpus, cpu_update_state is added to vm_change_state_head
in kvm_arch_init_vcpu(). But it forgot to delete in kvm_arch_destroy_vcpu()
after
unplug. Then it will cause a use-after-free access. This patch delete it in
kvm_arch_destroy_vcpu() to fix that.
Reproducer:
virsh setvcpus vm1 4 --live
virsh setvcpus vm1 2 --live
virsh suspend vm1
virsh resume vm1
The UAF stack:
==qemu-system-x86_64==28233==ERROR: AddressSanitizer: heap-use-after-free on
address 0x62e2e798 at pc 0x5573c6917d9e bp 0x7fff07139e50 sp 0x7fff07139e40
WRITE of size 1 at 0x62e2e798 thread T0
#0 0x5573c6917d9d in cpu_update_state /mnt/sdb/qemu/target/i386/kvm.c:742
#1 0x5573c699121a in vm_state_notify /mnt/sdb/qemu/vl.c:1290
#2 0x5573c636287e in vm_prepare_start /mnt/sdb/qemu/cpus.c:2144
#3 0x5573c6362927 in vm_start /mnt/sdb/qemu/cpus.c:2150
#4 0x5573c71e8304 in qmp_cont /mnt/sdb/qemu/monitor/qmp-cmds.c:173
#5 0x5573c727cb1e in qmp_marshal_cont qapi/qapi-commands-misc.c:835
#6 0x5573c7694c7a in do_qmp_dispatch /mnt/sdb/qemu/qapi/qmp-dispatch.c:132
#7 0x5573c7694c7a in qmp_dispatch /mnt/sdb/qemu/qapi/qmp-dispatch.c:175
#8 0x5573c71d9110 in monitor_qmp_dispatch /mnt/sdb/qemu/monitor/qmp.c:145
#9 0x5573c71dad4f in monitor_qmp_bh_dispatcher
/mnt/sdb/qemu/monitor/qmp.c:234
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
---
target/i386/cpu.h | 1 +
target/i386/kvm.c | 5 -
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index e818fc712a..afbd11b7a3 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1631,6 +1631,7 @@ struct X86CPU {
CPUNegativeOffsetState neg;
CPUX86State env;
+VMChangeStateEntry *vmsentry;
uint64_t ucode_rev;
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index 4901c6dd74..ff2848357e 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -1770,7 +1770,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
}
}
-qemu_add_vm_change_state_handler(cpu_update_state, env);
+cpu->vmsentry = qemu_add_vm_change_state_handler(cpu_update_state, env);
c = cpuid_find_entry(_data.cpuid, 1, 0);
if (c) {
@@ -1883,6 +1883,9 @@ int kvm_arch_destroy_vcpu(CPUState *cs)
env->nested_state = NULL;
}
+qemu_del_vm_change_state_handler(cpu->vmsentry);
+cpu->vmsentry = NULL;
+
return 0;
}
--
2.18.2
[PATCH 2/2] migration/rdma: cleanup rdma context before g_free to avoid memleaks
When error happen in initializing 'rdma_return_path', we should cleanup rdma
context
before g_free(rdma) to avoid some memleaks. This patch fix that.
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
---
migration/rdma.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/migration/rdma.c b/migration/rdma.c
index 72e8b1c95b..ec45d33ba3 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -4094,20 +4094,20 @@ void rdma_start_outgoing_migration(void *opaque,
rdma_return_path = qemu_rdma_data_init(host_port, errp);
if (rdma_return_path == NULL) {
-goto err;
+goto return_path_err;
}
ret = qemu_rdma_source_init(rdma_return_path,
s->enabled_capabilities[MIGRATION_CAPABILITY_RDMA_PIN_ALL], errp);
if (ret) {
-goto err;
+goto return_path_err;
}
ret = qemu_rdma_connect(rdma_return_path, errp);
if (ret) {
-goto err;
+goto return_path_err;
}
rdma->return_path = rdma_return_path;
@@ -4120,6 +4120,8 @@ void rdma_start_outgoing_migration(void *opaque,
s->to_dst_file = qemu_fopen_rdma(rdma, "wb");
migrate_fd_connect(s, NULL);
return;
+return_path_err:
+qemu_rdma_cleanup(rdma);
err:
g_free(rdma);
g_free(rdma_return_path);
--
2.18.2
[PATCH 1/2] migration/rdma: fix potential nullptr access in rdma_start_incoming_migration
'rdma' is NULL when taking the first error branch in
rdma_start_incoming_migration.
And it will cause a null pointer access in label 'err'. Fix that.
Fixes: 59c59c67ee6b0327ae932deb303caa47919aeb1e
Signed-off-by: Pan Nengyuan
---
migration/rdma.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/migration/rdma.c b/migration/rdma.c
index 967fda5b0c..72e8b1c95b 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -4056,7 +4056,9 @@ void rdma_start_incoming_migration(const char *host_port,
Error **errp)
return;
err:
error_propagate(errp, local_err);
-g_free(rdma->host);
+if (rdma) {
+g_free(rdma->host);
+}
g_free(rdma);
g_free(rdma_return_path);
}
--
2.18.2
[PATCH 0/2] migration/rdma: fix nullptr-def in rdma_start_incoming_migration
I fix a memleak in rdma_start_incoming_migration some time ago. https://patchwork.kernel.org/patch/11498191/ I'm sorry that it may cause a null-pointer access, this patch fix that. Since we are here, rdma_start_outgoing_migration has the similar memleak, fix it together. Pan Nengyuan (2): migration/rdma: fix potential nullptr access in rdma_start_incoming_migration migration/rdma: cleanup rdma context before g_free to avoid memleaks migration/rdma.c | 12 1 file changed, 8 insertions(+), 4 deletions(-) -- 2.18.2
Re: [PATCH] migration/rdma: fix a memleak on error path in rdma_start_incoming_migration
On 4/24/2020 5:46 PM, Dr. David Alan Gilbert wrote:
> * Pan Nengyuan (pannengy...@huawei.com) wrote:
>> 'rdma->host' is malloced in qemu_rdma_data_init, but forgot to free on the
>> error
>> path in rdma_start_incoming_migration(), this patch fix that.
>>
>> The leak stack:
>> Direct leak of 2 byte(s) in 1 object(s) allocated from:
>> #0 0x7fb7add18ae8 in __interceptor_malloc (/lib64/libasan.so.5+0xefae8)
>> #1 0x7fb7ad0df1d5 in g_malloc (/lib64/libglib-2.0.so.0+0x531d5)
>> #2 0x7fb7ad0f8b32 in g_strdup (/lib64/libglib-2.0.so.0+0x6cb32)
>> #3 0x55a0464a0f6f in qemu_rdma_data_init
>> /mnt/sdb/qemu/migration/rdma.c:2647
>> #4 0x55a0464b0e76 in rdma_start_incoming_migration
>> /mnt/sdb/qemu/migration/rdma.c:4020
>> #5 0x55a0463f898a in qemu_start_incoming_migration
>> /mnt/sdb/qemu/migration/migration.c:365
>> #6 0x55a0458c75d3 in qemu_init /mnt/sdb/qemu/softmmu/vl.c:4438
>> #7 0x55a046a3d811 in main /mnt/sdb/qemu/softmmu/main.c:48
>> #8 0x7fb7a8417872 in __libc_start_main (/lib64/libc.so.6+0x23872)
>> #9 0x55a04536b26d in _start
>> (/mnt/sdb/qemu/build/x86_64-softmmu/qemu-system-x86_64+0x286926d)
>>
>> Reported-by: Euler Robot
>> Signed-off-by: Pan Nengyuan
>
> Thanks,
>
>
> Reviewed-by: Dr. David Alan Gilbert
>
>> ---
>> migration/rdma.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/migration/rdma.c b/migration/rdma.c
>> index f61587891b..967fda5b0c 100644
>> --- a/migration/rdma.c
>> +++ b/migration/rdma.c
>> @@ -4056,6 +4056,7 @@ void rdma_start_incoming_migration(const char
>> *host_port, Error **errp)
>> return;
>> err:
>> error_propagate(errp, local_err);
>> +g_free(rdma->host);
>> g_free(rdma);
>> g_free(rdma_return_path);
>> }
>> --
>> 2.18.2
>>
Oh, I'm sorry, this may cause a potential nullptr access when taking the first
error branch in rdma_start_incoming_migration:
rdma = qemu_rdma_data_init(host_port, _err);
if (rdma == NULL) {
goto err;
}
...
Since it has applied, I will send a new patch to fix it.
> --
> Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
>
> .
>
[PATCH 0/2] migration/multifd: fix two memleaks
Fix two memleaks in multifd_send_thread/multifd_new_send_channel_async when error happen. Pan Nengyuan (2): migration/multifd: fix memleaks in multifd_new_send_channel_async migration/multifd: Do error_free after migrate_set_error to avoid memleaks migration/multifd.c | 5 + 1 file changed, 5 insertions(+) -- 2.18.2
[PATCH 2/2] migration/multifd: Do error_free after migrate_set_error to avoid memleaks
When error happen in multifd_send_thread, it use error_copy to set migrate
error in
multifd_send_terminate_threads(). We should call error_free after it.
Similarly, fix another two places in multifd_recv_thread/multifd_save_cleanup.
The leak stack:
Direct leak of 48 byte(s) in 1 object(s) allocated from:
#0 0x7f781af07cf0 in calloc (/lib64/libasan.so.5+0xefcf0)
#1 0x7f781a2ce22d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5322d)
#2 0x55ee1d075c17 in error_setv /mnt/sdb/backup/qemu/util/error.c:61
#3 0x55ee1d076464 in error_setg_errno_internal
/mnt/sdb/backup/qemu/util/error.c:109
#4 0x55ee1cef066e in qio_channel_socket_writev
/mnt/sdb/backup/qemu/io/channel-socket.c:569
#5 0x55ee1cee806b in qio_channel_writev
/mnt/sdb/backup/qemu/io/channel.c:207
#6 0x55ee1cee806b in qio_channel_writev_all
/mnt/sdb/backup/qemu/io/channel.c:171
#7 0x55ee1cee8248 in qio_channel_write_all
/mnt/sdb/backup/qemu/io/channel.c:257
#8 0x55ee1ca12c9a in multifd_send_thread
/mnt/sdb/backup/qemu/migration/multifd.c:657
#9 0x55ee1d0607fc in qemu_thread_start
/mnt/sdb/backup/qemu/util/qemu-thread-posix.c:519
#10 0x7f78159ae2dd in start_thread (/lib64/libpthread.so.0+0x82dd)
#11 0x7f78156df4b2 in __GI___clone (/lib64/libc.so.6+0xfc4b2)
Indirect leak of 52 byte(s) in 1 object(s) allocated from:
#0 0x7f781af07f28 in __interceptor_realloc (/lib64/libasan.so.5+0xeff28)
#1 0x7f78156f07d9 in __GI___vasprintf_chk (/lib64/libc.so.6+0x10d7d9)
#2 0x7f781a30ea6c in g_vasprintf (/lib64/libglib-2.0.so.0+0x93a6c)
#3 0x7f781a2e7cd0 in g_strdup_vprintf (/lib64/libglib-2.0.so.0+0x6ccd0)
#4 0x7f781a2e7d8c in g_strdup_printf (/lib64/libglib-2.0.so.0+0x6cd8c)
#5 0x55ee1d075c86 in error_setv /mnt/sdb/backup/qemu/util/error.c:65
#6 0x55ee1d076464 in error_setg_errno_internal
/mnt/sdb/backup/qemu/util/error.c:109
#7 0x55ee1cef066e in qio_channel_socket_writev
/mnt/sdb/backup/qemu/io/channel-socket.c:569
#8 0x55ee1cee806b in qio_channel_writev
/mnt/sdb/backup/qemu/io/channel.c:207
#9 0x55ee1cee806b in qio_channel_writev_all
/mnt/sdb/backup/qemu/io/channel.c:171
#10 0x55ee1cee8248 in qio_channel_write_all
/mnt/sdb/backup/qemu/io/channel.c:257
#11 0x55ee1ca12c9a in multifd_send_thread
/mnt/sdb/backup/qemu/migration/multifd.c:657
#12 0x55ee1d0607fc in qemu_thread_start
/mnt/sdb/backup/qemu/util/qemu-thread-posix.c:519
#13 0x7f78159ae2dd in start_thread (/lib64/libpthread.so.0+0x82dd)
#14 0x7f78156df4b2 in __GI___clone (/lib64/libc.so.6+0xfc4b2)
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
---
migration/multifd.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/migration/multifd.c b/migration/multifd.c
index 197d59294a..35ae3180d2 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -550,6 +550,7 @@ void multifd_save_cleanup(void)
multifd_send_state->ops->send_cleanup(p, _err);
if (local_err) {
migrate_set_error(migrate_get_current(), local_err);
+error_free(local_err);
}
}
qemu_sem_destroy(_send_state->channels_ready);
@@ -688,6 +689,7 @@ out:
if (local_err) {
trace_multifd_send_error(p->id);
multifd_send_terminate_threads(local_err);
+error_free(local_err);
}
/*
@@ -965,6 +967,7 @@ static void *multifd_recv_thread(void *opaque)
if (local_err) {
multifd_recv_terminate_threads(local_err);
+error_free(local_err);
}
qemu_mutex_lock(>mutex);
p->running = false;
--
2.18.2
[PATCH 1/2] migration/multifd: fix memleaks in multifd_new_send_channel_async
When error happen in multifd_new_send_channel_async, 'sioc' will not be used
to create the multifd_send_thread. Let's free it to avoid a memleak. And also
do error_free after migrate_set_error() to avoid another leak in the same place.
The leak stack:
Direct leak of 2880 byte(s) in 8 object(s) allocated from:
#0 0x7f20b5118ae8 in __interceptor_malloc (/lib64/libasan.so.5+0xefae8)
#1 0x7f20b44df1d5 in g_malloc (/lib64/libglib-2.0.so.0+0x531d5)
#2 0x564133bce18b in object_new_with_type
/mnt/sdb/backup/qemu/qom/object.c:683
#3 0x564133eea950 in qio_channel_socket_new
/mnt/sdb/backup/qemu/io/channel-socket.c:56
#4 0x5641339cfe4f in socket_send_channel_create
/mnt/sdb/backup/qemu/migration/socket.c:37
#5 0x564133a10328 in multifd_save_setup
/mnt/sdb/backup/qemu/migration/multifd.c:772
#6 0x5641339cebed in migrate_fd_connect
/mnt/sdb/backup/qemu/migration/migration.c:3530
#7 0x5641339d15e4 in migration_channel_connect
/mnt/sdb/backup/qemu/migration/channel.c:92
#8 0x5641339cf5b7 in socket_outgoing_migration
/mnt/sdb/backup/qemu/migration/socket.c:108
Direct leak of 384 byte(s) in 8 object(s) allocated from:
#0 0x7f20b5118cf0 in calloc (/lib64/libasan.so.5+0xefcf0)
#1 0x7f20b44df22d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5322d)
#2 0x56413406fc17 in error_setv /mnt/sdb/backup/qemu/util/error.c:61
#3 0x564134070464 in error_setg_errno_internal
/mnt/sdb/backup/qemu/util/error.c:109
#4 0x5641340851be in inet_connect_addr
/mnt/sdb/backup/qemu/util/qemu-sockets.c:379
#5 0x5641340851be in inet_connect_saddr
/mnt/sdb/backup/qemu/util/qemu-sockets.c:458
#6 0x5641340870ab in socket_connect
/mnt/sdb/backup/qemu/util/qemu-sockets.c:1105
#7 0x564133eeaabf in qio_channel_socket_connect_sync
/mnt/sdb/backup/qemu/io/channel-socket.c:145
#8 0x564133eeabf5 in qio_channel_socket_connect_worker
/mnt/sdb/backup/qemu/io/channel-socket.c:168
Indirect leak of 360 byte(s) in 8 object(s) allocated from:
#0 0x7f20b5118ae8 in __interceptor_malloc (/lib64/libasan.so.5+0xefae8)
#1 0x7f20af901817 in __GI___vasprintf_chk (/lib64/libc.so.6+0x10d817)
#2 0x7f20b451fa6c in g_vasprintf (/lib64/libglib-2.0.so.0+0x93a6c)
#3 0x7f20b44f8cd0 in g_strdup_vprintf (/lib64/libglib-2.0.so.0+0x6ccd0)
#4 0x7f20b44f8d8c in g_strdup_printf (/lib64/libglib-2.0.so.0+0x6cd8c)
#5 0x56413406fc86 in error_setv /mnt/sdb/backup/qemu/util/error.c:65
#6 0x564134070464 in error_setg_errno_internal
/mnt/sdb/backup/qemu/util/error.c:109
#7 0x5641340851be in inet_connect_addr
/mnt/sdb/backup/qemu/util/qemu-sockets.c:379
#8 0x5641340851be in inet_connect_saddr
/mnt/sdb/backup/qemu/util/qemu-sockets.c:458
#9 0x5641340870ab in socket_connect
/mnt/sdb/backup/qemu/util/qemu-sockets.c:1105
#10 0x564133eeaabf in qio_channel_socket_connect_sync
/mnt/sdb/backup/qemu/io/channel-socket.c:145
#11 0x564133eeabf5 in qio_channel_socket_connect_worker
/mnt/sdb/backup/qemu/io/channel-socket.c:168
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
---
migration/multifd.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/migration/multifd.c b/migration/multifd.c
index cb6a4a3ab8..197d59294a 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -727,6 +727,8 @@ static void multifd_new_send_channel_async(QIOTask *task,
gpointer opaque)
* its status.
*/
p->quit = true;
+object_unref(OBJECT(sioc));
+error_free(local_err);
} else {
p->c = QIO_CHANNEL(sioc);
qio_channel_set_delay(p->c, false);
--
2.18.2
Re: [PATCH] op_helper: fix some compile warnings
On 4/20/2020 5:49 PM, Yoshinori Sato wrote:
> On Mon, 20 Apr 2020 18:18:39 +0900,
> Pan Nengyuan wrote:
>>
>>
>>
>> On 4/20/2020 4:50 PM, Yoshinori Sato wrote:
>>> On Mon, 20 Apr 2020 14:49:59 +0900,
>>> Pan Nengyuan wrote:
>>>>
>>>> We got the following compile-time warnings(gcc7.3):
>>>> /mnt/sdb//qemu/target/rx/op_helper.c: In function ‘helper_scmpu’:
>>>> /mnt/sdb/qemu/target/rx/op_helper.c:213:24: error: ‘tmp1’ may be used
>>>> uninitialized in this function [-Werror=maybe-uninitialized]
>>>> env->psw_c = (tmp0 >= tmp1);
>>>> ~~^~~~
>>>> /mnt/sdb/qemu/target/rx/op_helper.c:213:24: error: ‘tmp0’ may be used
>>>> uninitialized in this function [-Werror=maybe-uninitialized]
>>>> /mnt/sdb/qemu/target/rx/op_helper.c: In function ‘helper_suntil’:
>>>> /mnt/sdb/qemu/target/rx/op_helper.c:299:23: error: ‘tmp’ may be used
>>>> uninitialized in this function [-Werror=maybe-uninitialized]
>>>> env->psw_c = (tmp <= env->regs[2]);
>>>> ~^~~~
>>>> /mnt/sdb/qemu/target/rx/op_helper.c: In function ‘helper_swhile’:
>>>> /mnt/sdb/qemu/target/rx/op_helper.c:318:23: error: ‘tmp’ may be used
>>>> uninitialized in this function [-Werror=maybe-uninitialized]
>>>> env->psw_c = (tmp <= env->regs[2]);
>>>>
>>>> Actually, it looks like a false-positive because it will enter the body of
>>>> while loop and init it for the first time.
>>>> Let's change 'while' to 'do .. while' to avoid it.
>>>
>>> OK.
>>>
>>>> Reported-by: Euler Robot
>>>> Signed-off-by: Pan Nengyuan
>>>> ---
>>>> target/rx/op_helper.c | 12 ++--
>>>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/target/rx/op_helper.c b/target/rx/op_helper.c
>>>> index f89d294f2b..b612ab1da8 100644
>>>> --- a/target/rx/op_helper.c
>>>> +++ b/target/rx/op_helper.c
>>>> @@ -201,14 +201,14 @@ void helper_scmpu(CPURXState *env)
>>>> if (env->regs[3] == 0) {
>>>> return;
>>>> }
>>>> -while (env->regs[3] != 0) {
>>>> +do {
>>>> tmp0 = cpu_ldub_data_ra(env, env->regs[1]++, GETPC());
>>>> tmp1 = cpu_ldub_data_ra(env, env->regs[2]++, GETPC());
>>>> env->regs[3]--;
>>>> if (tmp0 != tmp1 || tmp0 == '\0') {
>>>> break;
>>>> }
>>>> -}
>>>> +} while (env->regs[3] != 0);
>>>> env->psw_z = tmp0 - tmp1;
>>>> env->psw_c = (tmp0 >= tmp1);
>>>> }
>>>> @@ -287,14 +287,14 @@ void helper_suntil(CPURXState *env, uint32_t sz)
>>>> if (env->regs[3] == 0) {
>>>> return ;
>>>> }
>>>> -while (env->regs[3] != 0) {
>>>> +do {
>>>> tmp = cpu_ldufn[sz](env, env->regs[1], GETPC());
>>>> env->regs[1] += 1 << sz;
>>>> env->regs[3]--;
>>>> if (tmp == env->regs[2]) {
>>>> break;
>>>> }
>>>> -}
>>>> +} while (env->regs[3] != 0);
>>>> env->psw_z = tmp - env->regs[2];
>>>> env->psw_c = (tmp <= env->regs[2]);
>>>> }
>>>> @@ -306,14 +306,14 @@ void helper_swhile(CPURXState *env, uint32_t sz)
>>>> if (env->regs[3] == 0) {
>>>> return ;
>>>> }
>>>> -while (env->regs[3] != 0) {
>>>> +do {
>>>> tmp = cpu_ldufn[sz](env, env->regs[1], GETPC());
>>>> env->regs[1] += 1 << sz;
>>>> env->regs[3]--;
>>>> if (tmp != env->regs[2]) {
>>>> break;
>>>> }
>>>> -}
>>>> +} while (env->regs[3] != 0);
>>>> env->psw_z = env->regs[3];
>>>> env->psw_c = (tmp <= env->regs[2]);
>>>> }
>>>> --
>>>> 2.18.2
>>>>
>>>>
>>>
>>> It looks different result in env->regs[3] is zero.
>>
>> If env->regs[3] is zero, it will return at the begin of these functions:
>>
Re: [PATCH] op_helper: fix some compile warnings
On 4/20/2020 4:50 PM, Yoshinori Sato wrote:
> On Mon, 20 Apr 2020 14:49:59 +0900,
> Pan Nengyuan wrote:
>>
>> We got the following compile-time warnings(gcc7.3):
>> /mnt/sdb//qemu/target/rx/op_helper.c: In function ‘helper_scmpu’:
>> /mnt/sdb/qemu/target/rx/op_helper.c:213:24: error: ‘tmp1’ may be used
>> uninitialized in this function [-Werror=maybe-uninitialized]
>> env->psw_c = (tmp0 >= tmp1);
>> ~~^~~~
>> /mnt/sdb/qemu/target/rx/op_helper.c:213:24: error: ‘tmp0’ may be used
>> uninitialized in this function [-Werror=maybe-uninitialized]
>> /mnt/sdb/qemu/target/rx/op_helper.c: In function ‘helper_suntil’:
>> /mnt/sdb/qemu/target/rx/op_helper.c:299:23: error: ‘tmp’ may be used
>> uninitialized in this function [-Werror=maybe-uninitialized]
>> env->psw_c = (tmp <= env->regs[2]);
>> ~^~~~
>> /mnt/sdb/qemu/target/rx/op_helper.c: In function ‘helper_swhile’:
>> /mnt/sdb/qemu/target/rx/op_helper.c:318:23: error: ‘tmp’ may be used
>> uninitialized in this function [-Werror=maybe-uninitialized]
>> env->psw_c = (tmp <= env->regs[2]);
>>
>> Actually, it looks like a false-positive because it will enter the body of
>> while loop and init it for the first time.
>> Let's change 'while' to 'do .. while' to avoid it.
>
> OK.
>
>> Reported-by: Euler Robot
>> Signed-off-by: Pan Nengyuan
>> ---
>> target/rx/op_helper.c | 12 ++--
>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/target/rx/op_helper.c b/target/rx/op_helper.c
>> index f89d294f2b..b612ab1da8 100644
>> --- a/target/rx/op_helper.c
>> +++ b/target/rx/op_helper.c
>> @@ -201,14 +201,14 @@ void helper_scmpu(CPURXState *env)
>> if (env->regs[3] == 0) {
>> return;
>> }
>> -while (env->regs[3] != 0) {
>> +do {
>> tmp0 = cpu_ldub_data_ra(env, env->regs[1]++, GETPC());
>> tmp1 = cpu_ldub_data_ra(env, env->regs[2]++, GETPC());
>> env->regs[3]--;
>> if (tmp0 != tmp1 || tmp0 == '\0') {
>> break;
>> }
>> -}
>> +} while (env->regs[3] != 0);
>> env->psw_z = tmp0 - tmp1;
>> env->psw_c = (tmp0 >= tmp1);
>> }
>> @@ -287,14 +287,14 @@ void helper_suntil(CPURXState *env, uint32_t sz)
>> if (env->regs[3] == 0) {
>> return ;
>> }
>> -while (env->regs[3] != 0) {
>> +do {
>> tmp = cpu_ldufn[sz](env, env->regs[1], GETPC());
>> env->regs[1] += 1 << sz;
>> env->regs[3]--;
>> if (tmp == env->regs[2]) {
>> break;
>> }
>> -}
>> +} while (env->regs[3] != 0);
>> env->psw_z = tmp - env->regs[2];
>> env->psw_c = (tmp <= env->regs[2]);
>> }
>> @@ -306,14 +306,14 @@ void helper_swhile(CPURXState *env, uint32_t sz)
>> if (env->regs[3] == 0) {
>> return ;
>> }
>> -while (env->regs[3] != 0) {
>> +do {
>> tmp = cpu_ldufn[sz](env, env->regs[1], GETPC());
>> env->regs[1] += 1 << sz;
>> env->regs[3]--;
>> if (tmp != env->regs[2]) {
>> break;
>> }
>> -}
>> +} while (env->regs[3] != 0);
>> env->psw_z = env->regs[3];
>> env->psw_c = (tmp <= env->regs[2]);
>> }
>> --
>> 2.18.2
>>
>>
>
> It looks different result in env->regs[3] is zero.
If env->regs[3] is zero, it will return at the begin of these functions:
if (env->regs[3] == 0) {
return;
}
Thus, the while loop will not be reached.
In this case, I think 'while' and 'do .. while' will get the same result and it
will disappear the warnings.
> In such a case, nothing changes.
>
> I think that the warning of the uninitialized variable
> will disappear by fixing as follows.
>
Yes, it also can fix these warnings.
Thanks.
>>From 5de0c54a970e01e96b41870252d0ea54ec61c540 Mon Sep 17 00:00:00 2001
> From: Yoshinori Sato
> Date: Mon, 20 Apr 2020 17:41:04 +0900
> Subject: [PATCH] target/rx/op_helper: Fix uninitialized warning.
>
> Signed-off-by: Yoshinori Sato
> ---
> target/rx/op_helper.c | 101 --
> 1 file changed, 49 insertions(+), 52 deletions(-)
>
> diff --git a/target/rx/op_helper.c b/target/rx/op_helper.c
> index f89d294f2b..f84f6c706c 100644
&
Re: [PATCH v2] scsi-disk: define props in scsi_block_disk to avoid memleaks
Hi Paolo,
On 2/19/2020 3:52 PM, Pan Nengyuan wrote:
>
>
> On 1/22/2020 1:05 AM, Paolo Bonzini wrote:
>> On 14/01/20 10:16, pannengy...@huawei.com wrote:
>>> From: Pan Nengyuan
>>>
>>> scsi_block_realize() use scsi_realize() to init some props, but
>>> these props is not defined in scsi_block_disk_properties, so they will
>>> not be freed.
>>>
>>> This patch defines these prop in scsi_block_disk_properties and aslo
>>> calls scsi_unrealize to avoid memleaks, the leak stack as
>>> follow(it's easy to reproduce by attaching/detaching scsi-block-disks):
>>>
>>> =
>>> ==qemu-system-x86_64==32195==ERROR: LeakSanitizer: detected memory leaks
>>>
>>> Direct leak of 57 byte(s) in 3 object(s) allocated from:
>>> #0 0x7f19f8bed768 (/lib64/libasan.so.5+0xef768) ??:?
>>> #1 0x7f19f64d9445 (/lib64/libglib-2.0.so.0+0x52445) ??:?
>>> #2 0x7f19f64f2d92 (/lib64/libglib-2.0.so.0+0x6bd92) ??:?
>>> #3 0x55975366e596 (qemu-system-x86_64+0x35c0596)
>>> /mnt/sdb/qemu/hw/scsi/scsi-disk.c:2399
>>> #4 0x559753671201 (emu-system-x86_64+0x35c3201)
>>> /mnt/sdb/qemu/hw/scsi/scsi-disk.c:2681
>>> #5 0x559753687e3e (qemu-system-x86_64+0x35d9e3e)
>>> /mnt/sdb/qemu/hw/scsi/scsi-bus.c:58
>>> #6 0x55975368ac44 (qemu-system-x86_64+0x35dcc44)
>>> /mnt/sdb/qemu/hw/scsi/scsi-bus.c:216
>>> #7 0x5597532a7840 (qemu-system-x86_64+0x31f9840)
>>> /mnt/sdb/qemu/hw/core/qdev.c:876
>>>
>>> Direct leak of 15 byte(s) in 3 object(s) allocated from:
>>> #0 0x7f19f8bed768 (/lib64/libasan.so.5+0xef768) ??:?
>>> #1 0x7f19f64d9445 (/lib64/libglib-2.0.so.0+0x52445) ??:?
>>> #2 0x7f19f64f2d92 (/lib64/libglib-2.0.so.0+0x6bd92) ??:?
>>> #3 0x55975366e06f (qemu-system-x86_64+0x35c006f)
>>> /mnt/sdb/qemu/hw/scsi/scsi-disk.c:2388
>>> #4 0x559753671201 (qemu-system-x86_64+0x35c3201)
>>> /mnt/sdb/qemu/hw/scsi/scsi-disk.c:2681
>>> #5 0x559753687e3e (qemu-system-x86_64+0x35d9e3e)
>>> /mnt/sdb/qemu/hw/scsi/scsi-bus.c:58
>>> #6 0x55975368ac44 (qemu-system-x86_64+0x35dcc44)
>>> /mnt/sdb/qemu/hw/scsi/scsi-bus.c:216
>>>
>>> @@ -3079,9 +3080,8 @@ static const TypeInfo scsi_cd_info = {
>>>
>>> #ifdef __linux__
>>> static Property scsi_block_properties[] = {
>>> -DEFINE_BLOCK_ERROR_PROPERTIES(SCSIDiskState, qdev.conf), \
>>> +DEFINE_SCSI_DISK_PROPERTIES(),.
>> The properties defined there shouldn't apply to scsi-block.
>>
>> Can you explain what exactly is being leaked?
>
> Ohh, I'm sorry, I missed this email and reply it so late.
>
> When we attach a scsi-block disk, the props(version/vender/device_id) are
> malloced in scsi_realize() which it's called by scsi_block_realize(),
> but we don't define these props in scsi_block_properties. So these props will
> not be released when we detach the scsi-block disk.
>
> This patch will reuse scsi_disk_properties to define those props in
> scsi_block_properties to fix it.
> Similarly to scsi_hd, this patch aslo set unrealize to call
> del_boot_device_lchs().
>
> Thanks.
>
Maybe you missed this patch due to my late reply. Actually it will cause a
memleak when we attach/detach a scsi-block disk.
Reproducer:
(qemu) device_add virtio-scsi-pci,id=scsi1,bus=pci.0,addr=0x7
(qemu) drive_add 1
file=/dev/sdc,format=raw,if=none,id=drive-scsi1-0-0-12,cache=none,aio=native
OK
(qemu) device_add
scsi-block,bus=scsi1.0,channel=0,scsi-id=0,lun=10,share-rw=on,drive=drive-scsi1-0-0-12,id=scsi1-0-0-10
(qemu) device_del scsi1-0-0-10
(qemu) drive_del drive-scsi1-0-0-12
I'm not sure why the properties defined shouldn't be applied, can you give some
more suggestions?
Thanks.
>>
>> Paolo
>>
>> .
>>
Re: [PATCH] migration/rdma: fix a memleak on error path in rdma_start_incoming_migration
Correcting zhang hailiang's email. On 4/20/2020 6:27 PM, Pan Nengyuan wrote: > 'rdma->host' is malloced in qemu_rdma_data_init, but forgot to free on the > error > path in rdma_start_incoming_migration(), this patch fix that. > > The leak stack: > Direct leak of 2 byte(s) in 1 object(s) allocated from: > #0 0x7fb7add18ae8 in __interceptor_malloc (/lib64/libasan.so.5+0xefae8) > #1 0x7fb7ad0df1d5 in g_malloc (/lib64/libglib-2.0.so.0+0x531d5) > #2 0x7fb7ad0f8b32 in g_strdup (/lib64/libglib-2.0.so.0+0x6cb32) > #3 0x55a0464a0f6f in qemu_rdma_data_init > /mnt/sdb/qemu/migration/rdma.c:2647 > #4 0x55a0464b0e76 in rdma_start_incoming_migration > /mnt/sdb/qemu/migration/rdma.c:4020 > #5 0x55a0463f898a in qemu_start_incoming_migration > /mnt/sdb/qemu/migration/migration.c:365 > #6 0x55a0458c75d3 in qemu_init /mnt/sdb/qemu/softmmu/vl.c:4438 > #7 0x55a046a3d811 in main /mnt/sdb/qemu/softmmu/main.c:48 > #8 0x7fb7a8417872 in __libc_start_main (/lib64/libc.so.6+0x23872) > #9 0x55a04536b26d in _start > (/mnt/sdb/qemu/build/x86_64-softmmu/qemu-system-x86_64+0x286926d) > > Reported-by: Euler Robot > Signed-off-by: Pan Nengyuan > --- > migration/rdma.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/migration/rdma.c b/migration/rdma.c > index f61587891b..967fda5b0c 100644 > --- a/migration/rdma.c > +++ b/migration/rdma.c > @@ -4056,6 +4056,7 @@ void rdma_start_incoming_migration(const char > *host_port, Error **errp) > return; > err: > error_propagate(errp, local_err); > +g_free(rdma->host); > g_free(rdma); > g_free(rdma_return_path); > } >
[PATCH] migration/rdma: fix a memleak on error path in rdma_start_incoming_migration
'rdma->host' is malloced in qemu_rdma_data_init, but forgot to free on the error path in rdma_start_incoming_migration(), this patch fix that. The leak stack: Direct leak of 2 byte(s) in 1 object(s) allocated from: #0 0x7fb7add18ae8 in __interceptor_malloc (/lib64/libasan.so.5+0xefae8) #1 0x7fb7ad0df1d5 in g_malloc (/lib64/libglib-2.0.so.0+0x531d5) #2 0x7fb7ad0f8b32 in g_strdup (/lib64/libglib-2.0.so.0+0x6cb32) #3 0x55a0464a0f6f in qemu_rdma_data_init /mnt/sdb/qemu/migration/rdma.c:2647 #4 0x55a0464b0e76 in rdma_start_incoming_migration /mnt/sdb/qemu/migration/rdma.c:4020 #5 0x55a0463f898a in qemu_start_incoming_migration /mnt/sdb/qemu/migration/migration.c:365 #6 0x55a0458c75d3 in qemu_init /mnt/sdb/qemu/softmmu/vl.c:4438 #7 0x55a046a3d811 in main /mnt/sdb/qemu/softmmu/main.c:48 #8 0x7fb7a8417872 in __libc_start_main (/lib64/libc.so.6+0x23872) #9 0x55a04536b26d in _start (/mnt/sdb/qemu/build/x86_64-softmmu/qemu-system-x86_64+0x286926d) Reported-by: Euler Robot Signed-off-by: Pan Nengyuan --- migration/rdma.c | 1 + 1 file changed, 1 insertion(+) diff --git a/migration/rdma.c b/migration/rdma.c index f61587891b..967fda5b0c 100644 --- a/migration/rdma.c +++ b/migration/rdma.c @@ -4056,6 +4056,7 @@ void rdma_start_incoming_migration(const char *host_port, Error **errp) return; err: error_propagate(errp, local_err); +g_free(rdma->host); g_free(rdma); g_free(rdma_return_path); } -- 2.18.2
[PATCH] op_helper: fix some compile warnings
We got the following compile-time warnings(gcc7.3):
/mnt/sdb//qemu/target/rx/op_helper.c: In function ‘helper_scmpu’:
/mnt/sdb/qemu/target/rx/op_helper.c:213:24: error: ‘tmp1’ may be used
uninitialized in this function [-Werror=maybe-uninitialized]
env->psw_c = (tmp0 >= tmp1);
~~^~~~
/mnt/sdb/qemu/target/rx/op_helper.c:213:24: error: ‘tmp0’ may be used
uninitialized in this function [-Werror=maybe-uninitialized]
/mnt/sdb/qemu/target/rx/op_helper.c: In function ‘helper_suntil’:
/mnt/sdb/qemu/target/rx/op_helper.c:299:23: error: ‘tmp’ may be used
uninitialized in this function [-Werror=maybe-uninitialized]
env->psw_c = (tmp <= env->regs[2]);
~^~~~
/mnt/sdb/qemu/target/rx/op_helper.c: In function ‘helper_swhile’:
/mnt/sdb/qemu/target/rx/op_helper.c:318:23: error: ‘tmp’ may be used
uninitialized in this function [-Werror=maybe-uninitialized]
env->psw_c = (tmp <= env->regs[2]);
Actually, it looks like a false-positive because it will enter the body of
while loop and init it for the first time.
Let's change 'while' to 'do .. while' to avoid it.
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
---
target/rx/op_helper.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/target/rx/op_helper.c b/target/rx/op_helper.c
index f89d294f2b..b612ab1da8 100644
--- a/target/rx/op_helper.c
+++ b/target/rx/op_helper.c
@@ -201,14 +201,14 @@ void helper_scmpu(CPURXState *env)
if (env->regs[3] == 0) {
return;
}
-while (env->regs[3] != 0) {
+do {
tmp0 = cpu_ldub_data_ra(env, env->regs[1]++, GETPC());
tmp1 = cpu_ldub_data_ra(env, env->regs[2]++, GETPC());
env->regs[3]--;
if (tmp0 != tmp1 || tmp0 == '\0') {
break;
}
-}
+} while (env->regs[3] != 0);
env->psw_z = tmp0 - tmp1;
env->psw_c = (tmp0 >= tmp1);
}
@@ -287,14 +287,14 @@ void helper_suntil(CPURXState *env, uint32_t sz)
if (env->regs[3] == 0) {
return ;
}
-while (env->regs[3] != 0) {
+do {
tmp = cpu_ldufn[sz](env, env->regs[1], GETPC());
env->regs[1] += 1 << sz;
env->regs[3]--;
if (tmp == env->regs[2]) {
break;
}
-}
+} while (env->regs[3] != 0);
env->psw_z = tmp - env->regs[2];
env->psw_c = (tmp <= env->regs[2]);
}
@@ -306,14 +306,14 @@ void helper_swhile(CPURXState *env, uint32_t sz)
if (env->regs[3] == 0) {
return ;
}
-while (env->regs[3] != 0) {
+do {
tmp = cpu_ldufn[sz](env, env->regs[1], GETPC());
env->regs[1] += 1 << sz;
env->regs[3]--;
if (tmp != env->regs[2]) {
break;
}
-}
+} while (env->regs[3] != 0);
env->psw_z = env->regs[3];
env->psw_c = (tmp <= env->regs[2]);
}
--
2.18.2
[PATCH v4 1/2] virtio-blk: delete vqs on the error path in realize()
virtio_vqs forgot to free on the error path in realize(). Fix that.
The asan stack:
Direct leak of 14336 byte(s) in 1 object(s) allocated from:
#0 0x7f58b93fd970 in __interceptor_calloc (/lib64/libasan.so.5+0xef970)
#1 0x7f58b858249d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5249d)
#2 0x5562cc627f49 in virtio_add_queue /mnt/sdb/qemu/hw/virtio/virtio.c:2413
#3 0x5562cc4b524a in virtio_blk_device_realize
/mnt/sdb/qemu/hw/block/virtio-blk.c:1202
#4 0x5562cc613050 in virtio_device_realize
/mnt/sdb/qemu/hw/virtio/virtio.c:3615
#5 0x5562ccb7a568 in device_set_realized /mnt/sdb/qemu/hw/core/qdev.c:891
#6 0x5562cd39cd45 in property_set_bool /mnt/sdb/qemu/qom/object.c:2238
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
Reviewed-by: Stefano Garzarella
---
v2->v1:
- Fix incorrect free in v1, it will cause a uaf.
---
Cc: Stefan Hajnoczi
Cc: Kevin Wolf
Cc: Max Reitz
Cc: qemu-bl...@nongnu.org
---
hw/block/virtio-blk.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 142863a3b2..97ba8a2187 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1204,6 +1204,9 @@ static void virtio_blk_device_realize(DeviceState *dev,
Error **errp)
virtio_blk_data_plane_create(vdev, conf, >dataplane, );
if (err != NULL) {
error_propagate(errp, err);
+for (i = 0; i < conf->num_queues; i++) {
+virtio_del_queue(vdev, i);
+}
virtio_cleanup(vdev);
return;
}
--
2.18.2
[PATCH v4 0/2] fix two virtio queues memleak
This series fix two vqs leak: 1. Do delete vqs on the error path in virtio_blk_device_realize(). 2. Do delete vqs in virtio_iommu_device_unrealize() to fix another leaks. v2->v1: - Fix incorrect free in virtio_blk_device_realize, it will cause a uaf. v3->v2: - Also clean 's->as_by_busptr' hash table in virtio_iommu_device_unrealize.(Suggested by Stefano Garzarella) v4->v3: - update patch2/2 subject message and move g_hash_table_destroy() at the beggining of unrealize(). Pan Nengyuan (2): virtio-blk: delete vqs on the error path in realize() virtio-iommu: avoid memleak in the unrealize hw/block/virtio-blk.c| 3 +++ hw/virtio/virtio-iommu.c | 3 +++ 2 files changed, 6 insertions(+) -- 2.18.2
[PATCH v4 2/2] virtio-iommu: avoid memleak in the unrealize
req_vq/event_vq forgot to free in unrealize. Fix that. And also do clean 's->as_by_busptr' hash table in unrealize to fix another leak. Signed-off-by: Pan Nengyuan Acked-by: Eric Auger --- Cc: Eric Auger Cc: Stefan Hajnoczi --- v3->v1/v2: - Also clean 's->as_by_busptr' hash table in unrealize.(Suggested by Stefano Garzarella) v4->v3: - update subject msg and move g_hash_table_destroy to the beginning of unrealize. --- hw/virtio/virtio-iommu.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c index 4cee8083bc..22ba8848c2 100644 --- a/hw/virtio/virtio-iommu.c +++ b/hw/virtio/virtio-iommu.c @@ -693,9 +693,12 @@ static void virtio_iommu_device_unrealize(DeviceState *dev, Error **errp) VirtIODevice *vdev = VIRTIO_DEVICE(dev); VirtIOIOMMU *s = VIRTIO_IOMMU(dev); +g_hash_table_destroy(s->as_by_busptr); g_tree_destroy(s->domains); g_tree_destroy(s->endpoints); +virtio_delete_queue(s->req_vq); +virtio_delete_queue(s->event_vq); virtio_cleanup(vdev); } -- 2.18.2
Re: [PATCH v3 2/2] virtio-iommu: delete vqs in unrealize to fix memleak
On 3/28/2020 12:26 AM, Stefano Garzarella wrote: > On Fri, Mar 27, 2020 at 05:56:42PM +0800, Pan Nengyuan wrote: >> req_vq/event_vq forgot to free in unrealize. Fix that. >> And aslo do clean 's->as_by_busptr' hash table in unrealize to fix another >> leak. > > s/aslo/also > > Maybe we can also update the subject in something like this: > "virtio-iommu: avoid memleak in the unrealize" OK. > >> >> Signed-off-by: Pan Nengyuan >> Acked-by: Eric Auger >> --- >> Cc: Eric Auger >> --- >> v3->v1/v2: >> - Aslo clean 's->as_by_busptr' hash table in unrealize.(Suggested by Stefano >> Garzarella) >> --- >> hw/virtio/virtio-iommu.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c >> index 4cee8083bc..694698fa0f 100644 >> --- a/hw/virtio/virtio-iommu.c >> +++ b/hw/virtio/virtio-iommu.c >> @@ -696,7 +696,10 @@ static void virtio_iommu_device_unrealize(DeviceState >> *dev, Error **errp) >> g_tree_destroy(s->domains); >> g_tree_destroy(s->endpoints); >> >> +virtio_delete_queue(s->req_vq); >> +virtio_delete_queue(s->event_vq); >> virtio_cleanup(vdev); >> +g_hash_table_destroy(s->as_by_busptr); > > If you need to respin, you could move g_hash_table_destroy() > at the beggining of unrealize(), just to follow a reverse order of > realize(). OK. Thanks. > > Thanks, > Stefano > >> } >> >> static void virtio_iommu_device_reset(VirtIODevice *vdev) >> -- >> 2.18.2 >> >> >
[PATCH v3 0/2] fix two virtio queues memleak
This series fix two vqs leak: 1. Do delete vqs on the error path in virtio_blk_device_realize(). 2. Do delete vqs in virtio_iommu_device_unrealize() to fix another leaks. v2->v1: - Fix incorrect free in virtio_blk_device_realize, it will cause a uaf. v3->v2: - Aslo clean 's->as_by_busptr' hash table in virtio_iommu_device_unrealize.(Suggested by Stefano Garzarella) Pan Nengyuan (2): virtio-blk: delete vqs on the error path in realize() virtio-iommu: delete vqs in unrealize to fix memleak hw/block/virtio-blk.c| 3 +++ hw/virtio/virtio-iommu.c | 3 +++ 2 files changed, 6 insertions(+) -- 2.18.2
[PATCH v3 1/2] virtio-blk: delete vqs on the error path in realize()
virtio_vqs forgot to free on the error path in realize(). Fix that.
The asan stack:
Direct leak of 14336 byte(s) in 1 object(s) allocated from:
#0 0x7f58b93fd970 in __interceptor_calloc (/lib64/libasan.so.5+0xef970)
#1 0x7f58b858249d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5249d)
#2 0x5562cc627f49 in virtio_add_queue /mnt/sdb/qemu/hw/virtio/virtio.c:2413
#3 0x5562cc4b524a in virtio_blk_device_realize
/mnt/sdb/qemu/hw/block/virtio-blk.c:1202
#4 0x5562cc613050 in virtio_device_realize
/mnt/sdb/qemu/hw/virtio/virtio.c:3615
#5 0x5562ccb7a568 in device_set_realized /mnt/sdb/qemu/hw/core/qdev.c:891
#6 0x5562cd39cd45 in property_set_bool /mnt/sdb/qemu/qom/object.c:2238
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
Reviewed-by: Stefano Garzarella
---
v2->v1:
- Fix incorrect free in v1, it will cause a uaf.
---
Cc: Stefan Hajnoczi
Cc: Kevin Wolf
Cc: Max Reitz
Cc: qemu-bl...@nongnu.org
---
hw/block/virtio-blk.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 142863a3b2..97ba8a2187 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1204,6 +1204,9 @@ static void virtio_blk_device_realize(DeviceState *dev,
Error **errp)
virtio_blk_data_plane_create(vdev, conf, >dataplane, );
if (err != NULL) {
error_propagate(errp, err);
+for (i = 0; i < conf->num_queues; i++) {
+virtio_del_queue(vdev, i);
+}
virtio_cleanup(vdev);
return;
}
--
2.18.2
[PATCH v3 2/2] virtio-iommu: delete vqs in unrealize to fix memleak
req_vq/event_vq forgot to free in unrealize. Fix that. And aslo do clean 's->as_by_busptr' hash table in unrealize to fix another leak. Signed-off-by: Pan Nengyuan Acked-by: Eric Auger --- Cc: Eric Auger --- v3->v1/v2: - Aslo clean 's->as_by_busptr' hash table in unrealize.(Suggested by Stefano Garzarella) --- hw/virtio/virtio-iommu.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c index 4cee8083bc..694698fa0f 100644 --- a/hw/virtio/virtio-iommu.c +++ b/hw/virtio/virtio-iommu.c @@ -696,7 +696,10 @@ static void virtio_iommu_device_unrealize(DeviceState *dev, Error **errp) g_tree_destroy(s->domains); g_tree_destroy(s->endpoints); +virtio_delete_queue(s->req_vq); +virtio_delete_queue(s->event_vq); virtio_cleanup(vdev); +g_hash_table_destroy(s->as_by_busptr); } static void virtio_iommu_device_reset(VirtIODevice *vdev) -- 2.18.2
Re: [PATCH 2/2] virtio-iommu: delete vqs in unrealize to fix memleaks
On 3/27/2020 4:53 PM, Stefano Garzarella wrote: > On Fri, Mar 27, 2020 at 11:56:50AM +0800, Pan Nengyuan wrote: >> req_vq/event_vq forgot to free in unrealize(). Fix that. >> >> Signed-off-by: Pan Nengyuan >> --- >> Cc: Eric Auger >> --- >> hw/virtio/virtio-iommu.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c >> index 4cee8083bc..9d2ff0693c 100644 >> --- a/hw/virtio/virtio-iommu.c >> +++ b/hw/virtio/virtio-iommu.c >> @@ -696,6 +696,8 @@ static void virtio_iommu_device_unrealize(DeviceState >> *dev, Error **errp) >> g_tree_destroy(s->domains); >> g_tree_destroy(s->endpoints); >> >> +virtio_delete_queue(s->req_vq); >> +virtio_delete_queue(s->event_vq); >> virtio_cleanup(vdev); >> } > > Hi Pan, > thanks for this fix. > > Since we are here, should we also clean 's->as_by_busptr' hash table? > > Maybe adding this in the unrealize: > > g_hash_table_destroy(s->as_by_busptr); Yes, you are right. I will add it. Thanks. > > Thanks, > Stefano >
Re: [PATCH 1/2] virtio-blk: delete vqs on the error path in realize()
On 3/27/2020 4:41 PM, Stefano Garzarella wrote:
> On Fri, Mar 27, 2020 at 11:56:49AM +0800, Pan Nengyuan wrote:
>> virtio_vqs forgot to free on the error path in realize(). Fix that.
>>
>> The asan stack:
>> Direct leak of 14336 byte(s) in 1 object(s) allocated from:
>> #0 0x7f58b93fd970 in __interceptor_calloc (/lib64/libasan.so.5+0xef970)
>> #1 0x7f58b858249d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5249d)
>> #2 0x5562cc627f49 in virtio_add_queue
>> /mnt/sdb/qemu/hw/virtio/virtio.c:2413
>> #3 0x5562cc4b524a in virtio_blk_device_realize
>> /mnt/sdb/qemu/hw/block/virtio-blk.c:1202
>> #4 0x5562cc613050 in virtio_device_realize
>> /mnt/sdb/qemu/hw/virtio/virtio.c:3615
>> #5 0x5562ccb7a568 in device_set_realized /mnt/sdb/qemu/hw/core/qdev.c:891
>> #6 0x5562cd39cd45 in property_set_bool /mnt/sdb/qemu/qom/object.c:2238
>>
>> Reported-by: Euler Robot
>> Signed-off-by: Pan Nengyuan
>> ---
>> Cc: Stefan Hajnoczi
>> Cc: Kevin Wolf
>> Cc: Max Reitz
>> Cc: qemu-bl...@nongnu.org
>> ---
>> hw/block/virtio-blk.c | 8 ++--
>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
>> index 142863a3b2..a6682c2ced 100644
>> --- a/hw/block/virtio-blk.c
>> +++ b/hw/block/virtio-blk.c
>> @@ -1204,8 +1204,7 @@ static void virtio_blk_device_realize(DeviceState
>> *dev, Error **errp)
>> virtio_blk_data_plane_create(vdev, conf, >dataplane, );
>> if (err != NULL) {
>> error_propagate(errp, err);
>> -virtio_cleanup(vdev);
>> -return;
>> +goto fail;
>> }
>>
>> s->change = qemu_add_vm_change_state_handler(virtio_blk_dma_restart_cb,
>> s);
>> @@ -1218,6 +1217,11 @@ static void virtio_blk_device_realize(DeviceState
>> *dev, Error **errp)
>> conf->conf.lcyls,
>> conf->conf.lheads,
>> conf->conf.lsecs);
>
> I think we should add a return here, otherwise we will remove the virt
> queues also in the success case.
Yes, I have sent a new version v2 and changed it, you can review on it.
Thanks.
>
> Thanks,
> Stefano
>
>> +fail:
>> +for (i = 0; i < conf->num_queues; i++) {
>> +virtio_del_queue(vdev, i);
>> +}
>> +virtio_cleanup(vdev);
>> }
>>
>> static void virtio_blk_device_unrealize(DeviceState *dev, Error **errp)
>> --
>> 2.18.2
>>
>>
>
> .
>
[PATCH v2 1/2] virtio-blk: delete vqs on the error path in realize()
virtio_vqs forgot to free on the error path in realize(). Fix that.
The asan stack:
Direct leak of 14336 byte(s) in 1 object(s) allocated from:
#0 0x7f58b93fd970 in __interceptor_calloc (/lib64/libasan.so.5+0xef970)
#1 0x7f58b858249d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5249d)
#2 0x5562cc627f49 in virtio_add_queue /mnt/sdb/qemu/hw/virtio/virtio.c:2413
#3 0x5562cc4b524a in virtio_blk_device_realize
/mnt/sdb/qemu/hw/block/virtio-blk.c:1202
#4 0x5562cc613050 in virtio_device_realize
/mnt/sdb/qemu/hw/virtio/virtio.c:3615
#5 0x5562ccb7a568 in device_set_realized /mnt/sdb/qemu/hw/core/qdev.c:891
#6 0x5562cd39cd45 in property_set_bool /mnt/sdb/qemu/qom/object.c:2238
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
---
v2->v1:
- Fix incorrect free in v1, it will cause a uaf.
---
Cc: Stefan Hajnoczi
Cc: Kevin Wolf
Cc: Max Reitz
Cc: qemu-bl...@nongnu.org
---
hw/block/virtio-blk.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 142863a3b2..97ba8a2187 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1204,6 +1204,9 @@ static void virtio_blk_device_realize(DeviceState *dev,
Error **errp)
virtio_blk_data_plane_create(vdev, conf, >dataplane, );
if (err != NULL) {
error_propagate(errp, err);
+for (i = 0; i < conf->num_queues; i++) {
+virtio_del_queue(vdev, i);
+}
virtio_cleanup(vdev);
return;
}
--
2.18.2
[PATCH v2 0/2] fix two virtio queues memleak
This series fix two vqs leak: 1. Do delete vqs on the error path in virtio_blk_device_realize(). 2. Do delete vqs in virtio_iommu_device_unrealize() to fix another leaks. v2->v1: - Fix incorrect free in v1, it will cause a uaf. Pan Nengyuan (2): virtio-blk: delete vqs on the error path in realize() virtio-iommu: delete vqs in unrealize to fix memleak hw/block/virtio-blk.c| 3 +++ hw/virtio/virtio-iommu.c | 2 ++ 2 files changed, 5 insertions(+) -- 2.18.2
[PATCH v2 2/2] virtio-iommu: delete vqs in unrealize to fix memleak
req_vq/event_vq forgot to free in unrealize. Fix that. Signed-off-by: Pan Nengyuan --- Cc: Eric Auger --- hw/virtio/virtio-iommu.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c index 4cee8083bc..9d2ff0693c 100644 --- a/hw/virtio/virtio-iommu.c +++ b/hw/virtio/virtio-iommu.c @@ -696,6 +696,8 @@ static void virtio_iommu_device_unrealize(DeviceState *dev, Error **errp) g_tree_destroy(s->domains); g_tree_destroy(s->endpoints); +virtio_delete_queue(s->req_vq); +virtio_delete_queue(s->event_vq); virtio_cleanup(vdev); } -- 2.18.2
[PATCH 1/2] virtio-blk: delete vqs on the error path in realize()
virtio_vqs forgot to free on the error path in realize(). Fix that.
The asan stack:
Direct leak of 14336 byte(s) in 1 object(s) allocated from:
#0 0x7f58b93fd970 in __interceptor_calloc (/lib64/libasan.so.5+0xef970)
#1 0x7f58b858249d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5249d)
#2 0x5562cc627f49 in virtio_add_queue /mnt/sdb/qemu/hw/virtio/virtio.c:2413
#3 0x5562cc4b524a in virtio_blk_device_realize
/mnt/sdb/qemu/hw/block/virtio-blk.c:1202
#4 0x5562cc613050 in virtio_device_realize
/mnt/sdb/qemu/hw/virtio/virtio.c:3615
#5 0x5562ccb7a568 in device_set_realized /mnt/sdb/qemu/hw/core/qdev.c:891
#6 0x5562cd39cd45 in property_set_bool /mnt/sdb/qemu/qom/object.c:2238
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
---
Cc: Stefan Hajnoczi
Cc: Kevin Wolf
Cc: Max Reitz
Cc: qemu-bl...@nongnu.org
---
hw/block/virtio-blk.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 142863a3b2..a6682c2ced 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1204,8 +1204,7 @@ static void virtio_blk_device_realize(DeviceState *dev,
Error **errp)
virtio_blk_data_plane_create(vdev, conf, >dataplane, );
if (err != NULL) {
error_propagate(errp, err);
-virtio_cleanup(vdev);
-return;
+goto fail;
}
s->change = qemu_add_vm_change_state_handler(virtio_blk_dma_restart_cb, s);
@@ -1218,6 +1217,11 @@ static void virtio_blk_device_realize(DeviceState *dev,
Error **errp)
conf->conf.lcyls,
conf->conf.lheads,
conf->conf.lsecs);
+fail:
+for (i = 0; i < conf->num_queues; i++) {
+virtio_del_queue(vdev, i);
+}
+virtio_cleanup(vdev);
}
static void virtio_blk_device_unrealize(DeviceState *dev, Error **errp)
--
2.18.2
[PATCH 2/2] virtio-iommu: delete vqs in unrealize to fix memleaks
req_vq/event_vq forgot to free in unrealize(). Fix that. Signed-off-by: Pan Nengyuan --- Cc: Eric Auger --- hw/virtio/virtio-iommu.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c index 4cee8083bc..9d2ff0693c 100644 --- a/hw/virtio/virtio-iommu.c +++ b/hw/virtio/virtio-iommu.c @@ -696,6 +696,8 @@ static void virtio_iommu_device_unrealize(DeviceState *dev, Error **errp) g_tree_destroy(s->domains); g_tree_destroy(s->endpoints); +virtio_delete_queue(s->req_vq); +virtio_delete_queue(s->event_vq); virtio_cleanup(vdev); } -- 2.18.2
[PATCH 0/2] fix two virtio queues memleak
This series fix two vqs leak: 1. Do delete vqs on the error path in virtio_blk_device_realize(). 2. Do delete vqs in virtio_iommu_device_unrealize() to fix another leaks. Pan Nengyuan (2): virtio-blk: delete vqs on the error path in realize() virtio-iommu: delete vqs in unrealize to fix memleaks hw/block/virtio-blk.c| 8 ++-- hw/virtio/virtio-iommu.c | 2 ++ 2 files changed, 8 insertions(+), 2 deletions(-) -- 2.18.2
Re: [PATCH] hmp-cmd: fix a missing_break warning
Correcting zhang hailiang's email. On 3/18/2020 3:16 PM, Pan Nengyuan wrote: > This fix coverity issues 94417686: > 1260break; > CID 94417686: (MISSING_BREAK) > 1261. unterminated_case: The case for value > "MIGRATION_PARAMETER_THROTTLE_TRIGGER_THRESHOLD" is not terminated by a > 'break' statement. > 1261case MIGRATION_PARAMETER_THROTTLE_TRIGGER_THRESHOLD: > 1262p->has_throttle_trigger_threshold = true; > 1263visit_type_int(v, param, >throttle_trigger_threshold, > ); > 1264case MIGRATION_PARAMETER_CPU_THROTTLE_INITIAL: > > Fixes: dc14a470763c96fd9d360e1028ce38e8c3613a77 > Reported-by: Euler Robot > Signed-off-by: Pan Nengyuan > --- > Cc: zhukeqi...@huawei.com > --- > monitor/hmp-cmds.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c > index 58724031ea..c882c9f3cc 100644 > --- a/monitor/hmp-cmds.c > +++ b/monitor/hmp-cmds.c > @@ -1261,6 +1261,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const > QDict *qdict) > case MIGRATION_PARAMETER_THROTTLE_TRIGGER_THRESHOLD: > p->has_throttle_trigger_threshold = true; > visit_type_int(v, param, >throttle_trigger_threshold, ); > +break; > case MIGRATION_PARAMETER_CPU_THROTTLE_INITIAL: > p->has_cpu_throttle_initial = true; > visit_type_int(v, param, >cpu_throttle_initial, ); >
[PATCH] hmp-cmd: fix a missing_break warning
This fix coverity issues 94417686: 1260break; CID 94417686: (MISSING_BREAK) 1261. unterminated_case: The case for value "MIGRATION_PARAMETER_THROTTLE_TRIGGER_THRESHOLD" is not terminated by a 'break' statement. 1261case MIGRATION_PARAMETER_THROTTLE_TRIGGER_THRESHOLD: 1262p->has_throttle_trigger_threshold = true; 1263visit_type_int(v, param, >throttle_trigger_threshold, ); 1264case MIGRATION_PARAMETER_CPU_THROTTLE_INITIAL: Fixes: dc14a470763c96fd9d360e1028ce38e8c3613a77 Reported-by: Euler Robot Signed-off-by: Pan Nengyuan --- Cc: zhukeqi...@huawei.com --- monitor/hmp-cmds.c | 1 + 1 file changed, 1 insertion(+) diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c index 58724031ea..c882c9f3cc 100644 --- a/monitor/hmp-cmds.c +++ b/monitor/hmp-cmds.c @@ -1261,6 +1261,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict) case MIGRATION_PARAMETER_THROTTLE_TRIGGER_THRESHOLD: p->has_throttle_trigger_threshold = true; visit_type_int(v, param, >throttle_trigger_threshold, ); +break; case MIGRATION_PARAMETER_CPU_THROTTLE_INITIAL: p->has_cpu_throttle_initial = true; visit_type_int(v, param, >cpu_throttle_initial, ); -- 2.21.0.windows.1
[PATCH v5 2/4] mac_via: fix incorrect creation of mos6522 device in mac_via
This patch fix a bug in mac_via where it failed to actually realize devices it
was using.
And move the init codes which inits the mos6522 objects and properties on them
from realize()
into init(). However, we keep qdev_set_parent_bus in realize(), otherwise it
will cause
device-introspect-test test fail. Then do the realize mos6522 device in the
mac_vir_realize.
Signed-off-by: Pan Nengyuan
---
Cc: Laurent Vivier
Cc: Mark Cave-Ayland
---
v4->v3:
- split v3 into two patches, this patch fix incorrect creation of mos6522, move
inits and props
from realize into init.
v5->v4:
- remove redundant code.
- As discussion in https://patchwork.kernel.org/patch/11421229/, we still keep
qdev_set_parent_bus in realize().
---
hw/misc/mac_via.c | 40 +++-
1 file changed, 27 insertions(+), 13 deletions(-)
diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
index b7d0012794..d208f0b18a 100644
--- a/hw/misc/mac_via.c
+++ b/hw/misc/mac_via.c
@@ -868,24 +868,24 @@ static void mac_via_reset(DeviceState *dev)
static void mac_via_realize(DeviceState *dev, Error **errp)
{
MacVIAState *m = MAC_VIA(dev);
-MOS6522State *ms;
struct tm tm;
int ret;
+Error *err = NULL;
-/* Init VIAs 1 and 2 */
-sysbus_init_child_obj(OBJECT(dev), "via1", >mos6522_via1,
- sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1);
+qdev_set_parent_bus(DEVICE(>mos6522_via1), sysbus_get_default());
+qdev_set_parent_bus(DEVICE(>mos6522_via2), sysbus_get_default());
-sysbus_init_child_obj(OBJECT(dev), "via2", >mos6522_via2,
- sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2);
+object_property_set_bool(OBJECT(>mos6522_via1), true, "realized", );
+if (err != NULL) {
+error_propagate(errp, err);
+return;
+}
-/* Pass through mos6522 output IRQs */
-ms = MOS6522(>mos6522_via1);
-object_property_add_alias(OBJECT(dev), "irq[0]", OBJECT(ms),
- SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
-ms = MOS6522(>mos6522_via2);
-object_property_add_alias(OBJECT(dev), "irq[1]", OBJECT(ms),
- SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
+object_property_set_bool(OBJECT(>mos6522_via2), true, "realized", );
+if (err != NULL) {
+error_propagate(errp, err);
+return;
+}
/* Pass through mos6522 input IRQs */
qdev_pass_gpios(DEVICE(>mos6522_via1), dev, "via1-irq");
@@ -948,6 +948,20 @@ static void mac_via_init(Object *obj)
/* ADB */
qbus_create_inplace((BusState *)>adb_bus, sizeof(m->adb_bus),
TYPE_ADB_BUS, DEVICE(obj), "adb.0");
+
+/* Init VIAs 1 and 2 */
+object_initialize_child(OBJECT(m), "via1", >mos6522_via1,
+sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1,
+_abort, NULL);
+object_initialize_child(OBJECT(m), "via2", >mos6522_via2,
+sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2,
+_abort, NULL);
+
+/* Pass through mos6522 output IRQs */
+object_property_add_alias(OBJECT(m), "irq[0]", OBJECT(>mos6522_via1),
+ SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
+object_property_add_alias(OBJECT(m), "irq[1]", OBJECT(>mos6522_via2),
+ SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
}
static void postload_update_cb(void *opaque, int running, RunState state)
--
2.18.2
[PATCH v5 3/4] hw/misc/macio: fix incorrect creation of mos6522's subclasses
There are two other places where we create mos6522's subclasses but forgot to
realize
it. This patch do the realize in these places to fix that.
Signed-off-by: Pan Nengyuan
---
Cc: Mark Cave-Ayland
Cc: David Gibson
Cc: qemu-...@nongnu.org
---
v5:
- Also fix incorrect creation of mos6522's subclasses on two other places.
Not sure if there is a conversion plan, we still keep sysbus_init_child_obj
in init()
in this patch as it was.
---
hw/misc/macio/cuda.c | 11 +--
hw/misc/macio/pmu.c | 11 +--
2 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
index e0cc0aac5d..ee780a8288 100644
--- a/hw/misc/macio/cuda.c
+++ b/hw/misc/macio/cuda.c
@@ -36,6 +36,7 @@
#include "qemu/cutils.h"
#include "qemu/log.h"
#include "qemu/module.h"
+#include "qapi/error.h"
#include "trace.h"
/* Bits in B data register: all active low */
@@ -524,11 +525,17 @@ static void cuda_realize(DeviceState *dev, Error **errp)
CUDAState *s = CUDA(dev);
SysBusDevice *sbd;
MOS6522State *ms;
-DeviceState *d;
+DeviceState *d = DEVICE(>mos6522_cuda);
struct tm tm;
+Error *err = NULL;
+
+object_property_set_bool(OBJECT(d), true, "realized", );
+if (err != NULL) {
+error_propagate(errp, err);
+return;
+}
/* Pass IRQ from 6522 */
-d = DEVICE(>mos6522_cuda);
ms = MOS6522(d);
sbd = SYS_BUS_DEVICE(s);
sysbus_pass_irq(sbd, SYS_BUS_DEVICE(ms));
diff --git a/hw/misc/macio/pmu.c b/hw/misc/macio/pmu.c
index b8466a4a3f..ae55992288 100644
--- a/hw/misc/macio/pmu.c
+++ b/hw/misc/macio/pmu.c
@@ -43,6 +43,7 @@
#include "qemu/cutils.h"
#include "qemu/log.h"
#include "qemu/module.h"
+#include "qapi/error.h"
#include "trace.h"
@@ -741,11 +742,17 @@ static void pmu_realize(DeviceState *dev, Error **errp)
PMUState *s = VIA_PMU(dev);
SysBusDevice *sbd;
MOS6522State *ms;
-DeviceState *d;
+DeviceState *d = DEVICE(>mos6522_pmu);;
struct tm tm;
+Error *err = NULL;
+
+object_property_set_bool(OBJECT(d), true, "realized", );
+if (err != NULL) {
+error_propagate(errp, err);
+return;
+}
/* Pass IRQ from 6522 */
-d = DEVICE(>mos6522_pmu);
ms = MOS6522(d);
sbd = SYS_BUS_DEVICE(s);
sysbus_pass_irq(sbd, SYS_BUS_DEVICE(ms));
--
2.18.2
[PATCH v5 4/4] hw/misc/mos6522: move timer_new from init() into realize() to avoid memleaks
There are some memleaks when we call 'device_list_properties'.
This patch move timer_new from init into realize to fix it.
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
---
Cc: Laurent Vivier
Cc: Mark Cave-Ayland
Cc: David Gibson
Cc: qemu-...@nongnu.org
---
v2->v1:
- no changes in this patch.
v3->v2:
- remove null check in reset, and add calls to mos6522_realize() in
mac_via_realize to make this move to be valid.
v4->v3:
- split patch into two, this patch fix the memleaks.
v5->v4:
- No change in this patch. But add [patch 3/4] to fix SEGVs during make
check-qtest-ppc64 if apply this patch.
This patch also depend to another fix [patch 2/4].
---
hw/misc/mos6522.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index 19e154b870..c1cd154a84 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -485,6 +485,11 @@ static void mos6522_init(Object *obj)
for (i = 0; i < ARRAY_SIZE(s->timers); i++) {
s->timers[i].index = i;
}
+}
+
+static void mos6522_realize(DeviceState *dev, Error **errp)
+{
+MOS6522State *s = MOS6522(dev);
s->timers[0].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer1, s);
s->timers[1].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer2, s);
@@ -502,6 +507,7 @@ static void mos6522_class_init(ObjectClass *oc, void *data)
dc->reset = mos6522_reset;
dc->vmsd = _mos6522;
+dc->realize = mos6522_realize;
device_class_set_props(dc, mos6522_properties);
mdc->parent_reset = dc->reset;
mdc->set_sr_int = mos6522_set_sr_int;
--
2.18.2
[PATCH v5 1/4] s390x: fix memleaks in cpu_finalize
This patch fix memleaks when we call tests/qtest/cpu-plug-test on s390x. The
leak stack is as follow:
Direct leak of 48 byte(s) in 1 object(s) allocated from:
#0 0x7fb43c7cd970 in __interceptor_calloc (/lib64/libasan.so.5+0xef970)
#1 0x7fb43be2149d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5249d)
#2 0x558ba96da716 in timer_new_full
/mnt/sdb/qemu-new/qemu/include/qemu/timer.h:530
#3 0x558ba96da716 in timer_new
/mnt/sdb/qemu-new/qemu/include/qemu/timer.h:551
#4 0x558ba96da716 in timer_new_ns
/mnt/sdb/qemu-new/qemu/include/qemu/timer.h:569
#5 0x558ba96da716 in s390_cpu_initfn
/mnt/sdb/qemu-new/qemu/target/s390x/cpu.c:285
#6 0x558ba9c969ab in object_init_with_type
/mnt/sdb/qemu-new/qemu/qom/object.c:372
#7 0x558ba9c9eb5f in object_initialize_with_type
/mnt/sdb/qemu-new/qemu/qom/object.c:516
#8 0x558ba9c9f053 in object_new_with_type
/mnt/sdb/qemu-new/qemu/qom/object.c:684
#9 0x558ba967ede6 in s390x_new_cpu
/mnt/sdb/qemu-new/qemu/hw/s390x/s390-virtio-ccw.c:64
#10 0x558ba99764b3 in hmp_cpu_add
/mnt/sdb/qemu-new/qemu/hw/core/machine-hmp-cmds.c:57
#11 0x558ba9b1c27f in handle_hmp_command
/mnt/sdb/qemu-new/qemu/monitor/hmp.c:1082
#12 0x558ba96c1b02 in qmp_human_monitor_command
/mnt/sdb/qemu-new/qemu/monitor/misc.c:142
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
---
Cc: Richard Henderson
Cc: David Hildenbrand
Cc: Cornelia Huck
Cc: qemu-s3...@nongnu.org
---
v2->v1:
- Similarly to other cleanups, move timer_new into realize(Suggested by
Philippe Mathieu-Daudé)
v3->v2:
- Also do the timer_free in unrealize, it seems balanced.
v4->v3:
- Also do timer_free on the error path in realize() and fix some coding style.
- Use device_class_set_parent_unrealize to declare unrealize.
v5->v4:
- remove timer_del on the error path of realize(), it's redundant. (Suggested
by David Hildenbrand)
- Simply use errp instead a temporary variable. (Suggested by David Hildenbrand)
---
target/s390x/cpu-qom.h | 1 +
target/s390x/cpu.c | 30 ++
2 files changed, 27 insertions(+), 4 deletions(-)
diff --git a/target/s390x/cpu-qom.h b/target/s390x/cpu-qom.h
index dbe5346ec9..af9ffed0d8 100644
--- a/target/s390x/cpu-qom.h
+++ b/target/s390x/cpu-qom.h
@@ -61,6 +61,7 @@ typedef struct S390CPUClass {
const char *desc;
DeviceRealize parent_realize;
+DeviceUnrealize parent_unrealize;
void (*parent_reset)(CPUState *cpu);
void (*load_normal)(CPUState *cpu);
void (*reset)(CPUState *cpu, cpu_reset_type type);
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index cf84d307c6..8ce38bf399 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -182,6 +182,12 @@ static void s390_cpu_realizefn(DeviceState *dev, Error
**errp)
#if !defined(CONFIG_USER_ONLY)
MachineState *ms = MACHINE(qdev_get_machine());
unsigned int max_cpus = ms->smp.max_cpus;
+
+cpu->env.tod_timer =
+timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_tod_timer, cpu);
+cpu->env.cpu_timer =
+timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_cpu_timer, cpu);
+
if (cpu->env.core_id >= max_cpus) {
error_setg(, "Unable to add CPU with core-id: %" PRIu32
", maximum core-id: %d", cpu->env.core_id,
@@ -224,9 +230,27 @@ static void s390_cpu_realizefn(DeviceState *dev, Error
**errp)
scc->parent_realize(dev, );
out:
+timer_free(cpu->env.tod_timer);
+timer_free(cpu->env.cpu_timer);
error_propagate(errp, err);
}
+static void s390_cpu_unrealizefn(DeviceState *dev, Error **errp)
+{
+S390CPUClass *scc = S390_CPU_GET_CLASS(dev);
+
+#if !defined(CONFIG_USER_ONLY)
+S390CPU *cpu = S390_CPU(dev);
+
+timer_del(cpu->env.tod_timer);
+timer_del(cpu->env.cpu_timer);
+timer_free(cpu->env.tod_timer);
+timer_free(cpu->env.cpu_timer);
+#endif
+
+scc->parent_unrealize(dev, errp);
+}
+
static GuestPanicInformation *s390_cpu_get_crash_info(CPUState *cs)
{
GuestPanicInformation *panic_info;
@@ -279,10 +303,6 @@ static void s390_cpu_initfn(Object *obj)
s390_cpu_get_crash_info_qom, NULL, NULL, NULL, NULL);
s390_cpu_model_register_props(obj);
#if !defined(CONFIG_USER_ONLY)
-cpu->env.tod_timer =
-timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_tod_timer, cpu);
-cpu->env.cpu_timer =
-timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_cpu_timer, cpu);
s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
#endif
}
@@ -453,6 +473,8 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
device_class_set_parent_realize(dc, s390_cpu_realizefn,
>parent_realize);
+device_class_set_parent_unrealize(dc, s390_cpu_unrealizefn,
+ >parent_unrealize);
device_class_set_props(dc, s390x_cpu_properties);
dc->user_creatable = true;
--
2.18.2
[PATCH v5 0/4] delay timer_new from init to realize to fix memleaks.
This series delay timer_new from init into realize to avoid memleaks when we call 'device_list_properties'. And do timer_free only in s390x_cpu_finalize because it's hotplugable. However, mos6522_realize is never called at all due to the incorrect creation of it. So we fix the incorrect creation in mac_via/cuda/pmu first, then move the timer_new to mos6522_realize(). v1: - Delay timer_new() from init() to realize() to fix memleaks. v2: - Similarly to other cleanups, move timer_new into realize in target/s390x/cpu.c (Suggested by Philippe Mathieu-Daudé). - Send these two patches as a series instead of send each as a single patch but with wrong subject in v1. v3: - It's not valid in mos6522 if we move timer_new from init to realize, because it's never called at all. Thus, we remove null check in reset, and add calls to mos6522_realize() in mac_via_realize to make this move to be valid. - split patch by device to make it more clear. v4: - Also do timer_free on the error path in realize() and fix some coding style. Then use device_class_set_parent_unrealize to declare unrealize. - split the mos6522 patch into two, one to fix incorrect creation of mos6522, the other to fix memleak. v5: - Fix two other places where we create mos6522's subclasses but forgot to realize it(macio/cuda,macio/pmu). Otherwise, this will cause SEGVs during make check-qtest-ppc64. - Remove timer_del on the error path of s390x_cpu_realize() and simply use errp instead a temporary variable. Pan Nengyuan (4): s390x: fix memleaks in cpu_finalize mac_via: fix incorrect creation of mos6522 device in mac_via hw/misc/macio: fix incorrect creation of mos6522's subclasses hw/misc/mos6522: move timer_new from init() into realize() to avoid memleaks hw/misc/mac_via.c | 40 +++- hw/misc/macio/cuda.c | 11 +-- hw/misc/macio/pmu.c| 11 +-- hw/misc/mos6522.c | 6 ++ target/s390x/cpu-qom.h | 1 + target/s390x/cpu.c | 30 ++ 6 files changed, 78 insertions(+), 21 deletions(-) -- 2.18.2
[PATCH] qom-qmp-cmds: remove unnecessary alloc in qmp_object_add to fix memleak
In qmp_object_add(), user_creatable_add_type() may set errp with some error
message and
return NULL. In this case, qmp_object_add() still alloc memory to *ret_data
which return
to the caller and causes a memory leak.
This patch do this alloc() action only if obj is not NULL to fix it. And
initialize ret_data
in xen-block to avoid a possible uninitialized error.
The Leak stack:
Direct leak of 4120 byte(s) in 1 object(s) allocated from:
#0 0x7f6106ce5970 in __interceptor_calloc (/lib64/libasan.so.5+0xef970)
#1 0x7f6105e6a49d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5249d)
#2 0x55d2c58c17fd in qdict_new
/mnt/sdb/qemu-new/qemu_test/qemu/qobject/qdict.c:29
#3 0x55d2c53a0051 in qmp_object_add
/mnt/sdb/qemu-new/qemu_test/qemu/qom/qom-qmp-cmds.c:291
#4 0x55d2c57b47da in do_qmp_dispatch
/mnt/sdb/qemu-new/qemu_test/qemu/qapi/qmp-dispatch.c:132
#5 0x55d2c57b47da in qmp_dispatch
/mnt/sdb/qemu-new/qemu_test/qemu/qapi/qmp-dispatch.c:175
#6 0x55d2c52f1430 in monitor_qmp_dispatch
/mnt/sdb/qemu-new/qemu_test/qemu/monitor/qmp.c:145
#7 0x55d2c52f3087 in monitor_qmp_bh_dispatcher
/mnt/sdb/qemu-new/qemu_test/qemu/monitor/qmp.c:234
#8 0x55d2c58e6153 in aio_bh_call
/mnt/sdb/qemu-new/qemu_test/qemu/util/async.c:136
Fixes: 5f07c4d60d091320186e7b0edaf9ed2cc16b2d1e
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
---
Cc: Kevin Wolf
---
hw/block/xen-block.c | 2 +-
qom/qom-qmp-cmds.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index 3885464513..041866b846 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -860,7 +860,7 @@ static XenBlockIOThread *xen_block_iothread_create(const
char *id,
XenBlockIOThread *iothread = g_new(XenBlockIOThread, 1);
Error *local_err = NULL;
QDict *opts;
-QObject *ret_data;
+QObject *ret_data = NULL;
iothread->id = g_strdup(id);
diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
index 435193b036..6bd137ccbf 100644
--- a/qom/qom-qmp-cmds.c
+++ b/qom/qom-qmp-cmds.c
@@ -287,8 +287,8 @@ void qmp_object_add(QDict *qdict, QObject **ret_data, Error
**errp)
visit_free(v);
if (obj) {
object_unref(obj);
+*ret_data = QOBJECT(qdict_new());
}
-*ret_data = QOBJECT(qdict_new());
}
void qmp_object_del(const char *id, Error **errp)
--
2.18.2
[PATCH v2] qom-qmp-cmds: fix two memleaks in qmp_object_add
'type/id' forgot to free in qmp_object_add, this patch fix that.
The leak stack:
Direct leak of 84 byte(s) in 6 object(s) allocated from:
#0 0x7fe2a5ebf768 in __interceptor_malloc (/lib64/libasan.so.5+0xef768)
#1 0x7fe2a505 in g_malloc (/lib64/libglib-2.0.so.0+0x52445)
#2 0x7fe2a505dd92 in g_strdup (/lib64/libglib-2.0.so.0+0x6bd92)
#3 0x56344954e692 in qmp_object_add
/mnt/sdb/qemu-new/qemu_test/qemu/qom/qom-qmp-cmds.c:258
#4 0x563449960f5a in do_qmp_dispatch
/mnt/sdb/qemu-new/qemu_test/qemu/qapi/qmp-dispatch.c:132
#5 0x563449960f5a in qmp_dispatch
/mnt/sdb/qemu-new/qemu_test/qemu/qapi/qmp-dispatch.c:175
#6 0x563449498a30 in monitor_qmp_dispatch
/mnt/sdb/qemu-new/qemu_test/qemu/monitor/qmp.c:145
#7 0x56344949a64f in monitor_qmp_bh_dispatcher
/mnt/sdb/qemu-new/qemu_test/qemu/monitor/qmp.c:234
#8 0x563449a92a3a in aio_bh_call
/mnt/sdb/qemu-new/qemu_test/qemu/util/async.c:136
Direct leak of 54 byte(s) in 6 object(s) allocated from:
#0 0x7fe2a5ebf768 in __interceptor_malloc (/lib64/libasan.so.5+0xef768)
#1 0x7fe2a505 in g_malloc (/lib64/libglib-2.0.so.0+0x52445)
#2 0x7fe2a505dd92 in g_strdup (/lib64/libglib-2.0.so.0+0x6bd92)
#3 0x56344954e6c4 in qmp_object_add
/mnt/sdb/qemu-new/qemu_test/qemu/qom/qom-qmp-cmds.c:267
#4 0x563449960f5a in do_qmp_dispatch
/mnt/sdb/qemu-new/qemu_test/qemu/qapi/qmp-dispatch.c:132
#5 0x563449960f5a in qmp_dispatch
/mnt/sdb/qemu-new/qemu_test/qemu/qapi/qmp-dispatch.c:175
#6 0x563449498a30 in monitor_qmp_dispatch
/mnt/sdb/qemu-new/qemu_test/qemu/monitor/qmp.c:145
#7 0x56344949a64f in monitor_qmp_bh_dispatcher
/mnt/sdb/qemu-new/qemu_test/qemu/monitor/qmp.c:234
#8 0x563449a92a3a in aio_bh_call
/mnt/sdb/qemu-new/qemu_test/qemu/util/async.c:136
Fixes: 5f07c4d60d091320186e7b0edaf9ed2cc16b2d1e
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
---
v2->v1:
- do not store both a const and non-const string in the same variable, change
it to a non-const string.
(Suggested by Daniel P. Berrangé)
---
qom/qom-qmp-cmds.c | 16 ++--
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
index 49db926fcc..435193b036 100644
--- a/qom/qom-qmp-cmds.c
+++ b/qom/qom-qmp-cmds.c
@@ -247,26 +247,22 @@ void qmp_object_add(QDict *qdict, QObject **ret_data,
Error **errp)
QDict *pdict;
Visitor *v;
Object *obj;
-const char *type;
-const char *id;
+g_autofree char *type = NULL;
+g_autofree char *id = NULL;
-type = qdict_get_try_str(qdict, "qom-type");
+type = g_strdup(qdict_get_try_str(qdict, "qom-type"));
if (!type) {
error_setg(errp, QERR_MISSING_PARAMETER, "qom-type");
return;
-} else {
-type = g_strdup(type);
-qdict_del(qdict, "qom-type");
}
+qdict_del(qdict, "qom-type");
-id = qdict_get_try_str(qdict, "id");
+id = g_strdup(qdict_get_try_str(qdict, "id"));
if (!id) {
error_setg(errp, QERR_MISSING_PARAMETER, "id");
return;
-} else {
-id = g_strdup(id);
-qdict_del(qdict, "id");
}
+qdict_del(qdict, "id");
props = qdict_get(qdict, "props");
if (props) {
--
2.18.2
Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via
On 3/10/2020 12:16 AM, Mark Cave-Ayland wrote:
> On 09/03/2020 14:14, Markus Armbruster wrote:
>
>> Pan Nengyuan writes:
>>
>>> On 3/9/2020 8:34 PM, Markus Armbruster wrote:
>>>> Peter Maydell writes:
>>>>
>>>>> On Mon, 9 Mar 2020 at 10:02, Pan Nengyuan wrote:
>>>>>> On 3/9/2020 5:21 PM, Peter Maydell wrote:
>>>>>>> Could you explain more? My thought is that we should be using
>>>>>>> sysbus_init_child_obj() and we should be doing it in the init method.
>>>>>>> Why does that break the tests ? It's the same thing various other
>>>>>>> devices do.
>>>>>>
>>>>>> device-introspect-test do the follow check for each device type:
>>>>>>
>>>>>> qtree_start = qtest_hmp(qts, "info qtree");
>>>>>> ...
>>>>>> qtest_qmp(qts, "{'execute': 'device-list-properties','arguments':
>>>>>> {'typename': %s}}", type);
>>>>>> ...
>>>>>> qtree_end = qtest_hmp(qts, "info qtree");
>>>>>> g_assert_cmpstr(qtree_start, ==, qtree_end);
>>>>>>
>>>>>> If we do qdev_set_parent_bus in init, it will check fail when type =
>>>>>> 'mac_via'.
>>>>>> mac_via_init() is called by q800_init(). But it will not be called in
>>>>>> qtest(-machine none) in the step qtree_start.
>>>>>> And after we call 'device-list-properties', mac_via_init() was called
>>>>>> and set dev parent bus. We can find these
>>>>>> devices in the qtree_end. So it break the test on the assert.
>>>>>
>>>>> Markus, do you know what's happening here? Why is
>>>>> trying to use sysbus_init_child_obj() breaking the
>>>>> device-introspect-test for this particular device,
>>>>> but fine for the other places where we use it?
>>>>> (Maybe we're accidentally leaking a reference to
>>>>> something so the sub-device stays on the sysbus
>>>>> when it should have removed itself when the
>>>>> device was deinited ?)
>>>>
>>>> Pan Nengyuan, please provide the exact patch that fails for you.
>>>
>>> As the follow patch:
>>>
>>> >From 9b4f35e294597410cc03b967c127242ce099692e Mon Sep 17 00:00:00 2001
>>> From: Pan Nengyuan
>>> Date: Wed, 4 Mar 2020 11:29:28 +0800
>>> Subject: [PATCH] mac_via: fix incorrect creation of mos6522 device in
>>> mac_via
>>>
>>> This patch fix a bug in mac_via where it failed to actually realize devices
>>> it was using.
>>> And move the init codes which inits the mos6522 objects and properties on
>>> them from realize()
>>> into init(). However, we keep qdev_set_parent_bus in realize(), otherwise
>>> it will cause
>>> device-introspect-test test fail. Then do the realize mos6522 device in the
>>> mac_vir_realize.
>>>
>>> Signed-off-by: Pan Nengyuan
>>> ---
>>> hw/misc/mac_via.c | 40 ++--
>>> 1 file changed, 26 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
>>> index b7d0012794..4c5c432140 100644
>>> --- a/hw/misc/mac_via.c
>>> +++ b/hw/misc/mac_via.c
>>> @@ -868,24 +868,21 @@ static void mac_via_reset(DeviceState *dev)
>>> static void mac_via_realize(DeviceState *dev, Error **errp)
>>> {
>>> MacVIAState *m = MAC_VIA(dev);
>>> -MOS6522State *ms;
>>> struct tm tm;
>>> int ret;
>>> +Error *err = NULL;
>>>
>>> -/* Init VIAs 1 and 2 */
>>> -sysbus_init_child_obj(OBJECT(dev), "via1", >mos6522_via1,
>>> - sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1);
>>> -
>>> -sysbus_init_child_obj(OBJECT(dev), "via2", >mos6522_via2,
>>> - sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2);
>>> +object_property_set_bool(OBJECT(>mos6522_via1), true, "realized",
>>> );
>>> +if (err != NULL) {
>>> +error_propagate(errp, err);
>>> +return;
>>> +}
>>>
>>> -/* Pass through mos6522 output IRQs */
>>> -ms = MOS6522(>mos6522_via1);
>>> -object_property
Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via
On 3/9/2020 8:34 PM, Markus Armbruster wrote:
> Peter Maydell writes:
>
>> On Mon, 9 Mar 2020 at 10:02, Pan Nengyuan wrote:
>>> On 3/9/2020 5:21 PM, Peter Maydell wrote:
>>>> Could you explain more? My thought is that we should be using
>>>> sysbus_init_child_obj() and we should be doing it in the init method.
>>>> Why does that break the tests ? It's the same thing various other
>>>> devices do.
>>>
>>> device-introspect-test do the follow check for each device type:
>>>
>>> qtree_start = qtest_hmp(qts, "info qtree");
>>> ...
>>> qtest_qmp(qts, "{'execute': 'device-list-properties','arguments':
>>> {'typename': %s}}", type);
>>> ...
>>> qtree_end = qtest_hmp(qts, "info qtree");
>>> g_assert_cmpstr(qtree_start, ==, qtree_end);
>>>
>>> If we do qdev_set_parent_bus in init, it will check fail when type =
>>> 'mac_via'.
>>> mac_via_init() is called by q800_init(). But it will not be called in
>>> qtest(-machine none) in the step qtree_start.
>>> And after we call 'device-list-properties', mac_via_init() was called and
>>> set dev parent bus. We can find these
>>> devices in the qtree_end. So it break the test on the assert.
>>
>> Markus, do you know what's happening here? Why is
>> trying to use sysbus_init_child_obj() breaking the
>> device-introspect-test for this particular device,
>> but fine for the other places where we use it?
>> (Maybe we're accidentally leaking a reference to
>> something so the sub-device stays on the sysbus
>> when it should have removed itself when the
>> device was deinited ?)
>
> Pan Nengyuan, please provide the exact patch that fails for you.
As the follow patch:
>From 9b4f35e294597410cc03b967c127242ce099692e Mon Sep 17 00:00:00 2001
From: Pan Nengyuan
Date: Wed, 4 Mar 2020 11:29:28 +0800
Subject: [PATCH] mac_via: fix incorrect creation of mos6522 device in mac_via
This patch fix a bug in mac_via where it failed to actually realize devices it
was using.
And move the init codes which inits the mos6522 objects and properties on them
from realize()
into init(). However, we keep qdev_set_parent_bus in realize(), otherwise it
will cause
device-introspect-test test fail. Then do the realize mos6522 device in the
mac_vir_realize.
Signed-off-by: Pan Nengyuan
---
hw/misc/mac_via.c | 40 ++--
1 file changed, 26 insertions(+), 14 deletions(-)
diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
index b7d0012794..4c5c432140 100644
--- a/hw/misc/mac_via.c
+++ b/hw/misc/mac_via.c
@@ -868,24 +868,21 @@ static void mac_via_reset(DeviceState *dev)
static void mac_via_realize(DeviceState *dev, Error **errp)
{
MacVIAState *m = MAC_VIA(dev);
-MOS6522State *ms;
struct tm tm;
int ret;
+Error *err = NULL;
-/* Init VIAs 1 and 2 */
-sysbus_init_child_obj(OBJECT(dev), "via1", >mos6522_via1,
- sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1);
-
-sysbus_init_child_obj(OBJECT(dev), "via2", >mos6522_via2,
- sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2);
+object_property_set_bool(OBJECT(>mos6522_via1), true, "realized", );
+if (err != NULL) {
+error_propagate(errp, err);
+return;
+}
-/* Pass through mos6522 output IRQs */
-ms = MOS6522(>mos6522_via1);
-object_property_add_alias(OBJECT(dev), "irq[0]", OBJECT(ms),
- SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
-ms = MOS6522(>mos6522_via2);
-object_property_add_alias(OBJECT(dev), "irq[1]", OBJECT(ms),
- SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
+object_property_set_bool(OBJECT(>mos6522_via2), true, "realized", );
+if (err != NULL) {
+error_propagate(errp, err);
+return;
+}
/* Pass through mos6522 input IRQs */
qdev_pass_gpios(DEVICE(>mos6522_via1), dev, "via1-irq");
@@ -932,6 +929,7 @@ static void mac_via_init(Object *obj)
{
SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
MacVIAState *m = MAC_VIA(obj);
+MOS6522State *ms;
/* MMIO */
memory_region_init(>mmio, obj, "mac-via", 2 * VIA_SIZE);
@@ -948,6 +946,20 @@ static void mac_via_init(Object *obj)
/* ADB */
qbus_create_inplace((BusState *)>adb_bus, sizeof(m->adb_bus),
TYPE_ADB_BUS, DEVICE(obj), "adb.0");
+
+/* Init VIAs 1 and 2 */
+sysbus_init_child_obj(OBJECT(m), "via1", >mos6522_via1,
+ sizeof(m->mos
Re: [PATCH] core/qdev: fix memleak in qdev_get_gpio_out_connector()
On 3/9/2020 5:16 PM, Laurent Vivier wrote:
> Le 07/03/2020 à 11:39, Marc-André Lureau a écrit :
>> Hi
>>
>> On Sat, Mar 7, 2020 at 3:53 AM Pan Nengyuan wrote:
>>>
>>> Fix a memory leak in qdev_get_gpio_out_connector().
>>>
>>> Reported-by: Euler Robot
>>> Signed-off-by: Pan Nengyuan
>>
>> good catch,
>> Reviewed-by: Marc-André Lureau
>
> trivial question:
>
> Why do we prefer g_autofree() to the g_free() function?
Honestly, it's no special reason in this case, just personal preference. :)
Both of them is ok.
Thanks.
>
> Thanks,
> Laurent
>
>>> ---
>>> hw/core/qdev.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
>>> index 3937d1eb1a..85f062def7 100644
>>> --- a/hw/core/qdev.c
>>> +++ b/hw/core/qdev.c
>>> @@ -557,7 +557,7 @@ void qdev_connect_gpio_out_named(DeviceState *dev,
>>> const char *name, int n,
>>>
>>> qemu_irq qdev_get_gpio_out_connector(DeviceState *dev, const char *name,
>>> int n)
>>> {
>>> -char *propname = g_strdup_printf("%s[%d]",
>>> +g_autofree char *propname = g_strdup_printf("%s[%d]",
>>> name ? name : "unnamed-gpio-out", n);
>>>
>>> qemu_irq ret = (qemu_irq)object_property_get_link(OBJECT(dev),
>>> propname,
>>> --
>>> 2.18.2
>>>
>>>
>>
>>
>
Re: [PATCH] qom-qmp-cmds: fix two memleaks in qmp_object_add
On 3/9/2020 6:15 PM, Daniel P. Berrangé wrote:
> On Mon, Mar 09, 2020 at 10:51:45AM +0100, Igor Mammedov wrote:
>> On Mon, 9 Mar 2020 17:22:12 +0800
>> Pan Nengyuan wrote:
>>
>>> 'type/id' forgot to free in qmp_object_add, this patch fix that.
>>>
>>> The leak stack:
>>> Direct leak of 84 byte(s) in 6 object(s) allocated from:
>>> #0 0x7fe2a5ebf768 in __interceptor_malloc (/lib64/libasan.so.5+0xef768)
>>> #1 0x7fe2a505 in g_malloc (/lib64/libglib-2.0.so.0+0x52445)
>>> #2 0x7fe2a505dd92 in g_strdup (/lib64/libglib-2.0.so.0+0x6bd92)
>>> #3 0x56344954e692 in qmp_object_add
>>> /mnt/sdb/qemu-new/qemu_test/qemu/qom/qom-qmp-cmds.c:258
>>> #4 0x563449960f5a in do_qmp_dispatch
>>> /mnt/sdb/qemu-new/qemu_test/qemu/qapi/qmp-dispatch.c:132
>>> #5 0x563449960f5a in qmp_dispatch
>>> /mnt/sdb/qemu-new/qemu_test/qemu/qapi/qmp-dispatch.c:175
>>> #6 0x563449498a30 in monitor_qmp_dispatch
>>> /mnt/sdb/qemu-new/qemu_test/qemu/monitor/qmp.c:145
>>> #7 0x56344949a64f in monitor_qmp_bh_dispatcher
>>> /mnt/sdb/qemu-new/qemu_test/qemu/monitor/qmp.c:234
>>> #8 0x563449a92a3a in aio_bh_call
>>> /mnt/sdb/qemu-new/qemu_test/qemu/util/async.c:136
>>>
>>> Direct leak of 54 byte(s) in 6 object(s) allocated from:
>>> #0 0x7fe2a5ebf768 in __interceptor_malloc (/lib64/libasan.so.5+0xef768)
>>> #1 0x7fe2a505 in g_malloc (/lib64/libglib-2.0.so.0+0x52445)
>>> #2 0x7fe2a505dd92 in g_strdup (/lib64/libglib-2.0.so.0+0x6bd92)
>>> #3 0x56344954e6c4 in qmp_object_add
>>> /mnt/sdb/qemu-new/qemu_test/qemu/qom/qom-qmp-cmds.c:267
>>> #4 0x563449960f5a in do_qmp_dispatch
>>> /mnt/sdb/qemu-new/qemu_test/qemu/qapi/qmp-dispatch.c:132
>>> #5 0x563449960f5a in qmp_dispatch
>>> /mnt/sdb/qemu-new/qemu_test/qemu/qapi/qmp-dispatch.c:175
>>> #6 0x563449498a30 in monitor_qmp_dispatch
>>> /mnt/sdb/qemu-new/qemu_test/qemu/monitor/qmp.c:145
>>> #7 0x56344949a64f in monitor_qmp_bh_dispatcher
>>> /mnt/sdb/qemu-new/qemu_test/qemu/monitor/qmp.c:234
>>> #8 0x563449a92a3a in aio_bh_call
>>> /mnt/sdb/qemu-new/qemu_test/qemu/util/async.c:136
>>>
>>> Fixes: 5f07c4d60d091320186e7b0edaf9ed2cc16b2d1e
>>> Reported-by: Euler Robot
>>> Signed-off-by: Pan Nengyuan
>>> ---
>>> qom/qom-qmp-cmds.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
>>> index 49db926fcc..ac59ba1aa8 100644
>>> --- a/qom/qom-qmp-cmds.c
>>> +++ b/qom/qom-qmp-cmds.c
>>> @@ -247,8 +247,8 @@ void qmp_object_add(QDict *qdict, QObject **ret_data,
>>> Error **errp)
>>> QDict *pdict;
>>> Visitor *v;
>>> Object *obj;
>>> -const char *type;
>>> -const char *id;
>>> +g_autofree const char *type = NULL;
>>> +g_autofree const char *id = NULL;
>>
>> not sure that's it's correct
>>
>> qdict_get_try_str() returns the reference to a string within
>> the QDict, so caller who provided qdict should take care of
>> freeing it.
>
> This is correct, but two lines later we have "type = g_strdup(type)".
>
> IOW, the code is storing both a const and non-const string in the
> same variable which is gross.
>
> So there's definitely a leak, but this is also not the right way
> to fix it.
>
> To fix it, notice that g_strdup says
>
>"Duplicates a string. If str is NULL it returns NULL. The
> returned string should be freed with g_free() when no longer
> needed."
>
> IOW, instead of
>
> const char *type;
>
> type = qdict_get_try_str(qdict, "qom-type");
> if (!type) {
> error_setg(errp, QERR_MISSING_PARAMETER, "qom-type");
> return;
> } else {
> type = g_strdup(type);
> qdict_del(qdict, "qom-type");
> }
>
> we want
>
> g_autofree char *type = NULL;
>
> type = g_strdup(qdict_get_try_str(qdict, "qom-type"));
> if (!type) {
> error_setg(errp, QERR_MISSING_PARAMETER, "qom-type");
> return;
> }
>
> qdict_del(qdict, "qom-type");
Good points. Looks very clear.
Thanks.
>
>>> type = qdict_get_try_str(qdict, "qom-type");
>>> if (!type) {
>
>
>
>
> Regards,
> Daniel
>
Re: [PATCH] qom-qmp-cmds: fix two memleaks in qmp_object_add
On 3/9/2020 5:51 PM, Igor Mammedov wrote:
> On Mon, 9 Mar 2020 17:22:12 +0800
> Pan Nengyuan wrote:
>
>> 'type/id' forgot to free in qmp_object_add, this patch fix that.
>>
>> The leak stack:
>> Direct leak of 84 byte(s) in 6 object(s) allocated from:
>> #0 0x7fe2a5ebf768 in __interceptor_malloc (/lib64/libasan.so.5+0xef768)
>> #1 0x7fe2a505 in g_malloc (/lib64/libglib-2.0.so.0+0x52445)
>> #2 0x7fe2a505dd92 in g_strdup (/lib64/libglib-2.0.so.0+0x6bd92)
>> #3 0x56344954e692 in qmp_object_add
>> /mnt/sdb/qemu-new/qemu_test/qemu/qom/qom-qmp-cmds.c:258
>> #4 0x563449960f5a in do_qmp_dispatch
>> /mnt/sdb/qemu-new/qemu_test/qemu/qapi/qmp-dispatch.c:132
>> #5 0x563449960f5a in qmp_dispatch
>> /mnt/sdb/qemu-new/qemu_test/qemu/qapi/qmp-dispatch.c:175
>> #6 0x563449498a30 in monitor_qmp_dispatch
>> /mnt/sdb/qemu-new/qemu_test/qemu/monitor/qmp.c:145
>> #7 0x56344949a64f in monitor_qmp_bh_dispatcher
>> /mnt/sdb/qemu-new/qemu_test/qemu/monitor/qmp.c:234
>> #8 0x563449a92a3a in aio_bh_call
>> /mnt/sdb/qemu-new/qemu_test/qemu/util/async.c:136
>>
>> Direct leak of 54 byte(s) in 6 object(s) allocated from:
>> #0 0x7fe2a5ebf768 in __interceptor_malloc (/lib64/libasan.so.5+0xef768)
>> #1 0x7fe2a505 in g_malloc (/lib64/libglib-2.0.so.0+0x52445)
>> #2 0x7fe2a505dd92 in g_strdup (/lib64/libglib-2.0.so.0+0x6bd92)
>> #3 0x56344954e6c4 in qmp_object_add
>> /mnt/sdb/qemu-new/qemu_test/qemu/qom/qom-qmp-cmds.c:267
>> #4 0x563449960f5a in do_qmp_dispatch
>> /mnt/sdb/qemu-new/qemu_test/qemu/qapi/qmp-dispatch.c:132
>> #5 0x563449960f5a in qmp_dispatch
>> /mnt/sdb/qemu-new/qemu_test/qemu/qapi/qmp-dispatch.c:175
>> #6 0x563449498a30 in monitor_qmp_dispatch
>> /mnt/sdb/qemu-new/qemu_test/qemu/monitor/qmp.c:145
>> #7 0x56344949a64f in monitor_qmp_bh_dispatcher
>> /mnt/sdb/qemu-new/qemu_test/qemu/monitor/qmp.c:234
>> #8 0x563449a92a3a in aio_bh_call
>> /mnt/sdb/qemu-new/qemu_test/qemu/util/async.c:136
>>
>> Fixes: 5f07c4d60d091320186e7b0edaf9ed2cc16b2d1e
>> Reported-by: Euler Robot
>> Signed-off-by: Pan Nengyuan
>> ---
>> qom/qom-qmp-cmds.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
>> index 49db926fcc..ac59ba1aa8 100644
>> --- a/qom/qom-qmp-cmds.c
>> +++ b/qom/qom-qmp-cmds.c
>> @@ -247,8 +247,8 @@ void qmp_object_add(QDict *qdict, QObject **ret_data,
>> Error **errp)
>> QDict *pdict;
>> Visitor *v;
>> Object *obj;
>> -const char *type;
>> -const char *id;
>> +g_autofree const char *type = NULL;
>> +g_autofree const char *id = NULL;
>
> not sure that's it's correct
>
> qdict_get_try_str() returns the reference to a string within
> the QDict, so caller who provided qdict should take care of
> freeing it.
>
>>
>> type = qdict_get_try_str(qdict, "qom-type");
>> if (!type) {
>
type = qdict_get_try_str(qdict, "qom-type");
if (!type) {
error_setg(errp, QERR_MISSING_PARAMETER, "qom-type");
return;
} else {
type = g_strdup(type);
qdict_del(qdict, "qom-type");
}
If type is not NULL, it will go to the else branch, type re-assign immediately.
And it's aslo in the scope, so the string within the QDict will not be freed.
> .
>
Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via
On 3/9/2020 5:21 PM, Peter Maydell wrote:
> On Mon, 9 Mar 2020 at 00:56, Pan Nengyuan wrote:
>>
>>
>>
>> On 3/8/2020 9:29 PM, Peter Maydell wrote:
>>> On Thu, 5 Mar 2020 at 06:39, Pan Nengyuan wrote:
>>>> -/* Init VIAs 1 and 2 */
>>>> -sysbus_init_child_obj(OBJECT(dev), "via1", >mos6522_via1,
>>>> - sizeof(m->mos6522_via1),
>>>> TYPE_MOS6522_Q800_VIA1);
>>>> +qdev_set_parent_bus(DEVICE(>mos6522_via1), sysbus_get_default());
>>>> +qdev_set_parent_bus(DEVICE(>mos6522_via2), sysbus_get_default());
>>>
>>> Rather than manually setting the parent bus, you can use
>>> sysbus_init_child_obj() instead of object_initialize_child() --
>>> it is a convenience function that does both object_initialize_child()
>>> and qdev_set_parent_bus() for you.
>>
>> Actually I used sysbus_init_child_obj() first, but it will fail to run
>> device-introspect-test.
>> Because qdev_set_parent_bus() will change 'info qtree' after we call
>> 'device-list-properties'.
>> Thus, I do it in the realize.
>
> Could you explain more? My thought is that we should be using
> sysbus_init_child_obj() and we should be doing it in the init method.
> Why does that break the tests ? It's the same thing various other
> devices do.
device-introspect-test do the follow check for each device type:
qtree_start = qtest_hmp(qts, "info qtree");
...
qtest_qmp(qts, "{'execute': 'device-list-properties','arguments':
{'typename': %s}}", type);
...
qtree_end = qtest_hmp(qts, "info qtree");
g_assert_cmpstr(qtree_start, ==, qtree_end);
If we do qdev_set_parent_bus in init, it will check fail when type = 'mac_via'.
mac_via_init() is called by q800_init(). But it will not be called in
qtest(-machine none) in the step qtree_start.
And after we call 'device-list-properties', mac_via_init() was called and set
dev parent bus. We can find these
devices in the qtree_end. So it break the test on the assert.
Here is the output:
# Testing device 'mac_via'
adb.0=>
drive=- Node name or ID of a block device to use as a backend
irq[0]=>
irq[1]=>
mac-via[0]=>
via1=>
via1[0]=>
via2=>
via2[0]=>
qtree_start: bus: main-system-bus
type System
qtree_end: bus: main-system-bus
type System
dev: mos6522-q800-via2, id ""
gpio-in "via2-irq" 8
gpio-out "sysbus-irq" 1
frequency = 0 (0x0)
mmio /0010
dev: mos6522-q800-via1, id ""
gpio-in "via1-irq" 8
gpio-out "sysbus-irq" 1
frequency = 0 (0x0)
mmio /0010
>
> thanks
> -- PMM
> .
>
[PATCH] qom-qmp-cmds: fix two memleaks in qmp_object_add
'type/id' forgot to free in qmp_object_add, this patch fix that.
The leak stack:
Direct leak of 84 byte(s) in 6 object(s) allocated from:
#0 0x7fe2a5ebf768 in __interceptor_malloc (/lib64/libasan.so.5+0xef768)
#1 0x7fe2a505 in g_malloc (/lib64/libglib-2.0.so.0+0x52445)
#2 0x7fe2a505dd92 in g_strdup (/lib64/libglib-2.0.so.0+0x6bd92)
#3 0x56344954e692 in qmp_object_add
/mnt/sdb/qemu-new/qemu_test/qemu/qom/qom-qmp-cmds.c:258
#4 0x563449960f5a in do_qmp_dispatch
/mnt/sdb/qemu-new/qemu_test/qemu/qapi/qmp-dispatch.c:132
#5 0x563449960f5a in qmp_dispatch
/mnt/sdb/qemu-new/qemu_test/qemu/qapi/qmp-dispatch.c:175
#6 0x563449498a30 in monitor_qmp_dispatch
/mnt/sdb/qemu-new/qemu_test/qemu/monitor/qmp.c:145
#7 0x56344949a64f in monitor_qmp_bh_dispatcher
/mnt/sdb/qemu-new/qemu_test/qemu/monitor/qmp.c:234
#8 0x563449a92a3a in aio_bh_call
/mnt/sdb/qemu-new/qemu_test/qemu/util/async.c:136
Direct leak of 54 byte(s) in 6 object(s) allocated from:
#0 0x7fe2a5ebf768 in __interceptor_malloc (/lib64/libasan.so.5+0xef768)
#1 0x7fe2a505 in g_malloc (/lib64/libglib-2.0.so.0+0x52445)
#2 0x7fe2a505dd92 in g_strdup (/lib64/libglib-2.0.so.0+0x6bd92)
#3 0x56344954e6c4 in qmp_object_add
/mnt/sdb/qemu-new/qemu_test/qemu/qom/qom-qmp-cmds.c:267
#4 0x563449960f5a in do_qmp_dispatch
/mnt/sdb/qemu-new/qemu_test/qemu/qapi/qmp-dispatch.c:132
#5 0x563449960f5a in qmp_dispatch
/mnt/sdb/qemu-new/qemu_test/qemu/qapi/qmp-dispatch.c:175
#6 0x563449498a30 in monitor_qmp_dispatch
/mnt/sdb/qemu-new/qemu_test/qemu/monitor/qmp.c:145
#7 0x56344949a64f in monitor_qmp_bh_dispatcher
/mnt/sdb/qemu-new/qemu_test/qemu/monitor/qmp.c:234
#8 0x563449a92a3a in aio_bh_call
/mnt/sdb/qemu-new/qemu_test/qemu/util/async.c:136
Fixes: 5f07c4d60d091320186e7b0edaf9ed2cc16b2d1e
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
---
qom/qom-qmp-cmds.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
index 49db926fcc..ac59ba1aa8 100644
--- a/qom/qom-qmp-cmds.c
+++ b/qom/qom-qmp-cmds.c
@@ -247,8 +247,8 @@ void qmp_object_add(QDict *qdict, QObject **ret_data, Error
**errp)
QDict *pdict;
Visitor *v;
Object *obj;
-const char *type;
-const char *id;
+g_autofree const char *type = NULL;
+g_autofree const char *id = NULL;
type = qdict_get_try_str(qdict, "qom-type");
if (!type) {
--
2.18.2
[PATCH v3] virtio-serial-bus: Plug memory leak on realize() error paths
We neglect to free port->bh on the error paths. Fix that.
Reproducer:
{'execute': 'device_add', 'arguments': {'id': 'virtio_serial_pci0',
'driver': 'virtio-serial-pci', 'bus': 'pci.0', 'addr': '0x5'}, 'id': 'yVkZcGgV'}
{'execute': 'device_add', 'arguments': {'id': 'port1', 'driver':
'virtserialport', 'name': 'port1', 'chardev': 'channel1', 'bus':
'virtio_serial_pci0.0', 'nr': 1}, 'id': '3dXdUgJA'}
{'execute': 'device_add', 'arguments': {'id': 'port2', 'driver':
'virtserialport', 'name': 'port2', 'chardev': 'channel2', 'bus':
'virtio_serial_pci0.0', 'nr': 1}, 'id': 'qLzcCkob'}
{'execute': 'device_add', 'arguments': {'id': 'port2', 'driver':
'virtserialport', 'name': 'port2', 'chardev': 'channel2', 'bus':
'virtio_serial_pci0.0', 'nr': 2}, 'id': 'qLzcCkob'}
The leak stack:
Direct leak of 40 byte(s) in 1 object(s) allocated from:
#0 0x7f04a8008ae8 in __interceptor_malloc (/lib64/libasan.so.5+0xefae8)
#1 0x7f04a73cf1d5 in g_malloc (/lib64/libglib-2.0.so.0+0x531d5)
#2 0x56273eaee484 in aio_bh_new /mnt/sdb/backup/qemu/util/async.c:125
#3 0x56273eafe9a8 in qemu_bh_new /mnt/sdb/backup/qemu/util/main-loop.c:532
#4 0x56273d52e62e in virtser_port_device_realize
/mnt/sdb/backup/qemu/hw/char/virtio-serial-bus.c:946
#5 0x56273dcc5040 in device_set_realized
/mnt/sdb/backup/qemu/hw/core/qdev.c:891
#6 0x56273e5ebbce in property_set_bool
/mnt/sdb/backup/qemu/qom/object.c:2238
#7 0x56273e5e5a9c in object_property_set
/mnt/sdb/backup/qemu/qom/object.c:1324
#8 0x56273e5ef5f8 in object_property_set_qobject
/mnt/sdb/backup/qemu/qom/qom-qobject.c:26
#9 0x56273e5e5e6a in object_property_set_bool
/mnt/sdb/backup/qemu/qom/object.c:1390
#10 0x56273daa40de in qdev_device_add
/mnt/sdb/backup/qemu/qdev-monitor.c:680
#11 0x56273daa53e9 in qmp_device_add /mnt/sdb/backup/qemu/qdev-monitor.c:805
Fixes: 199646d81522509ac2dba6d28c31e8c7d807bc93
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
Reviewed-by: Markus Armbruster
Reviewed-by: Amit Shah
---
v1->v2:
- simply create port->bh last in virtser_port_device_realize() to fix
memleaks.(Suggested by Markus Armbruster)
v3->v2:
- tidy up commit message
---
hw/char/virtio-serial-bus.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 941ed5aca9..99a65bab7f 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -943,7 +943,6 @@ static void virtser_port_device_realize(DeviceState *dev,
Error **errp)
Error *err = NULL;
port->vser = bus->vser;
-port->bh = qemu_bh_new(flush_queued_data_bh, port);
assert(vsc->have_data);
@@ -992,6 +991,7 @@ static void virtser_port_device_realize(DeviceState *dev,
Error **errp)
return;
}
+port->bh = qemu_bh_new(flush_queued_data_bh, port);
port->elem = NULL;
}
--
2.18.2
Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via
On 3/8/2020 9:29 PM, Peter Maydell wrote:
> On Thu, 5 Mar 2020 at 06:39, Pan Nengyuan wrote:
>>
>> This patch fix a bug in mac_via where it failed to actually realize devices
>> it was using.
>> And move the init codes which inits the mos6522 objects and properties on
>> them from realize()
>> into init(). However, we keep qdev_set_parent_bus in realize(), otherwise it
>> will cause
>> device-introspect-test test fail. Then do the realize mos6522 device in the
>> mac_vir_realize.
>>
>> Signed-off-by: Pan Nengyuan
>> ---
>> Cc: Laurent Vivier
>> Cc: Mark Cave-Ayland
>> ---
>
>> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
>> index b7d0012794..4c5ad08805 100644
>> --- a/hw/misc/mac_via.c
>> +++ b/hw/misc/mac_via.c
>> @@ -868,24 +868,24 @@ static void mac_via_reset(DeviceState *dev)
>> static void mac_via_realize(DeviceState *dev, Error **errp)
>> {
>> MacVIAState *m = MAC_VIA(dev);
>> -MOS6522State *ms;
>> struct tm tm;
>> int ret;
>> +Error *err = NULL;
>>
>> -/* Init VIAs 1 and 2 */
>> -sysbus_init_child_obj(OBJECT(dev), "via1", >mos6522_via1,
>> - sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1);
>> +qdev_set_parent_bus(DEVICE(>mos6522_via1), sysbus_get_default());
>> +qdev_set_parent_bus(DEVICE(>mos6522_via2), sysbus_get_default());
>
> Rather than manually setting the parent bus, you can use
> sysbus_init_child_obj() instead of object_initialize_child() --
> it is a convenience function that does both object_initialize_child()
> and qdev_set_parent_bus() for you.
Actually I used sysbus_init_child_obj() first, but it will fail to run
device-introspect-test.
Because qdev_set_parent_bus() will change 'info qtree' after we call
'device-list-properties'.
Thus, I do it in the realize.
>
>> -sysbus_init_child_obj(OBJECT(dev), "via2", >mos6522_via2,
>> - sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2);
>> +object_property_set_bool(OBJECT(>mos6522_via1), true, "realized",
>> );
>> +if (err != NULL) {
>> +error_propagate(errp, err);
>> +return;
>> +}
>>
>> -/* Pass through mos6522 output IRQs */
>> -ms = MOS6522(>mos6522_via1);
>> -object_property_add_alias(OBJECT(dev), "irq[0]", OBJECT(ms),
>> - SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
>> -ms = MOS6522(>mos6522_via2);
>> -object_property_add_alias(OBJECT(dev), "irq[1]", OBJECT(ms),
>> - SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
>> +object_property_set_bool(OBJECT(>mos6522_via2), true, "realized",
>> );
>> +if (err != NULL) {
>> +error_propagate(errp, err);
>> +return;
>> +}
>>
>> /* Pass through mos6522 input IRQs */
>> qdev_pass_gpios(DEVICE(>mos6522_via1), dev, "via1-irq");
>> @@ -932,6 +932,7 @@ static void mac_via_init(Object *obj)
>> {
>> SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>> MacVIAState *m = MAC_VIA(obj);
>> +MOS6522State *ms;
>>
>> /* MMIO */
>> memory_region_init(>mmio, obj, "mac-via", 2 * VIA_SIZE);
>> @@ -948,6 +949,22 @@ static void mac_via_init(Object *obj)
>> /* ADB */
>> qbus_create_inplace((BusState *)>adb_bus, sizeof(m->adb_bus),
>> TYPE_ADB_BUS, DEVICE(obj), "adb.0");
>> +
>> +/* Init VIAs 1 and 2 */
>> +object_initialize_child(OBJECT(m), "via1", >mos6522_via1,
>> +sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1,
>> +_abort, NULL);
>> +object_initialize_child(OBJECT(m), "via2", >mos6522_via2,
>> +sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2,
>> +_abort, NULL);
>> +
>> +/* Pass through mos6522 output IRQs */
>> +ms = MOS6522(>mos6522_via1);
>> +object_property_add_alias(OBJECT(m), "irq[0]", OBJECT(ms),
>> + SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
>
> There's no point in using the MOS6522() cast to get a MOS6522*,
> because the only thing you do with it is immediately cast it
> to an Object* with the OBJECT() cast. Just use the OBJECT cast directly.
Ok, will do it.
Thanks.
>
>> +ms = MOS6522(>mos6522_via2);
>> +object_property_add_alias(OBJECT(m), "irq[1]", OBJECT(ms),
>> + SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
>> }
>>
>> static void postload_update_cb(void *opaque, int running, RunState state)
>
> thanks
> -- PMM
> .
>
Re: [PATCH v4 0/3] delay timer_new from init to realize to fix memleaks.
On 3/8/2020 9:39 PM, Peter Maydell wrote: > On Sun, 8 Mar 2020 at 11:58, Mark Cave-Ayland > wrote: >> I just tried this patchset applied on top of git master and it causes >> qemu-system-ppc >> to segfault on startup: >> >> $ gdb --args ./qemu-system-ppc >> ... >> ... >> Thread 1 "qemu-system-ppc" received signal SIGSEGV, Segmentation fault. >> 0x55e7e38c in timer_del (ts=0x0) at util/qemu-timer.c:429 >> 429 QEMUTimerList *timer_list = ts->timer_list; >> (gdb) bt >> #0 0x55e7e38c in timer_del (ts=0x0) at util/qemu-timer.c:429 >> #1 0x55b5d2c1 in mos6522_reset (dev=0x56e0ac50) at >> hw/misc/mos6522.c:468 >> #2 0x55b63570 in mos6522_cuda_reset (dev=0x56e0ac50) at >> hw/misc/macio/cuda.c:599 > > It looks like we haven't caught all the cases of "somebody created a > MOS6522 (or one of its subclasses) but forgot to realize it". This > particular one I think is the s->cuda which is inited in macio_oldworld_init() > but not realized in macio_oldworld_realize(). I think that pmu_init() in > hw/misc/macio/pmu.c also has this bug. We need to go through and > audit all the places where we create TYPE_MOS6522 or any of its > subclasses and make sure they are also realizing the devices they create. > (The presence of the new 3-phase reset infrastructure in the backtrace > is a red herring here -- this would have crashed the same way with the > old code too.) > Yes, that's right, this series miss other cases, I will search and fix all the cases about MOS6522 device. Thanks. > We should probably find some generic place in Device code where we > can stick an assert "are we trying to reset an unrealized device?" > because I bet we have other instances of this bug which we haven't > noticed because the reset function happens to not misbehave on > an inited-but-not-realized device... > > thanks > -- PMM > . >
[PATCH] core/qdev: fix memleak in qdev_get_gpio_out_connector()
Fix a memory leak in qdev_get_gpio_out_connector().
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
---
hw/core/qdev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 3937d1eb1a..85f062def7 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -557,7 +557,7 @@ void qdev_connect_gpio_out_named(DeviceState *dev, const
char *name, int n,
qemu_irq qdev_get_gpio_out_connector(DeviceState *dev, const char *name, int n)
{
-char *propname = g_strdup_printf("%s[%d]",
+g_autofree char *propname = g_strdup_printf("%s[%d]",
name ? name : "unnamed-gpio-out", n);
qemu_irq ret = (qemu_irq)object_property_get_link(OBJECT(dev), propname,
--
2.18.2
Re: [PATCH v2] virtio-serial-bus: do cleanup on the error path in realize() to avoid memleaks
On 3/6/2020 4:51 PM, Markus Armbruster wrote:
> Pan Nengyuan writes:
>
>> port->bh forgot to delete on the error path, this patch add it to fix
>> memleaks. It's easy to reproduce as follow(add a same nr port):
>
> Long line. Suggest:
>
> virtio-serial-bus: Plug memory leak on realize() error paths
>
> We neglect to free port->bh on the error paths. Fix that.
> Reproducer:
Good suggestion. Looks simple and clear.
Thanks.
>
> Perhaps the maintainer can tweak this for you without a respin.
>
>> {'execute': 'device_add', 'arguments': {'id': 'virtio_serial_pci0',
>> 'driver': 'virtio-serial-pci', 'bus': 'pci.0', 'addr': '0x5'}, 'id':
>> 'yVkZcGgV'}
>> {'execute': 'device_add', 'arguments': {'id': 'port1', 'driver':
>> 'virtserialport', 'name': 'port1', 'chardev': 'channel1', 'bus':
>> 'virtio_serial_pci0.0', 'nr': 1}, 'id': '3dXdUgJA'}
>> {'execute': 'device_add', 'arguments': {'id': 'port2', 'driver':
>> 'virtserialport', 'name': 'port2', 'chardev': 'channel2', 'bus':
>> 'virtio_serial_pci0.0', 'nr': 1}, 'id': 'qLzcCkob'}
>> {'execute': 'device_add', 'arguments': {'id': 'port2', 'driver':
>> 'virtserialport', 'name': 'port2', 'chardev': 'channel2', 'bus':
>> 'virtio_serial_pci0.0', 'nr': 2}, 'id': 'qLzcCkob'}
>>
>> The leak stack:
>> Direct leak of 40 byte(s) in 1 object(s) allocated from:
>> #0 0x7f04a8008ae8 in __interceptor_malloc (/lib64/libasan.so.5+0xefae8)
>> #1 0x7f04a73cf1d5 in g_malloc (/lib64/libglib-2.0.so.0+0x531d5)
>> #2 0x56273eaee484 in aio_bh_new /mnt/sdb/backup/qemu/util/async.c:125
>> #3 0x56273eafe9a8 in qemu_bh_new
>> /mnt/sdb/backup/qemu/util/main-loop.c:532
>> #4 0x56273d52e62e in virtser_port_device_realize
>> /mnt/sdb/backup/qemu/hw/char/virtio-serial-bus.c:946
>> #5 0x56273dcc5040 in device_set_realized
>> /mnt/sdb/backup/qemu/hw/core/qdev.c:891
>> #6 0x56273e5ebbce in property_set_bool
>> /mnt/sdb/backup/qemu/qom/object.c:2238
>> #7 0x56273e5e5a9c in object_property_set
>> /mnt/sdb/backup/qemu/qom/object.c:1324
>> #8 0x56273e5ef5f8 in object_property_set_qobject
>> /mnt/sdb/backup/qemu/qom/qom-qobject.c:26
>> #9 0x56273e5e5e6a in object_property_set_bool
>> /mnt/sdb/backup/qemu/qom/object.c:1390
>> #10 0x56273daa40de in qdev_device_add
>> /mnt/sdb/backup/qemu/qdev-monitor.c:680
>> #11 0x56273daa53e9 in qmp_device_add
>> /mnt/sdb/backup/qemu/qdev-monitor.c:805
>>
>> Fixes: 199646d81522509ac2dba6d28c31e8c7d807bc93
>> Reported-by: Euler Robot
>> Signed-off-by: Pan Nengyuan
>> ---
>> v1->v2:
>> - simply create port->bh last in virtser_port_device_realize() to fix
>> memleaks.(Suggested by Markus Armbruster)
>> ---
>> hw/char/virtio-serial-bus.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
>> index 941ed5aca9..99a65bab7f 100644
>> --- a/hw/char/virtio-serial-bus.c
>> +++ b/hw/char/virtio-serial-bus.c
>> @@ -943,7 +943,6 @@ static void virtser_port_device_realize(DeviceState
>> *dev, Error **errp)
>> Error *err = NULL;
>>
>> port->vser = bus->vser;
>> -port->bh = qemu_bh_new(flush_queued_data_bh, port);
>>
>> assert(vsc->have_data);
>>
>> @@ -992,6 +991,7 @@ static void virtser_port_device_realize(DeviceState
>> *dev, Error **errp)
>> return;
>> }
>>
>> +port->bh = qemu_bh_new(flush_queued_data_bh, port);
>> port->elem = NULL;
>> }
>
> Preferably with a tidied up commit message:
> Reviewed-by: Markus Armbruster
>
> .
>
[PATCH v2] virtio-serial-bus: do cleanup on the error path in realize() to avoid memleaks
port->bh forgot to delete on the error path, this patch add it to fix memleaks.
It's easy to reproduce as follow(add a same nr port):
{'execute': 'device_add', 'arguments': {'id': 'virtio_serial_pci0',
'driver': 'virtio-serial-pci', 'bus': 'pci.0', 'addr': '0x5'}, 'id': 'yVkZcGgV'}
{'execute': 'device_add', 'arguments': {'id': 'port1', 'driver':
'virtserialport', 'name': 'port1', 'chardev': 'channel1', 'bus':
'virtio_serial_pci0.0', 'nr': 1}, 'id': '3dXdUgJA'}
{'execute': 'device_add', 'arguments': {'id': 'port2', 'driver':
'virtserialport', 'name': 'port2', 'chardev': 'channel2', 'bus':
'virtio_serial_pci0.0', 'nr': 1}, 'id': 'qLzcCkob'}
{'execute': 'device_add', 'arguments': {'id': 'port2', 'driver':
'virtserialport', 'name': 'port2', 'chardev': 'channel2', 'bus':
'virtio_serial_pci0.0', 'nr': 2}, 'id': 'qLzcCkob'}
The leak stack:
Direct leak of 40 byte(s) in 1 object(s) allocated from:
#0 0x7f04a8008ae8 in __interceptor_malloc (/lib64/libasan.so.5+0xefae8)
#1 0x7f04a73cf1d5 in g_malloc (/lib64/libglib-2.0.so.0+0x531d5)
#2 0x56273eaee484 in aio_bh_new /mnt/sdb/backup/qemu/util/async.c:125
#3 0x56273eafe9a8 in qemu_bh_new /mnt/sdb/backup/qemu/util/main-loop.c:532
#4 0x56273d52e62e in virtser_port_device_realize
/mnt/sdb/backup/qemu/hw/char/virtio-serial-bus.c:946
#5 0x56273dcc5040 in device_set_realized
/mnt/sdb/backup/qemu/hw/core/qdev.c:891
#6 0x56273e5ebbce in property_set_bool
/mnt/sdb/backup/qemu/qom/object.c:2238
#7 0x56273e5e5a9c in object_property_set
/mnt/sdb/backup/qemu/qom/object.c:1324
#8 0x56273e5ef5f8 in object_property_set_qobject
/mnt/sdb/backup/qemu/qom/qom-qobject.c:26
#9 0x56273e5e5e6a in object_property_set_bool
/mnt/sdb/backup/qemu/qom/object.c:1390
#10 0x56273daa40de in qdev_device_add
/mnt/sdb/backup/qemu/qdev-monitor.c:680
#11 0x56273daa53e9 in qmp_device_add /mnt/sdb/backup/qemu/qdev-monitor.c:805
Fixes: 199646d81522509ac2dba6d28c31e8c7d807bc93
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
---
v1->v2:
- simply create port->bh last in virtser_port_device_realize() to fix
memleaks.(Suggested by Markus Armbruster)
---
hw/char/virtio-serial-bus.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 941ed5aca9..99a65bab7f 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -943,7 +943,6 @@ static void virtser_port_device_realize(DeviceState *dev,
Error **errp)
Error *err = NULL;
port->vser = bus->vser;
-port->bh = qemu_bh_new(flush_queued_data_bh, port);
assert(vsc->have_data);
@@ -992,6 +991,7 @@ static void virtser_port_device_realize(DeviceState *dev,
Error **errp)
return;
}
+port->bh = qemu_bh_new(flush_queued_data_bh, port);
port->elem = NULL;
}
--
2.18.2
Re: [PATCH v4 3/3] hw/misc/mos6522: move timer_new from init() into realize() to avoid memleaks
On 3/6/2020 6:56 AM, David Gibson wrote:
> On Thu, Mar 05, 2020 at 02:54:22PM +0800, Pan Nengyuan wrote:
>> There are some memleaks when we call 'device_list_properties'. This patch
>> move timer_new from init into realize to fix it.
>>
>> Reported-by: Euler Robot
>> Signed-off-by: Pan Nengyuan
>
> Applied to ppc-for-5.0.
Thanks.
And this patch depend to another fix (patch2/3:
https://patchwork.kernel.org/patch/11421229/). Otherwise, it'll be invalid for
this move.
I forgot cc it to you, but I think it should let you known.
>
> Probably the memory region stuff should be in realize() rather than
> init() as well, but that can be fixed later.
>
>> ---
>> Cc: Laurent Vivier
>> Cc: Mark Cave-Ayland
>> Cc: David Gibson
>> Cc: qemu-...@nongnu.org
>> ---
>> v2->v1:
>> - no changes in this patch.
>> v3->v2:
>> - remove null check in reset, and add calls to mos6522_realize() in
>> mac_via_realize to make this move to be valid.
>> v4->v3:
>> - split patch into two, this patch fix the memleaks.
>> ---
>> hw/misc/mos6522.c | 6 ++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
>> index 19e154b870..c1cd154a84 100644
>> --- a/hw/misc/mos6522.c
>> +++ b/hw/misc/mos6522.c
>> @@ -485,6 +485,11 @@ static void mos6522_init(Object *obj)
>> for (i = 0; i < ARRAY_SIZE(s->timers); i++) {
>> s->timers[i].index = i;
>> }
>> +}
>> +
>> +static void mos6522_realize(DeviceState *dev, Error **errp)
>> +{
>> +MOS6522State *s = MOS6522(dev);
>>
>> s->timers[0].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer1,
>> s);
>> s->timers[1].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer2,
>> s);
>> @@ -502,6 +507,7 @@ static void mos6522_class_init(ObjectClass *oc, void
>> *data)
>>
>> dc->reset = mos6522_reset;
>> dc->vmsd = _mos6522;
>> +dc->realize = mos6522_realize;
>> device_class_set_props(dc, mos6522_properties);
>> mdc->parent_reset = dc->reset;
>> mdc->set_sr_int = mos6522_set_sr_int;
>
Re: [PATCH v4 1/3] s390x: fix memleaks in cpu_finalize
On 3/5/2020 4:34 PM, David Hildenbrand wrote:
>
>> #if !defined(CONFIG_USER_ONLY)
>> MachineState *ms = MACHINE(qdev_get_machine());
>> unsigned int max_cpus = ms->smp.max_cpus;
>> +
>> +cpu->env.tod_timer =
>> +timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_tod_timer, cpu);
>> +cpu->env.cpu_timer =
>> +timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_cpu_timer, cpu);
>> +
>> if (cpu->env.core_id >= max_cpus) {
>> error_setg(, "Unable to add CPU with core-id: %" PRIu32
>> ", maximum core-id: %d", cpu->env.core_id,
>> @@ -224,9 +230,38 @@ static void s390_cpu_realizefn(DeviceState *dev, Error
>> **errp)
>>
>> scc->parent_realize(dev, );
>> out:
>> +if (cpu->env.tod_timer) {
>> +timer_del(cpu->env.tod_timer);
>> +}
>> +if (cpu->env.cpu_timer) {
>> +timer_del(cpu->env.cpu_timer);
>> +}
>> +timer_free(cpu->env.tod_timer);
>> +timer_free(cpu->env.cpu_timer);
>
> timer_free() should be sufficient, as it cannot be running, no?
Yes, it's redundant.
>
>> error_propagate(errp, err);
>> }
>>
>> +static void s390_cpu_unrealizefn(DeviceState *dev, Error **errp)
>> +{
>> +S390CPUClass *scc = S390_CPU_GET_CLASS(dev);
>> +Error *err = NULL;
>> +
>> +#if !defined(CONFIG_USER_ONLY)
>> +S390CPU *cpu = S390_CPU(dev);
>> +
>> +timer_del(cpu->env.tod_timer);
>> +timer_del(cpu->env.cpu_timer);
>> +timer_free(cpu->env.tod_timer);
>> +timer_free(cpu->env.cpu_timer);
>> +#endif
>> +
>> +scc->parent_unrealize(dev, );
>> +if (err != NULL) {
>> +error_propagate(errp, err);
>> +return;
>> +}
>> +}
>
> Simply a
>
> scc->parent_unrealize(dev, errp) and you can drop the temporary variable.
Fine, it looks more clear and I will change it.
And I think it's the same on x86_cpu_unrealize/ppc_cpu_unrealize, I refer to
the implement of them.
Thanks.
>
>
Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via
On 3/5/2020 2:54 PM, Pan Nengyuan wrote:
> This patch fix a bug in mac_via where it failed to actually realize devices
> it was using.
> And move the init codes which inits the mos6522 objects and properties on
> them from realize()
> into init(). However, we keep qdev_set_parent_bus in realize(), otherwise it
> will cause
> device-introspect-test test fail. Then do the realize mos6522 device in the
> mac_vir_realize.
>
> Signed-off-by: Pan Nengyuan
> ---
> Cc: Laurent Vivier
> Cc: Mark Cave-Ayland
> ---
> v4->v3:
> - split v3 into two patches, this patch fix incorrect creation of mos6522,
> move inits and props
> from realize into init. The v3 is:
> https://patchwork.kernel.org/patch/11407635/
> ---
> hw/misc/mac_via.c | 43 ++-
> 1 file changed, 30 insertions(+), 13 deletions(-)
>
> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
> index b7d0012794..4c5ad08805 100644
> --- a/hw/misc/mac_via.c
> +++ b/hw/misc/mac_via.c
> @@ -868,24 +868,24 @@ static void mac_via_reset(DeviceState *dev)
> static void mac_via_realize(DeviceState *dev, Error **errp)
> {
> MacVIAState *m = MAC_VIA(dev);
> -MOS6522State *ms;
> struct tm tm;
> int ret;
> +Error *err = NULL;
>
> -/* Init VIAs 1 and 2 */
> -sysbus_init_child_obj(OBJECT(dev), "via1", >mos6522_via1,
> - sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1);
> +qdev_set_parent_bus(DEVICE(>mos6522_via1), sysbus_get_default());
> +qdev_set_parent_bus(DEVICE(>mos6522_via2), sysbus_get_default());
>
> -sysbus_init_child_obj(OBJECT(dev), "via2", >mos6522_via2,
> - sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2);
> +object_property_set_bool(OBJECT(>mos6522_via1), true, "realized",
> );
> +if (err != NULL) {
> +error_propagate(errp, err);
> +return;
> +}
>
> -/* Pass through mos6522 output IRQs */
> -ms = MOS6522(>mos6522_via1);
> -object_property_add_alias(OBJECT(dev), "irq[0]", OBJECT(ms),
> - SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
> -ms = MOS6522(>mos6522_via2);
> -object_property_add_alias(OBJECT(dev), "irq[1]", OBJECT(ms),
> - SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
> +object_property_set_bool(OBJECT(>mos6522_via2), true, "realized",
> );
> +if (err != NULL) {
> +error_propagate(errp, err);
> +return;
> +}
>
> /* Pass through mos6522 input IRQs */
> qdev_pass_gpios(DEVICE(>mos6522_via1), dev, "via1-irq");
> @@ -932,6 +932,7 @@ static void mac_via_init(Object *obj)
> {
> SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> MacVIAState *m = MAC_VIA(obj);
> +MOS6522State *ms;
>
> /* MMIO */
> memory_region_init(>mmio, obj, "mac-via", 2 * VIA_SIZE);
> @@ -948,6 +949,22 @@ static void mac_via_init(Object *obj)
> /* ADB */
> qbus_create_inplace((BusState *)>adb_bus, sizeof(m->adb_bus),
> TYPE_ADB_BUS, DEVICE(obj), "adb.0");
> +
> +/* Init VIAs 1 and 2 */
> +object_initialize_child(OBJECT(m), "via1", >mos6522_via1,
Sorry, one more space at the end of the above line, and fail to run checkpatch.
Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via
On 3/5/2020 2:54 PM, Pan Nengyuan wrote:
> This patch fix a bug in mac_via where it failed to actually realize devices
> it was using.
> And move the init codes which inits the mos6522 objects and properties on
> them from realize()
> into init(). However, we keep qdev_set_parent_bus in realize(), otherwise it
> will cause
> device-introspect-test test fail. Then do the realize mos6522 device in the
> mac_vir_realize.
>
> Signed-off-by: Pan Nengyuan
> ---
> Cc: Laurent Vivier
> Cc: Mark Cave-Ayland
> ---
> v4->v3:
> - split v3 into two patches, this patch fix incorrect creation of mos6522,
> move inits and props
> from realize into init. The v3 is:
> https://patchwork.kernel.org/patch/11407635/
> ---
> hw/misc/mac_via.c | 43 ++-
> 1 file changed, 30 insertions(+), 13 deletions(-)
>
> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
> index b7d0012794..4c5ad08805 100644
> --- a/hw/misc/mac_via.c
> +++ b/hw/misc/mac_via.c
> @@ -868,24 +868,24 @@ static void mac_via_reset(DeviceState *dev)
> static void mac_via_realize(DeviceState *dev, Error **errp)
> {
> MacVIAState *m = MAC_VIA(dev);
> -MOS6522State *ms;
> struct tm tm;
> int ret;
> +Error *err = NULL;
>
> -/* Init VIAs 1 and 2 */
> -sysbus_init_child_obj(OBJECT(dev), "via1", >mos6522_via1,
> - sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1);
> +qdev_set_parent_bus(DEVICE(>mos6522_via1), sysbus_get_default());
> +qdev_set_parent_bus(DEVICE(>mos6522_via2), sysbus_get_default());
>
> -sysbus_init_child_obj(OBJECT(dev), "via2", >mos6522_via2,
> - sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2);
> +object_property_set_bool(OBJECT(>mos6522_via1), true, "realized",
> );
> +if (err != NULL) {
> +error_propagate(errp, err);
> +return;
> +}
>
> -/* Pass through mos6522 output IRQs */
> -ms = MOS6522(>mos6522_via1);
> -object_property_add_alias(OBJECT(dev), "irq[0]", OBJECT(ms),
> - SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
> -ms = MOS6522(>mos6522_via2);
> -object_property_add_alias(OBJECT(dev), "irq[1]", OBJECT(ms),
> - SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
> +object_property_set_bool(OBJECT(>mos6522_via2), true, "realized",
> );
> +if (err != NULL) {
> +error_propagate(errp, err);
> +return;
> +}
>
> /* Pass through mos6522 input IRQs */
> qdev_pass_gpios(DEVICE(>mos6522_via1), dev, "via1-irq");
> @@ -932,6 +932,7 @@ static void mac_via_init(Object *obj)
> {
> SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> MacVIAState *m = MAC_VIA(obj);
> +MOS6522State *ms;
>
> /* MMIO */
> memory_region_init(>mmio, obj, "mac-via", 2 * VIA_SIZE);
> @@ -948,6 +949,22 @@ static void mac_via_init(Object *obj)
> /* ADB */
> qbus_create_inplace((BusState *)>adb_bus, sizeof(m->adb_bus),
> TYPE_ADB_BUS, DEVICE(obj), "adb.0");
> +
> +/* Init VIAs 1 and 2 */
> +object_initialize_child(OBJECT(m), "via1", >mos6522_via1,Sorry, one
> more space at the end of the above line, and fail to run checkpatch.
> +sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1,
> +_abort, NULL);
> +object_initialize_child(OBJECT(m), "via2", >mos6522_via2,
> +sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2,
> +_abort, NULL);
> +
> +/* Pass through mos6522 output IRQs */
> +ms = MOS6522(>mos6522_via1);
> +object_property_add_alias(OBJECT(m), "irq[0]", OBJECT(ms),
> + SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
> +ms = MOS6522(>mos6522_via2);
> +object_property_add_alias(OBJECT(m), "irq[1]", OBJECT(ms),
> + SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
> }
>
> static void postload_update_cb(void *opaque, int running, RunState state)
>
[PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via
This patch fix a bug in mac_via where it failed to actually realize devices it
was using.
And move the init codes which inits the mos6522 objects and properties on them
from realize()
into init(). However, we keep qdev_set_parent_bus in realize(), otherwise it
will cause
device-introspect-test test fail. Then do the realize mos6522 device in the
mac_vir_realize.
Signed-off-by: Pan Nengyuan
---
Cc: Laurent Vivier
Cc: Mark Cave-Ayland
---
v4->v3:
- split v3 into two patches, this patch fix incorrect creation of mos6522, move
inits and props
from realize into init. The v3 is:
https://patchwork.kernel.org/patch/11407635/
---
hw/misc/mac_via.c | 43 ++-
1 file changed, 30 insertions(+), 13 deletions(-)
diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
index b7d0012794..4c5ad08805 100644
--- a/hw/misc/mac_via.c
+++ b/hw/misc/mac_via.c
@@ -868,24 +868,24 @@ static void mac_via_reset(DeviceState *dev)
static void mac_via_realize(DeviceState *dev, Error **errp)
{
MacVIAState *m = MAC_VIA(dev);
-MOS6522State *ms;
struct tm tm;
int ret;
+Error *err = NULL;
-/* Init VIAs 1 and 2 */
-sysbus_init_child_obj(OBJECT(dev), "via1", >mos6522_via1,
- sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1);
+qdev_set_parent_bus(DEVICE(>mos6522_via1), sysbus_get_default());
+qdev_set_parent_bus(DEVICE(>mos6522_via2), sysbus_get_default());
-sysbus_init_child_obj(OBJECT(dev), "via2", >mos6522_via2,
- sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2);
+object_property_set_bool(OBJECT(>mos6522_via1), true, "realized", );
+if (err != NULL) {
+error_propagate(errp, err);
+return;
+}
-/* Pass through mos6522 output IRQs */
-ms = MOS6522(>mos6522_via1);
-object_property_add_alias(OBJECT(dev), "irq[0]", OBJECT(ms),
- SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
-ms = MOS6522(>mos6522_via2);
-object_property_add_alias(OBJECT(dev), "irq[1]", OBJECT(ms),
- SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
+object_property_set_bool(OBJECT(>mos6522_via2), true, "realized", );
+if (err != NULL) {
+error_propagate(errp, err);
+return;
+}
/* Pass through mos6522 input IRQs */
qdev_pass_gpios(DEVICE(>mos6522_via1), dev, "via1-irq");
@@ -932,6 +932,7 @@ static void mac_via_init(Object *obj)
{
SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
MacVIAState *m = MAC_VIA(obj);
+MOS6522State *ms;
/* MMIO */
memory_region_init(>mmio, obj, "mac-via", 2 * VIA_SIZE);
@@ -948,6 +949,22 @@ static void mac_via_init(Object *obj)
/* ADB */
qbus_create_inplace((BusState *)>adb_bus, sizeof(m->adb_bus),
TYPE_ADB_BUS, DEVICE(obj), "adb.0");
+
+/* Init VIAs 1 and 2 */
+object_initialize_child(OBJECT(m), "via1", >mos6522_via1,
+sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1,
+_abort, NULL);
+object_initialize_child(OBJECT(m), "via2", >mos6522_via2,
+sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2,
+_abort, NULL);
+
+/* Pass through mos6522 output IRQs */
+ms = MOS6522(>mos6522_via1);
+object_property_add_alias(OBJECT(m), "irq[0]", OBJECT(ms),
+ SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
+ms = MOS6522(>mos6522_via2);
+object_property_add_alias(OBJECT(m), "irq[1]", OBJECT(ms),
+ SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
}
static void postload_update_cb(void *opaque, int running, RunState state)
--
2.18.2
[PATCH v4 3/3] hw/misc/mos6522: move timer_new from init() into realize() to avoid memleaks
There are some memleaks when we call 'device_list_properties'. This patch move
timer_new from init into realize to fix it.
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
---
Cc: Laurent Vivier
Cc: Mark Cave-Ayland
Cc: David Gibson
Cc: qemu-...@nongnu.org
---
v2->v1:
- no changes in this patch.
v3->v2:
- remove null check in reset, and add calls to mos6522_realize() in
mac_via_realize to make this move to be valid.
v4->v3:
- split patch into two, this patch fix the memleaks.
---
hw/misc/mos6522.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index 19e154b870..c1cd154a84 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -485,6 +485,11 @@ static void mos6522_init(Object *obj)
for (i = 0; i < ARRAY_SIZE(s->timers); i++) {
s->timers[i].index = i;
}
+}
+
+static void mos6522_realize(DeviceState *dev, Error **errp)
+{
+MOS6522State *s = MOS6522(dev);
s->timers[0].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer1, s);
s->timers[1].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer2, s);
@@ -502,6 +507,7 @@ static void mos6522_class_init(ObjectClass *oc, void *data)
dc->reset = mos6522_reset;
dc->vmsd = _mos6522;
+dc->realize = mos6522_realize;
device_class_set_props(dc, mos6522_properties);
mdc->parent_reset = dc->reset;
mdc->set_sr_int = mos6522_set_sr_int;
--
2.18.2
[PATCH v4 1/3] s390x: fix memleaks in cpu_finalize
This patch fix memleaks when we call tests/qtest/cpu-plug-test on s390x. The
leak stack is as follow:
Direct leak of 48 byte(s) in 1 object(s) allocated from:
#0 0x7fb43c7cd970 in __interceptor_calloc (/lib64/libasan.so.5+0xef970)
#1 0x7fb43be2149d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5249d)
#2 0x558ba96da716 in timer_new_full
/mnt/sdb/qemu-new/qemu/include/qemu/timer.h:530
#3 0x558ba96da716 in timer_new
/mnt/sdb/qemu-new/qemu/include/qemu/timer.h:551
#4 0x558ba96da716 in timer_new_ns
/mnt/sdb/qemu-new/qemu/include/qemu/timer.h:569
#5 0x558ba96da716 in s390_cpu_initfn
/mnt/sdb/qemu-new/qemu/target/s390x/cpu.c:285
#6 0x558ba9c969ab in object_init_with_type
/mnt/sdb/qemu-new/qemu/qom/object.c:372
#7 0x558ba9c9eb5f in object_initialize_with_type
/mnt/sdb/qemu-new/qemu/qom/object.c:516
#8 0x558ba9c9f053 in object_new_with_type
/mnt/sdb/qemu-new/qemu/qom/object.c:684
#9 0x558ba967ede6 in s390x_new_cpu
/mnt/sdb/qemu-new/qemu/hw/s390x/s390-virtio-ccw.c:64
#10 0x558ba99764b3 in hmp_cpu_add
/mnt/sdb/qemu-new/qemu/hw/core/machine-hmp-cmds.c:57
#11 0x558ba9b1c27f in handle_hmp_command
/mnt/sdb/qemu-new/qemu/monitor/hmp.c:1082
#12 0x558ba96c1b02 in qmp_human_monitor_command
/mnt/sdb/qemu-new/qemu/monitor/misc.c:142
Reported-by: Euler Robot
Signed-off-by: Pan Nengyuan
---
Cc: Richard Henderson
Cc: David Hildenbrand
Cc: Cornelia Huck
Cc: qemu-s3...@nongnu.org
---
v2->v1:
- Similarly to other cleanups, move timer_new into realize(Suggested by
Philippe Mathieu-Daudé)
v3->v2:
- Aslo do the timer_free in unrealize, it seems balanced.
v4->v3:
- Aslo do timer_free on the error path in realize() and fix some coding style.
- Use device_class_set_parent_unrealize to declare unrealize.
---
target/s390x/cpu-qom.h | 1 +
target/s390x/cpu.c | 41 +
2 files changed, 38 insertions(+), 4 deletions(-)
diff --git a/target/s390x/cpu-qom.h b/target/s390x/cpu-qom.h
index dbe5346ec9..af9ffed0d8 100644
--- a/target/s390x/cpu-qom.h
+++ b/target/s390x/cpu-qom.h
@@ -61,6 +61,7 @@ typedef struct S390CPUClass {
const char *desc;
DeviceRealize parent_realize;
+DeviceUnrealize parent_unrealize;
void (*parent_reset)(CPUState *cpu);
void (*load_normal)(CPUState *cpu);
void (*reset)(CPUState *cpu, cpu_reset_type type);
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index cf84d307c6..80b987ff1b 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -182,6 +182,12 @@ static void s390_cpu_realizefn(DeviceState *dev, Error
**errp)
#if !defined(CONFIG_USER_ONLY)
MachineState *ms = MACHINE(qdev_get_machine());
unsigned int max_cpus = ms->smp.max_cpus;
+
+cpu->env.tod_timer =
+timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_tod_timer, cpu);
+cpu->env.cpu_timer =
+timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_cpu_timer, cpu);
+
if (cpu->env.core_id >= max_cpus) {
error_setg(, "Unable to add CPU with core-id: %" PRIu32
", maximum core-id: %d", cpu->env.core_id,
@@ -224,9 +230,38 @@ static void s390_cpu_realizefn(DeviceState *dev, Error
**errp)
scc->parent_realize(dev, );
out:
+if (cpu->env.tod_timer) {
+timer_del(cpu->env.tod_timer);
+}
+if (cpu->env.cpu_timer) {
+timer_del(cpu->env.cpu_timer);
+}
+timer_free(cpu->env.tod_timer);
+timer_free(cpu->env.cpu_timer);
error_propagate(errp, err);
}
+static void s390_cpu_unrealizefn(DeviceState *dev, Error **errp)
+{
+S390CPUClass *scc = S390_CPU_GET_CLASS(dev);
+Error *err = NULL;
+
+#if !defined(CONFIG_USER_ONLY)
+S390CPU *cpu = S390_CPU(dev);
+
+timer_del(cpu->env.tod_timer);
+timer_del(cpu->env.cpu_timer);
+timer_free(cpu->env.tod_timer);
+timer_free(cpu->env.cpu_timer);
+#endif
+
+scc->parent_unrealize(dev, );
+if (err != NULL) {
+error_propagate(errp, err);
+return;
+}
+}
+
static GuestPanicInformation *s390_cpu_get_crash_info(CPUState *cs)
{
GuestPanicInformation *panic_info;
@@ -279,10 +314,6 @@ static void s390_cpu_initfn(Object *obj)
s390_cpu_get_crash_info_qom, NULL, NULL, NULL, NULL);
s390_cpu_model_register_props(obj);
#if !defined(CONFIG_USER_ONLY)
-cpu->env.tod_timer =
-timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_tod_timer, cpu);
-cpu->env.cpu_timer =
-timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_cpu_timer, cpu);
s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
#endif
}
@@ -453,6 +484,8 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
device_class_set_parent_realize(dc, s390_cpu_realizefn,
>parent_realize);
+device_class_set_parent_unrealize(dc, s390_cpu_unrealizefn,
+ >parent_unrealize);
device_class_set_props(dc, s390x_cpu_properties);
dc->user_creatable = true;
--
2.18.2
