Re: [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";