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

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

[Paolo Bonzini]

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");
-        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;
 }
-- 
2.31.1

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

[Markus Armbruster]

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?

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

[Paolo Bonzini]

On 08/07/21 11:24, Markus Armbruster wrote:
> 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?

Yes, good idea.

Paolo