[v2] linux-user: fix resource leaks in gen-vdso

[PATCH v2] linux-user: fix resource leaks in gen-vdso

Posted by Daniel P. Berrangé 6 months ago

There are a number of resource leaks in gen-vdso. In theory they are
harmless because this is a short lived process, but when building QEMU
with --extra-cflags="-fsanitize=address" problems ensure. The gen-vdso
program is run as part of the build, and that aborts due to the
sanitizer identifying memory leaks, leaving QEMU unbuildable.

FAILED: libqemu-x86_64-linux-user.a.p/vdso.c.inc
/var/home/berrange/src/virt/qemu/build/linux-user/gen-vdso -o libqemu-x86_64-linux-user.a.p/vdso.c.inc ../linux-user/x86_64/vdso.so

=================================================================
==1696332==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 2968 byte(s) in 1 object(s) allocated from:
    #0 0x56495873f1f3  (/var/home/berrange/src/virt/qemu/build/linux-user/gen-vdso+0xa11f3) (BuildId: b69e241ad44719b6f3934f3c71dfc6727e8bdb12)
    #1 0x564958780b90  (/var/home/berrange/src/virt/qemu/build/linux-user/gen-vdso+0xe2b90) (BuildId: b69e241ad44719b6f3934f3c71dfc6727e8bdb12)

This complaint is about the 'buf' variable, however, the FILE objects
are also leaked in some error scenarios, so this fix refactors the
cleanup paths to fix all leaks. For completeness it also reports an
error if fclose() fails on 'inf'.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---

Changed in v2:

 - Add missing braces

 linux-user/gen-vdso.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/linux-user/gen-vdso.c b/linux-user/gen-vdso.c
