Re: [Qemu-trivial] [Qemu-devel] [PATCH] monitor: Fix Resource leak

[Markus Armbruster <armbru@redhat.com>]

<arei.gonglei@huawei.com> writes:

> From: Gonglei <arei.gonglei@huawei.com>
>
> Coverity spot:
> qemu_using_spice allocates memory that is stored into err,
> but not freed before return.
>
> Cc:Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  monitor.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/monitor.c b/monitor.c
> index c3cc060..137d23f 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1095,12 +1095,13 @@ static int client_migrate_info(Monitor *mon, const QDict *qdict,
>      const char *subject  = qdict_get_try_str(qdict, "cert-subject");
>      int port             = qdict_get_try_int(qdict, "port", -1);
>      int tls_port         = qdict_get_try_int(qdict, "tls-port", -1);
> -    Error *err;
> +    Error *err = NULL;
>      int ret;
>  
>      if (strcmp(protocol, "spice") == 0) {
>          if (!qemu_using_spice(&err)) {
>              qerror_report_err(err);
> +            error_free(err);
>              return -1;
>          }

Your commit message is incomplete.  The resource leak is real, but it's
the less serious bug here.  The serious one is missing initialization of
err.

If using_spice, qemu_using_spice() returns true without touching err.
All fine.

Else, if err is null by chance, qemu_using_spice()'s error_set() creates
an error object and stores it in err.  We leak it.

If err is not null, error_set() fails its assertion.

Broken in commit b25d81b by yours truly.  Thanks for cleaning up my
mess!

Let's fix up the commit message:

    monitor: Fix missing err = NULL in client_migrate_info()

    When SPICE isn't used, we either fail an assertion in error_set(),
    or leak an error object.  Broken in commit b25d81b.

With such a fixup:

Reviewed-by: Markus Armbruster <armbru@redhat.com>