thr3ads.net - Libguestfs - [Libguestfs] [libnbd PATCH] RFC: generator: Mark APIs with allocator/deallocator semantics [Sep 2022]

If this information is useful, please help other people find it:
Share via:

Eric Blake

2022-Sep-30 15:42 UTC

[Libguestfs] [libnbd PATCH] RFC: generator: Mark APIs with allocator/deallocator semantics

Modern GCC has two related attributes for functions returning a
pointer:

__attribute__((__malloc__)) - this function returns a new pointer, not
aliased to any existing pointer

__attribute__((__malloc__(fn,1))) - call fn(return_value) to avoid
leaking memory allocated by this function

With those attributes, static analyzers can better detect when we pass
the resulting pointer to the wrong deallocator, deallocate more than
once, have a use after free, or otherwise leak the memory.  (Sadly, as
of gcc 12.2.1, -fanalyzer still has a LOT of false positives, such as:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107100; since our code
base triggers some of these, we can't quite rely on it yet).
---
 lib/internal.h |  4 +++-
 generator/C.ml | 24 +++++++++++++++++++++---
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/lib/internal.h b/lib/internal.h
index 0e17fbea..22f500b4 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -508,8 +508,10 @@ extern void nbd_internal_fork_safe_perror (const char *s)
 extern char *nbd_internal_printable_buffer (const void *buf, size_t count)
   LIBNBD_ATTRIBUTE_NONNULL(1);
 extern char *nbd_internal_printable_string (const char *str)
+  LIBNBD_ATTRIBUTE_ALLOC_DEALLOC(free)
   LIBNBD_ATTRIBUTE_NONNULL(1);
-extern char *nbd_internal_printable_string_list (char **list);
+extern char *nbd_internal_printable_string_list (char **list)
+  LIBNBD_ATTRIBUTE_ALLOC_DEALLOC(free);

 /* These are wrappers around socket(2) and socketpair(2).  They
  * always set SOCK_CLOEXEC.  nbd_internal_socket can set SOCK_NONBLOCK
diff --git a/generator/C.ml b/generator/C.ml
index c74fae77..cfa2964c 100644
--- a/generator/C.ml
+++ b/generator/C.ml
@@ -244,6 +244,16 @@ let
   pr "extern ";
   print_call ?wrap ?closure_style name args optargs ret;

+  (* For RString, note that the caller must free() the argument.
+   * Since this is used in a public header, we must use gcc's spelling
+   * of __builtin_free in case bare free is defined as a macro.
+   *)
+  (match ret with
+   | RString ->
+      pr "\n    LIBNBD_ATTRIBUTE_ALLOC_DEALLOC(__builtin_free)";
+   | _ -> ()
+  );
+
   (* Output __attribute__((nonnull)) for the function parameters:
    * eg. struct nbd_handle *, int, char *
    *     => [ true, false, true ]
@@ -403,6 +413,13 @@ let
   pr "#endif\n";
   pr "#endif /* ! defined LIBNBD_ATTRIBUTE_NONNULL */\n";
   pr "\n";
+  pr "#if defined(__GNUC__) && LIBNBD_GCC_VERSION >= 120000 /*
gcc >= 12.0 */\n";
+  pr "#define LIBNBD_ATTRIBUTE_ALLOC_DEALLOC(fn) \\\n";
+  pr "  __attribute__((__malloc__, __malloc__(fn, 1)))\n";
+  pr "#else\n";
+  pr "#define LIBNBD_ATTRIBUTE_ALLOC_DEALLOC(fn)\n";
+  pr "#endif\n";
+  pr "\n";
   pr "struct nbd_handle;\n";
   pr "\n";
   List.iter (
@@ -433,12 +450,13 @@ let
       pr "#define %-40s %d\n" n i
   ) constants;
   pr "\n";
-  pr "extern struct nbd_handle *nbd_create (void);\n";
-  pr "#define LIBNBD_HAVE_NBD_CREATE 1\n";
-  pr "\n";
   pr "extern void nbd_close (struct nbd_handle *h); /* h can be NULL
*/\n";
   pr "#define LIBNBD_HAVE_NBD_CLOSE 1\n";
   pr "\n";
+  pr "extern struct nbd_handle *nbd_create (void)\n";
+  pr "    LIBNBD_ATTRIBUTE_ALLOC_DEALLOC(nbd_close);\n";
+  pr "#define LIBNBD_HAVE_NBD_CREATE 1\n";
+  pr "\n";
   pr "extern const char *nbd_get_error (void);\n";
   pr "#define LIBNBD_HAVE_NBD_GET_ERROR 1\n";
   pr "\n";
-- 
2.37.3

Richard W.M. Jones

2022-Sep-30 16:27 UTC

head link

[Libguestfs] [libnbd PATCH] RFC: generator: Mark APIs with allocator/deallocator semantics

On Fri, Sep 30, 2022 at 10:42:01AM -0500, Eric Blake
wrote:> Modern GCC has two related attributes for functions returning a
> pointer:
> 
> __attribute__((__malloc__)) - this function returns a new pointer, not
> aliased to any existing pointer
> 
> __attribute__((__malloc__(fn,1))) - call fn(return_value) to avoid
> leaking memory allocated by this function
> 
> With those attributes, static analyzers can better detect when we pass
> the resulting pointer to the wrong deallocator, deallocate more than
> once, have a use after free, or otherwise leak the memory.  (Sadly, as
> of gcc 12.2.1, -fanalyzer still has a LOT of false positives, such as:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107100; since our code
> base triggers some of these, we can't quite rely on it yet).
> ---
>  lib/internal.h |  4 +++-
>  generator/C.ml | 24 +++++++++++++++++++++---
>  2 files changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/internal.h b/lib/internal.h
> index 0e17fbea..22f500b4 100644
> --- a/lib/internal.h
> +++ b/lib/internal.h
> @@ -508,8 +508,10 @@ extern void nbd_internal_fork_safe_perror (const char
*s)
>  extern char *nbd_internal_printable_buffer (const void *buf, size_t count)
>    LIBNBD_ATTRIBUTE_NONNULL(1);
>  extern char *nbd_internal_printable_string (const char *str)
> +  LIBNBD_ATTRIBUTE_ALLOC_DEALLOC(free)
>    LIBNBD_ATTRIBUTE_NONNULL(1);
> -extern char *nbd_internal_printable_string_list (char **list);
> +extern char *nbd_internal_printable_string_list (char **list)
> +  LIBNBD_ATTRIBUTE_ALLOC_DEALLOC(free);
> 
>  /* These are wrappers around socket(2) and socketpair(2).  They
>   * always set SOCK_CLOEXEC.  nbd_internal_socket can set SOCK_NONBLOCK
> diff --git a/generator/C.ml b/generator/C.ml
> index c74fae77..cfa2964c 100644
> --- a/generator/C.ml
> +++ b/generator/C.ml
> @@ -244,6 +244,16 @@ let
>    pr "extern ";
>    print_call ?wrap ?closure_style name args optargs ret;
> 
> +  (* For RString, note that the caller must free() the argument.
> +   * Since this is used in a public header, we must use gcc's spelling
> +   * of __builtin_free in case bare free is defined as a macro.
> +   *)
> +  (match ret with
> +   | RString ->
> +      pr "\n    LIBNBD_ATTRIBUTE_ALLOC_DEALLOC(__builtin_free)";
> +   | _ -> ()
> +  );
> +
>    (* Output __attribute__((nonnull)) for the function parameters:
>     * eg. struct nbd_handle *, int, char *
>     *     => [ true, false, true ]
> @@ -403,6 +413,13 @@ let
>    pr "#endif\n";
>    pr "#endif /* ! defined LIBNBD_ATTRIBUTE_NONNULL */\n";
>    pr "\n";
> +  pr "#if defined(__GNUC__) && LIBNBD_GCC_VERSION >=
120000 /* gcc >= 12.0 */\n";
> +  pr "#define LIBNBD_ATTRIBUTE_ALLOC_DEALLOC(fn) \\\n";
> +  pr "  __attribute__((__malloc__, __malloc__(fn, 1)))\n";
> +  pr "#else\n";
> +  pr "#define LIBNBD_ATTRIBUTE_ALLOC_DEALLOC(fn)\n";
> +  pr "#endif\n";
> +  pr "\n";
>    pr "struct nbd_handle;\n";
>    pr "\n";
>    List.iter (
> @@ -433,12 +450,13 @@ let
>        pr "#define %-40s %d\n" n i
>    ) constants;
>    pr "\n";
> -  pr "extern struct nbd_handle *nbd_create (void);\n";
> -  pr "#define LIBNBD_HAVE_NBD_CREATE 1\n";
> -  pr "\n";
>    pr "extern void nbd_close (struct nbd_handle *h); /* h can be NULL
*/\n";
>    pr "#define LIBNBD_HAVE_NBD_CLOSE 1\n";
>    pr "\n";
> +  pr "extern struct nbd_handle *nbd_create (void)\n";
> +  pr "    LIBNBD_ATTRIBUTE_ALLOC_DEALLOC(nbd_close);\n";
> +  pr "#define LIBNBD_HAVE_NBD_CREATE 1\n";
> +  pr "\n";
>    pr "extern const char *nbd_get_error (void);\n";
>    pr "#define LIBNBD_HAVE_NBD_GET_ERROR 1\n";
>    pr "\n";
ACK - worth a go, if it causes too many problems we can always
back it out later!

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

Libguestfs - Sep 2022 - [libnbd PATCH] RFC: generator: Mark APIs with allocator/deallocator semantics

[Libguestfs] [libnbd PATCH] RFC: generator: Mark APIs with allocator/deallocator semantics

[Libguestfs] [libnbd PATCH] RFC: generator: Mark APIs with allocator/deallocator semantics