Libguestfs - Sep 2022 - [PATCH libnbd v2 0/4] Additional non-NULL warnings, checks and docs

v1 was:
https://listman.redhat.com/archives/libguestfs/2022-September/029982.html

This series is actually quite a bit different from v1 because it is
rebased on top of Eric's series and takes that into account.

Generally this heads down the return error instead of asserting route,
hoping that the GCC annotations will be sufficient to catch as many
errors as possible at compile time.

Rich.

For API parameters that are pointers and must not be NULL, add the
appropriate GCC annotations.  These are only enabled in very recent
GCC (>= 12) because we have concerns with earlier versions, see for
example: https://bugzilla.redhat.com/show_bug.cgi?id=1041336
---
 generator/C.ml | 53 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 51 insertions(+), 2 deletions(-)

diff --git a/generator/C.ml b/generator/C.ml
index 66498a7d88..cafc5590e2 100644
--- a/generator/C.ml
+++ b/generator/C.ml
@@ -107,6 +107,26 @@ let
   | UInt64 n -> [n]
   | UIntPtr n -> [n]
 
+let arg_attr_nonnull +  function
+  (* BytesIn/Out are passed using a non-null pointer, and size_t *)
+  | BytesIn _
+  | BytesOut _
+  | BytesPersistIn _
+  | BytesPersistOut _ -> [ true; false ]
+  (* sockaddr is also non-null pointer, and length *)
+  | SockAddrAndLen (n, len) -> [ true; false ]
+  (* strings should be marked as non-null *)
+  | Path _ | String _ -> [ true ]
+  (* list of strings should be marked as non-null *)
+  | StringList n -> [ true ]
+  (* other non-pointer types can never be null *)
+  | Bool _ | Closure _ | Enum _ | Fd _ | Flags _
+  | Int _ | Int64 _ | SizeT _
+  | UInt _ | UInt32 _ | UInt64 _ | UIntPtr _ -> [ false ]
+
+let optarg_attr_nonnull (OClosure _ | OFlags _) = [ false ]
+
 let rec print_arg_list ?(wrap = false) ?maxcol ?handle ?types ?(parens = true)
           ?closure_style args optargs    if parens then pr "(";
@@ -216,7 +236,17 @@ let
 let print_fndecl ?wrap ?closure_style name args optargs ret    pr "extern
";
   print_call ?wrap ?closure_style name args optargs ret;
-  pr ";\n"
+
+  (* Non-null attribute. *)
+  let nns +    [ [ true ] ] (* for struct nbd_handle pointer *)
+    @ List.map arg_attr_nonnull args
+    @ List.map optarg_attr_nonnull optargs in
+  let nns : bool list = List.flatten nns in
+  let nns = List.mapi (fun i b -> (i+1, b)) nns in
+  let nns = filter_map (fun (i, b) -> if b then Some i else None) nns in
+  let nns : string list = List.map string_of_int nns in
+  pr "\n    LIBNBD_ATTRIBUTE_NONNULL((%s));\n" (String.concat
"," nns)
 
 let rec print_cbarg_list ?(wrap = false) ?maxcol ?types ?(parens = true)
           cbargs @@ -349,6 +379,19 @@ let
   pr "extern \"C\" {\n";
   pr "#endif\n";
   pr "\n";
+  pr "#if defined(__GNUC__)\n";
+  pr "#define LIBNBD_GCC_VERSION \\\n";
+  pr "    (__GNUC__ * 10000 + __GNUC_MINOR__ * 100 +
__GNUC_PATCHLEVEL__)\n";
+  pr "#endif\n";
+  pr "\n";
+  pr "#ifndef LIBNBD_ATTRIBUTE_NONNULL\n";
+  pr "#if defined(__GNUC__) && LIBNBD_GCC_VERSION >= 120000 /*
gcc >= 12.0 */\n";
+  pr "#define LIBNBD_ATTRIBUTE_NONNULL(s) __attribute__((__nonnull__
s))\n";
+  pr "#else\n";
+  pr "#define LIBNBD_ATTRIBUTE_NONNULL(s)\n";
+  pr "#endif\n";
+  pr "#endif /* ! defined LIBNBD_ATTRIBUTE_NONNULL */\n";
+  pr "\n";
   pr "struct nbd_handle;\n";
   pr "\n";
   List.iter (
@@ -382,7 +425,7 @@ let
   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);\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 const char *nbd_get_error (void);\n";
@@ -770,6 +813,12 @@ let
   pr "\n";
   pr "#include <pthread.h>\n";
   pr "\n";
+  pr "/* GCC will remove NULL checks from this file for any
parameter\n";
+  pr " * annotated with attribute((nonnull)).  See RHBZ#1041336. 
To\n";
+  pr " * avoid this, disable the attribute when including
libnbd.h.\n";
+  pr " */\n";
+  pr "#define LIBNBD_ATTRIBUTE_NONNULL(s)\n";
+  pr "\n";
   pr "#include \"libnbd.h\"\n";
   pr "#include \"internal.h\"\n";
   pr "\n";
-- 
2.37.0.rc2

We previously checked only that String parameters are not NULL,
returning an error + EFAULT if so.

However we did not check Bytes*, SockAddrAndLen, Path or StringList
parameters, also never NULL.  Be consistent about checks.

