1. Patchew
  2. QEMU
  3. [PATCH 0/5] dump: Minor fixes & improvements
  4. View patch

[PATCH 2/5] dump: Fix g_array_unref(NULL) in dump-guest-memory

Markus Armbruster posted 5 patches 1 year ago
Maintainers: "Marc-André Lureau" <marcandre.lureau@redhat.com>
There is a newer version of this series
[PATCH 2/5] dump: Fix g_array_unref(NULL) in dump-guest-memory
Posted by Markus Armbruster 1 year ago 
When dump_init()'s check for non-zero @length fails, dump_cleanup()
passes null s->string_table_buf to g_array_unref(), which spews "GLib:
g_array_unref: assertion 'array' failed" to stderr.

Guard the g_array_unref().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 dump/dump.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/dump/dump.c b/dump/dump.c
index a1fad17f9c..d8ea364af2 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -100,7 +100,9 @@ static int dump_cleanup(DumpState *s)
     memory_mapping_list_free(&s->list);
     close(s->fd);
     g_free(s->guest_note);
-    g_array_unref(s->string_table_buf);
+    if (s->string_table_buf) {
+        g_array_unref(s->string_table_buf);
+    }
     s->guest_note = NULL;
     if (s->resume) {
         if (s->detached) {
-- 
2.41.0
Re: [PATCH 2/5] dump: Fix g_array_unref(NULL) in dump-guest-memory
Posted by Marc-André Lureau 1 year ago 
Hi

On Mon, Oct 30, 2023 at 5:37 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> When dump_init()'s check for non-zero @length fails, dump_cleanup()
> passes null s->string_table_buf to g_array_unref(), which spews "GLib:
> g_array_unref: assertion 'array' failed" to stderr.
>
> Guard the g_array_unref().
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  dump/dump.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index a1fad17f9c..d8ea364af2 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -100,7 +100,9 @@ static int dump_cleanup(DumpState *s)
>      memory_mapping_list_free(&s->list);
>      close(s->fd);
>      g_free(s->guest_note);
> -    g_array_unref(s->string_table_buf);
> +    if (s->string_table_buf) {
> +        g_array_unref(s->string_table_buf);
> +    }

or:
g_clear_pointer(&s->string_table_buf, g_array_unref)

>      s->guest_note = NULL;
>      if (s->resume) {
>          if (s->detached) {
> --
> 2.41.0
>
Re: [PATCH 2/5] dump: Fix g_array_unref(NULL) in dump-guest-memory
Posted by Markus Armbruster 1 year ago 
Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Hi
>
> On Mon, Oct 30, 2023 at 5:37 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> When dump_init()'s check for non-zero @length fails, dump_cleanup()
>> passes null s->string_table_buf to g_array_unref(), which spews "GLib:
>> g_array_unref: assertion 'array' failed" to stderr.
>>
>> Guard the g_array_unref().
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
>> ---
>>  dump/dump.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/dump/dump.c b/dump/dump.c
>> index a1fad17f9c..d8ea364af2 100644
>> --- a/dump/dump.c
>> +++ b/dump/dump.c
>> @@ -100,7 +100,9 @@ static int dump_cleanup(DumpState *s)
>>      memory_mapping_list_free(&s->list);
>>      close(s->fd);
>>      g_free(s->guest_note);
>> -    g_array_unref(s->string_table_buf);
>> +    if (s->string_table_buf) {
>> +        g_array_unref(s->string_table_buf);
>> +    }
>
> or:
> g_clear_pointer(&s->string_table_buf, g_array_unref)

Since dump_cleanup() doesn't clear any of the other members of @s, I'll
stick to g_array_unref() for consistency and simplicity.

>>      s->guest_note = NULL;
>>      if (s->resume) {
>>          if (s->detached) {
>> --
>> 2.41.0
>>

Thanks!
Re: [PATCH 2/5] dump: Fix g_array_unref(NULL) in dump-guest-memory
Posted by Markus Armbruster 1 year ago 
Markus Armbruster <armbru@redhat.com> writes:

> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
>> Hi
>>
>> On Mon, Oct 30, 2023 at 5:37 PM Markus Armbruster <armbru@redhat.com> wrote:
>>>
>>> When dump_init()'s check for non-zero @length fails, dump_cleanup()
>>> passes null s->string_table_buf to g_array_unref(), which spews "GLib:
>>> g_array_unref: assertion 'array' failed" to stderr.
>>>
>>> Guard the g_array_unref().
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>
>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>>> ---
>>>  dump/dump.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/dump/dump.c b/dump/dump.c
>>> index a1fad17f9c..d8ea364af2 100644
>>> --- a/dump/dump.c
>>> +++ b/dump/dump.c
>>> @@ -100,7 +100,9 @@ static int dump_cleanup(DumpState *s)
>>>      memory_mapping_list_free(&s->list);
>>>      close(s->fd);
>>>      g_free(s->guest_note);
>>> -    g_array_unref(s->string_table_buf);
>>> +    if (s->string_table_buf) {
>>> +        g_array_unref(s->string_table_buf);
>>> +    }
>>
>> or:
>> g_clear_pointer(&s->string_table_buf, g_array_unref)
>
> Since dump_cleanup() doesn't clear any of the other members of @s, I'll
> stick to g_array_unref() for consistency and simplicity.

Wait!  You suggest *unconditional*

         g_clear_pointer(&s->string_table_buf, g_array_unref)         

don't you?

Got a preference?

>>>      s->guest_note = NULL;
>>>      if (s->resume) {
>>>          if (s->detached) {
>>> --
>>> 2.41.0
>>>
>
> Thanks!
Re: [PATCH 2/5] dump: Fix g_array_unref(NULL) in dump-guest-memory
Posted by Marc-André Lureau 1 year ago 
Hi

On Tue, Oct 31, 2023 at 1:06 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Markus Armbruster <armbru@redhat.com> writes:
>
> > Marc-André Lureau <marcandre.lureau@redhat.com> writes:
> >
> >> Hi
> >>
> >> On Mon, Oct 30, 2023 at 5:37 PM Markus Armbruster <armbru@redhat.com> wrote:
> >>>
> >>> When dump_init()'s check for non-zero @length fails, dump_cleanup()
> >>> passes null s->string_table_buf to g_array_unref(), which spews "GLib:
> >>> g_array_unref: assertion 'array' failed" to stderr.
> >>>
> >>> Guard the g_array_unref().
> >>>
> >>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >>
> >> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >>
> >>> ---
> >>>  dump/dump.c | 4 +++-
> >>>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/dump/dump.c b/dump/dump.c
> >>> index a1fad17f9c..d8ea364af2 100644
> >>> --- a/dump/dump.c
> >>> +++ b/dump/dump.c
> >>> @@ -100,7 +100,9 @@ static int dump_cleanup(DumpState *s)
> >>>      memory_mapping_list_free(&s->list);
> >>>      close(s->fd);
> >>>      g_free(s->guest_note);
> >>> -    g_array_unref(s->string_table_buf);
> >>> +    if (s->string_table_buf) {
> >>> +        g_array_unref(s->string_table_buf);
> >>> +    }
> >>
> >> or:
> >> g_clear_pointer(&s->string_table_buf, g_array_unref)
> >
> > Since dump_cleanup() doesn't clear any of the other members of @s, I'll
> > stick to g_array_unref() for consistency and simplicity.
>
> Wait!  You suggest *unconditional*
>
>          g_clear_pointer(&s->string_table_buf, g_array_unref)
>
> don't you?
>
> Got a preference?

Yes, I think it's safe and more future proof in general. Up to you if
you respin.

thanks

>
> >>>      s->guest_note = NULL;
> >>>      if (s->resume) {
> >>>          if (s->detached) {
> >>> --
> >>> 2.41.0
> >>>
> >
> > Thanks!
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.