Paolo Bonzini <pbonz...@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: arm...@redhat.com
> Reported-by: Peter Maydell <peter.mayd...@linaro.org>
> Signed-off-by: Paolo Bonzini <pbonz...@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?
