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

[Marc-André Lureau]

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!
>