Thanks: Eric Blake for help and an earlier version of the patch
---
 generator/API.ml | 7 +++++--
 generator/C.ml   | 7 ++++++-
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/generator/API.ml b/generator/API.ml
index 6e3213ad26..0975a407c1 100644
--- a/generator/API.ml
+++ b/generator/API.ml
@@ -3851,13 +3851,16 @@ let ()  
     (* !may_set_error is incompatible with certain parameters because
      * we have to do a NULL-check on those which may return an error.
+     * Refer to generator/C.ml:generator_lib_api_c.
      *)
     | name, { args; may_set_error = false }
          when List.exists
                 (function
-                 | Closure _ | Enum _ | Flags _ | String _ -> true
+                 | Closure _ | Enum _ | Flags _
+                 | BytesIn _ | BytesOut _ | BytesPersistIn _ | BytesPersistOut
_
+                 | SockAddrAndLen _ | Path _ | String _ -> true
                  | _ -> false) args ->
-       failwithf "%s: if args contains Closure/Enum/Flags/String
parameter, may_set_error must be false" name
+       failwithf "%s: if args contains any non-null pointer parameter,
may_set_error must be false" name
 
     (* !may_set_error is incompatible with certain optargs too.
      *)
diff --git a/generator/C.ml b/generator/C.ml
index cafc5590e2..bfc216609e 100644
--- a/generator/C.ml
+++ b/generator/C.ml
@@ -614,7 +614,12 @@ let
          need_out_label := true
       | Flags (n, flags) ->
          print_flags_check n flags None
-      | String n ->
+      | BytesIn (n, _) | BytesOut (n, _)
+      | BytesPersistIn (n, _) | BytesPersistOut (n, _)
+      | SockAddrAndLen (n, _)
+      | Path n
+      | String n
+      | StringList n ->
          let value = match errcode with
            | Some value -> value
            | None -> assert false in
-- 
2.37.0.rc2

In the C API documentation mention the potential problems of calling
non-nullable parameters with NULL.  Usually an error is returned, but
warnings and worse might happen too.

Thanks: Eric Blake
---
 docs/libnbd.pod | 18 ++++++++++++++++++
 generator/C.ml  | 21 +++++++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/docs/libnbd.pod b/docs/libnbd.pod
index eae9bb413b..170fc1fa07 100644
--- a/docs/libnbd.pod
+++ b/docs/libnbd.pod
@@ -387,8 +387,26 @@ The library ran out of memory while performing some
operation.
 A request is too large, for example if you try to read too many bytes
 in a single L<nbd_pread(3)> call.
 
+=item C<EFAULT>
+
+A pointer parameter was C<NULL> when it should be non-NULL.
+See the section below.
+
 =back
 
+=head2 Non-NULL parameters
+
+Almost all libnbd functions when called from C take one or more
+pointer parameters that must not be C<NULL>.  For example, the handle
+parameter, strings and buffers should usually not be C<NULL>.
+
+If a C<NULL> is passed as one of these parameters, libnbd attempts to
+return an error with L<nbd_get_errno(3)> returning C<EFAULT>.
+
+However it may cause other compiler-related warnings and even
+undefined behaviour, so you should try to avoid this programming
+mistake.
+
 =head1 DEBUGGING MESSAGES
 
 Libnbd can print lots of debugging messages, useful if you have a
diff --git a/generator/C.ml b/generator/C.ml
index bfc216609e..dc123d20a4 100644
--- a/generator/C.ml
+++ b/generator/C.ml
@@ -107,6 +107,11 @@ let
   | UInt64 n -> [n]
   | UIntPtr n -> [n]
 
+let name_of_optarg +  function
+  | OClosure { cbname } -> cbname
+  | OFlags (n, _, _) -> n
+
 let arg_attr_nonnull    function
   (* BytesIn/Out are passed using a non-null pointer, and size_t *)
@@ -974,6 +979,7 @@ let
 
   pr "=head1 ERRORS\n";
   pr "\n";
+
   if may_set_error then (
     let value = match errcode with
       | Some value -> value
@@ -987,6 +993,21 @@ let
     pr "This function does not fail.\n";
   pr "\n";
 
+  pr "The following parameters must not be NULL: C<h>";
+  List.iter (
+    fun arg ->
+      let nns = arg_attr_nonnull arg in
+      if List.mem true nns then pr ", C<%s>" (List.hd
(name_of_arg arg))
+  ) args;
+  List.iter (
+    fun arg ->
+      let nns = optarg_attr_nonnull arg in
+      if List.mem true nns then pr ", C<%s>" (name_of_optarg
arg)
+  ) optargs;
+  pr ".\n";
+  pr "For more information see L<libnbd(3)/Non-NULL
parameters>.\n";
+  pr "\n";
+
   if permitted_states <> [] then (
     pr "=head1 HANDLE STATE\n";
     pr "\n";
-- 
2.37.0.rc2

Change check into an assertion, and add detailed comments explaining
our assumptions.

Updates: commit d0fbb769286a97728b0d1358e7accc2eb708d795
---
 lib/utils.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/lib/utils.c b/lib/utils.c
index cc31b00309..93e520d2fb 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -79,11 +79,21 @@ nbd_internal_copy_string_list (string_vector *v, char **in)
 int
 nbd_internal_set_argv (struct nbd_handle *h, char **argv)
 {
-  /* FIXME: Do we want to assert(argv)? */
-  if (argv == NULL || argv[0] == NULL) {
+  /* This should never be NULL.  Normally the generator adds code to
+   * each StringList call in lib/api.c to check this and return an
+   * error.
+   */
+  assert (argv);
+
+  /* Because this function is only called from functions that take
+   * argv-style lists of strings (such as nbd_connect_command) we can
+   * check here that the command name is present.
+   */
+  if (argv[0] == NULL) {
     set_error (EINVAL, "missing command name in argv list");
     return -1;
   }
+
   if (nbd_internal_copy_string_list (&h->argv, argv) == -1) {
     set_error (errno, "realloc");
     return -1;
-- 
2.37.0.rc2