Re: [PATCH] qemu-config: never call the callback after an error, fix leak

[Markus Armbruster <armbru@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Ensure that the callback to qemu_config_foreach is never called upon
> an error, by moving the invocation before the "out" label and ensuring
> all error cases jump to the label.  The qobject_unref however needs
> to be done in all cases (which Coverity is already complaining about).
>
> The leak is basically impossible to reach, since the only common way
> to get ferror(fp) is by passing a directory to -readconfig.  In that
> case, the error occurs before qdict is set to anything non-NULL.
> However, it's theoretically possible to get there after an EIO.
>
> Cc: armbru@redhat.com
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  util/qemu-config.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/util/qemu-config.c b/util/qemu-config.c
> index 84ee6dc4ea..6c4373e8fb 100644
> --- a/util/qemu-config.c
> +++ b/util/qemu-config.c
> @@ -412,16 +412,15 @@ static int qemu_config_foreach(FILE *fp, QEMUConfigCB *cb, void *opaque,
>          goto out;
>      }
>      if (ferror(fp)) {
> -        loc_pop(&loc);
>          error_setg_errno(errp, errno, "Cannot read config file");

I'm afraid we now report the error with the wrong location when @errp is
&error-fatal.

> -        return res;
> +        goto out;
>      }
>      res = count;
> -out:
>      if (qdict) {
>          cb(group, qdict, opaque, errp);
> -        qobject_unref(qdict);
>      }
> +out:
> +    qobject_unref(qdict);
>      loc_pop(&loc);
>      return res;
>  }

Looks like the patch fixes two separate issues:

1. Memory leak on ferror()
   Fixes: f7544edcd32e602af1aae86714dc7c32350d5d7c

2. Callback can run on error.
   Fixes: 37701411397c7b7d709ae92abd347cc593940ee5

   I *think* this happens when the cb() further up fails, and when a
   line following the [...] section header cannot be parsed.

Worth fixing the separate bugs in separate patches?