index 721f38d5a3..fce9d5cbc3 100644
--- a/linux-user/gen-vdso.c
+++ b/linux-user/gen-vdso.c
@@ -56,13 +56,14 @@ static unsigned rt_sigreturn_addr;
 
 int main(int argc, char **argv)
 {
-    FILE *inf, *outf;
+    FILE *inf = NULL, *outf = NULL;
     long total_len;
     const char *prefix = "vdso";
     const char *inf_name;
     const char *outf_name = NULL;
-    unsigned char *buf;
+    unsigned char *buf = NULL;
     bool need_bswap;
+    int ret = EXIT_FAILURE;
 
     while (1) {
         int opt = getopt(argc, argv, "o:p:r:s:");
@@ -129,7 +130,6 @@ int main(int argc, char **argv)
         fprintf(stderr, "%s: incomplete read\n", inf_name);
         return EXIT_FAILURE;
     }
-    fclose(inf);
 
     /*
      * Identify which elf flavor we're processing.
@@ -205,19 +205,24 @@ int main(int argc, char **argv)
     fprintf(outf, "    .rt_sigreturn_ofs = 0x%x,\n", rt_sigreturn_addr);
     fprintf(outf, "};\n");
 
-    /*
-     * Everything should have gone well.
-     */
-    if (fclose(outf)) {
-        goto perror_outf;
+    ret = EXIT_SUCCESS;
+
+ cleanup:
+    free(buf);
+
+    if (outf && fclose(outf) != 0) {
+        ret = EXIT_FAILURE;
+    }
+    if (inf && fclose(inf) != 0) {
+        ret = EXIT_FAILURE;
     }
-    return EXIT_SUCCESS;
+    return ret;
 
  perror_inf:
     perror(inf_name);
-    return EXIT_FAILURE;
+    goto cleanup;
 
  perror_outf:
     perror(outf_name);
-    return EXIT_FAILURE;
+    goto cleanup;
 }
-- 
2.49.0

Re: [PATCH v2] linux-user: fix resource leaks in gen-vdso

Posted by Richard Henderson 4 months, 3 weeks ago

On 5/13/25 08:03, Daniel P. Berrangé wrote:
> There are a number of resource leaks in gen-vdso. In theory they are
> harmless because this is a short lived process, but when building QEMU
> with --extra-cflags="-fsanitize=address" problems ensure. The gen-vdso
> program is run as part of the build, and that aborts due to the
> sanitizer identifying memory leaks, leaving QEMU unbuildable.
> 
> FAILED: libqemu-x86_64-linux-user.a.p/vdso.c.inc
> /var/home/berrange/src/virt/qemu/build/linux-user/gen-vdso -o libqemu-x86_64-linux-user.a.p/vdso.c.inc ../linux-user/x86_64/vdso.so
> 
> =================================================================
> ==1696332==ERROR: LeakSanitizer: detected memory leaks
> 
> Direct leak of 2968 byte(s) in 1 object(s) allocated from:
>      #0 0x56495873f1f3  (/var/home/berrange/src/virt/qemu/build/linux-user/gen-vdso+0xa11f3) (BuildId: b69e241ad44719b6f3934f3c71dfc6727e8bdb12)
>      #1 0x564958780b90  (/var/home/berrange/src/virt/qemu/build/linux-user/gen-vdso+0xe2b90) (BuildId: b69e241ad44719b6f3934f3c71dfc6727e8bdb12)
> 
> This complaint is about the 'buf' variable, however, the FILE objects
> are also leaked in some error scenarios, so this fix refactors the
> cleanup paths to fix all leaks. For completeness it also reports an
> error if fclose() fails on 'inf'.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---

Queued, thanks.


r~

Re: [PATCH v2] linux-user: fix resource leaks in gen-vdso

Posted by Arusekk 5 months ago

Tested-by: Arusekk <floss@arusekk.pl>

Thanks for the patch. I am just a drive-by contributor, not someone
experienced in qemu development, so take my feedback with a grain of salt.

> There are a number of resource leaks in gen-vdso. In theory they are
> harmless because this is a short lived process, but when building QEMU
> with --extra-cflags="-fsanitize=address" problems ensure. The gen-vdso
> program is run as part of the build, and that aborts due to the
> sanitizer identifying memory leaks, leaving QEMU unbuildable.

I have not encountered it personally before.
However, I can confirm that qemu fails to build using

./configure --target-list=x86_64-linux-user --extra-cflags=-fsanitize=address && make

on current master, and that the patch fixes it.

I use gcc (Gentoo Hardened 15.1.0 p1) 15.1.0 libasan.so.8.
Curiously, the leak does not happen e.g. for arm-linux-user target.

> This complaint is about the 'buf' variable, however, the FILE objects
> are also leaked in some error scenarios, so this fix refactors the
> cleanup paths to fix all leaks. For completeness it also reports an
> error if fclose() fails on 'inf'.

How about other error cases?

> diff --git a/linux-user/gen-vdso.c b/linux-user/gen-vdso.c
> index 721f38d5a3..fce9d5cbc3 100644
> --- a/linux-user/gen-vdso.c
> +++ b/linux-user/gen-vdso.c
> @@ -129,7 +130,6 @@ int main(int argc, char **argv)
>          fprintf(stderr, "%s: incomplete read\n", inf_name);
>          return EXIT_FAILURE;

Like this one. Are other places like here possible candidates to also goto cleanup?

- Arusekk <https://arusekk.pl>

Re: [PATCH v2] linux-user: fix resource leaks in gen-vdso

Posted by Daniel P. Berrangé 5 months, 1 week ago

Ping for review/acks

On Tue, May 13, 2025 at 04:03:46PM +0100, Daniel P. Berrangé wrote:
> There are a number of resource leaks in gen-vdso. In theory they are
> harmless because this is a short lived process, but when building QEMU
> with --extra-cflags="-fsanitize=address" problems ensure. The gen-vdso
> program is run as part of the build, and that aborts due to the
> sanitizer identifying memory leaks, leaving QEMU unbuildable.
> 
> FAILED: libqemu-x86_64-linux-user.a.p/vdso.c.inc
> /var/home/berrange/src/virt/qemu/build/linux-user/gen-vdso -o libqemu-x86_64-linux-user.a.p/vdso.c.inc ../linux-user/x86_64/vdso.so
> 
> =================================================================
> ==1696332==ERROR: LeakSanitizer: detected memory leaks
> 
> Direct leak of 2968 byte(s) in 1 object(s) allocated from:
>     #0 0x56495873f1f3  (/var/home/berrange/src/virt/qemu/build/linux-user/gen-vdso+0xa11f3) (BuildId: b69e241ad44719b6f3934f3c71dfc6727e8bdb12)
>     #1 0x564958780b90  (/var/home/berrange/src/virt/qemu/build/linux-user/gen-vdso+0xe2b90) (BuildId: b69e241ad44719b6f3934f3c71dfc6727e8bdb12)
> 
> This complaint is about the 'buf' variable, however, the FILE objects
> are also leaked in some error scenarios, so this fix refactors the
> cleanup paths to fix all leaks. For completeness it also reports an
> error if fclose() fails on 'inf'.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> 
> Changed in v2:
> 
>  - Add missing braces
> 
>  linux-user/gen-vdso.c | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
> 
> diff --git a/linux-user/gen-vdso.c b/linux-user/gen-vdso.c
> index 721f38d5a3..fce9d5cbc3 100644
> --- a/linux-user/gen-vdso.c
> +++ b/linux-user/gen-vdso.c
> @@ -56,13 +56,14 @@ static unsigned rt_sigreturn_addr;
>  
>  int main(int argc, char **argv)
>  {
> -    FILE *inf, *outf;
> +    FILE *inf = NULL, *outf = NULL;
>      long total_len;
>      const char *prefix = "vdso";
>      const char *inf_name;
>      const char *outf_name = NULL;
> -    unsigned char *buf;
> +    unsigned char *buf = NULL;
>      bool need_bswap;
> +    int ret = EXIT_FAILURE;
>  
>      while (1) {
>          int opt = getopt(argc, argv, "o:p:r:s:");
> @@ -129,7 +130,6 @@ int main(int argc, char **argv)
>          fprintf(stderr, "%s: incomplete read\n", inf_name);
>          return EXIT_FAILURE;
>      }
> -    fclose(inf);
>  
>      /*
>       * Identify which elf flavor we're processing.
> @@ -205,19 +205,24 @@ int main(int argc, char **argv)
>      fprintf(outf, "    .rt_sigreturn_ofs = 0x%x,\n", rt_sigreturn_addr);
>      fprintf(outf, "};\n");
>  
> -    /*
> -     * Everything should have gone well.
> -     */
> -    if (fclose(outf)) {
> -        goto perror_outf;
> +    ret = EXIT_SUCCESS;
> +
> + cleanup:
> +    free(buf);
> +
> +    if (outf && fclose(outf) != 0) {
> +        ret = EXIT_FAILURE;
> +    }
> +    if (inf && fclose(inf) != 0) {
> +        ret = EXIT_FAILURE;
>      }
> -    return EXIT_SUCCESS;
> +    return ret;
>  
>   perror_inf:
>      perror(inf_name);
> -    return EXIT_FAILURE;
> +    goto cleanup;
>  
>   perror_outf:
>      perror(outf_name);
> -    return EXIT_FAILURE;
> +    goto cleanup;
>  }
> -- 
> 2.49.0
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|