Libguestfs - Sep 2022 - [PATCH libnbd 5/5] lib/connect: Avoid segfault for zero-length argv

This patch series adds nonnull annotations for parameters which should
be non-NULL.

There was much discussion on IRC about whether this is a good idea,
pointing in particular to the bug below which is still present in
modern GCC.  It's better to have these discussions on list so they're
archived.

https://bugzilla.redhat.com/show_bug.cgi?id=1041336

There's a possible follow-up patch which *removes* all the pointer =NULL
tests added in the final patch, again something for discussion.
See my view on this topic here (and Eric's follow up):
https://listman.redhat.com/archives/libguestfs/2022-September/029966.html

Rich.

No change, just make the layout consistent with similar functions
above.
---
 generator/C.ml | 45 +++++++++++++++++++++++----------------------
 1 file changed, 23 insertions(+), 22 deletions(-)

diff --git a/generator/C.ml b/generator/C.ml
index b2d46f985a..f4d24b650d 100644
--- a/generator/C.ml
+++ b/generator/C.ml
@@ -83,28 +83,29 @@ let
   | RUInt64 -> "uint64_t"
   | RFlags _ -> "uint32_t"
 
-let rec name_of_arg = function
-| Bool n -> [n]
-| BytesIn (n, len) -> [n; len]
-| BytesOut (n, len) -> [n; len]
-| BytesPersistIn (n, len) -> [n; len]
-| BytesPersistOut (n, len) -> [n; len]
-| Closure { cbname } ->
-   [ sprintf "%s_callback" cbname; sprintf "%s_user_data"
cbname ]
-| Enum (n, _) -> [n]
-| Fd n -> [n]
-| Flags (n, _) -> [n]
-| Int n -> [n]
-| Int64 n -> [n]
-| Path n -> [n]
-| SizeT n -> [n]
-| SockAddrAndLen (n, len) -> [n; len]
-| String n -> [n]
-| StringList n -> [n]
-| UInt n -> [n]
-| UInt32 n -> [n]
-| UInt64 n -> [n]
-| UIntPtr n -> [n]
+let rec name_of_arg +  function
+  | Bool n -> [n]
+  | BytesIn (n, len) -> [n; len]
+  | BytesOut (n, len) -> [n; len]
+  | BytesPersistIn (n, len) -> [n; len]
+  | BytesPersistOut (n, len) -> [n; len]
+  | Closure { cbname } ->
+     [ sprintf "%s_callback" cbname; sprintf "%s_user_data"
cbname ]
+  | Enum (n, _) -> [n]
+  | Fd n -> [n]
+  | Flags (n, _) -> [n]
+  | Int n -> [n]
+  | Int64 n -> [n]
+  | Path n -> [n]
+  | SizeT n -> [n]
+  | SockAddrAndLen (n, len) -> [n; len]
+  | String n -> [n]
+  | StringList n -> [n]
+  | UInt n -> [n]
+  | UInt32 n -> [n]
+  | UInt64 n -> [n]
+  | UIntPtr n -> [n]
 
 let rec print_arg_list ?(wrap = false) ?maxcol ?handle ?types ?(parens = true)
           ?closure_style args optargs -- 
2.37.0.rc2

This function generates C function decls so name it accordingly.
---
 generator/C.ml | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/generator/C.ml b/generator/C.ml
index f4d24b650d..013f81edf4 100644
--- a/generator/C.ml
+++ b/generator/C.ml
@@ -213,7 +213,7 @@ let
   pr "%s nbd_%s " (type_of_ret ret) name;
   print_arg_list ~handle:true ?wrap ?maxcol ?closure_style args optargs
 
-let print_extern ?wrap ?closure_style name args optargs ret +let print_fndecl
?wrap ?closure_style name args optargs ret    pr "extern ";
   print_call ?wrap ?closure_style name args optargs ret;
   pr ";\n"
@@ -303,9 +303,9 @@ let
   ) oclosures;
   pr "\n"
 
-let print_extern_and_define ?wrap name args optargs ret +let
print_fndecl_and_define ?wrap name args optargs ret    let name_upper =
String.uppercase_ascii name in
-  print_extern ?wrap name args optargs ret;
+  print_fndecl ?wrap name args optargs ret;
   pr "#define LIBNBD_HAVE_NBD_%s 1\n" name_upper;
   pr "\n"
 
@@ -394,7 +394,7 @@ let
   print_closure_structs ();
   List.iter (
     fun (name, { args; optargs; ret }) ->
-      print_extern_and_define ~wrap:true name args optargs ret
+      print_fndecl_and_define ~wrap:true name args optargs ret
   ) handle_calls;
   List.iter (
     fun (ns, ctxts) -> print_ns ns ctxts
@@ -413,7 +413,7 @@ let
   pr "\n";
   List.iter (
     fun (name, { args; optargs; ret }) ->
-    print_extern ~wrap:true ~closure_style:Pointer ("unlocked_" ^
name)
+    print_fndecl ~wrap:true ~closure_style:Pointer ("unlocked_" ^
name)
       args optargs ret
   ) handle_calls;
   pr "\n";
-- 
2.37.0.rc2

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 | 52 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 50 insertions(+), 2 deletions(-)

diff --git a/generator/C.ml b/generator/C.ml
index 013f81edf4..4f758e526f 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,17 @@ 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 "#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 "\n";
   pr "struct nbd_handle;\n";
   pr "\n";
   List.iter (
@@ -382,7 +423,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";
@@ -773,6 +814,13 @@ let
   pr "#include \"libnbd.h\"\n";
   pr "#include \"internal.h\"\n";
   pr "\n";
+  pr "/* We check that some string parameters declared as nonnull
are\n";
+  pr " * not NULL.  This is intentional because we do not know if
the\n";
+  pr " * calling compiler checked the attributes.  So ignore
those\n";
+  pr " * warnings here.\n";
+  pr " */\n";
+  pr "#pragma GCC diagnostic ignored
\"-Wnonnull-compare\"\n";
+  pr "\n";
   List.iter print_wrapper handle_calls
 
 (* We generate a fragment of Makefile.am containing the list
-- 
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.

I'm not sure if we ought to be checking parameters for NULL like this
at all (preferring instead to simply crash), but at least let's be
consistent about it.
---
 generator/C.ml | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/generator/C.ml b/generator/C.ml
index 4f758e526f..87ed5969ff 100644
--- a/generator/C.ml
+++ b/generator/C.ml
@@ -612,7 +612,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

Eric found that passing a zero length array to nbd_connect_command or
nbd_connect_systemd_socket_activation results in a segfault.  This can
be triggered through Python as follows:

$ nbdsh -c 'h.connect_command([])'
nbdsh: generator/states-connect.c:247: enter_STATE_CONNECT_COMMAND_START:
Assertion `h->argv.ptr[0]' failed.
Aborted (core dumped)

Reported-by: Eric Blake
---
 lib/connect.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/lib/connect.c b/lib/connect.c
index 5008063034..629f35db7c 100644
--- a/lib/connect.c
+++ b/lib/connect.c
@@ -251,6 +251,11 @@ nbd_unlocked_aio_connect_socket (struct nbd_handle *h, int
sock)
 int
 nbd_unlocked_aio_connect_command (struct nbd_handle *h, char **argv)
 {
+  if (argv[0] == NULL) {
+    set_error (EINVAL, "argv parameter must have at least 1
element");
+    return -1;
+  }
+
   if (nbd_internal_set_argv (&h->argv, argv) == -1) {
     set_error (errno, "realloc");
     return -1;
@@ -263,6 +268,11 @@ int
 nbd_unlocked_aio_connect_systemd_socket_activation (struct nbd_handle *h,
                                                     char **argv)
 {
+  if (argv[0] == NULL) {
+    set_error (EINVAL, "argv parameter must have at least 1
element");
+    return -1;
+  }
+
   if (nbd_internal_set_argv (&h->argv, argv) == -1) {
     set_error (errno, "realloc");
     return -1;
-- 
2.37.0.rc2

On 09/27/22 16:46, Richard W.M. Jones wrote:
> This patch series adds nonnull annotations for parameters which should
> be non-NULL.
> 
> There was much discussion on IRC about whether this is a good idea,
> pointing in particular to the bug below which is still present in
> modern GCC.  It's better to have these discussions on list so
they're
> archived.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1041336
> 
> There's a possible follow-up patch which *removes* all the pointer
=> NULL tests added in the final patch, again something for discussion.
> See my view on this topic here (and Eric's follow up):
> https://listman.redhat.com/archives/libguestfs/2022-September/029966.html
I think it boils down to the permitted multiplicity of a paramater:

- exactly 1 (mandatory parameter)
- 0 or 1 (optional parameter)
- 1 or more (mandatory (non-empty) list)
- 0 or more (optional list)

First I think we should figure out what parameter has what multiplicity.

Then, it should be documented for the end user (this can be generated,
but either way, the documentation should be clear about the decisions).

Considering "optional list" in particular, I see no semantic
difference
between vec==NULL and vec[0]==NULL. If an optional list is expected,
both should be tolerated; if a mandatory (non-empty) list is expected,
both are invalid.

Once we decided / documented what parameters were valid, I think the
most practical way to enfoce mandatory parameters (in case they are
taken by address) and mandatory (non-empty) lists would be with assert().

(We should also make sure that NDEBUG is never defined -- some parts of
libnbd and nbdkit already "#undef NDEBUG"; I'd go farther and just
forbid building libnbd and nbdkit without assertions. Assertions cost a
few CPU cycles, and I don't expect nbdkit to be CPU-bound ever.
Assertions are worth the CPU costs.)

assert() is good because:

- it crashes (and yes, once we document the expectations, crashing a
program from a library is fine),

- static analyzers such as coverity understand it (to my knowledge),

- gcc will not remove it (in the absence of NDEBUG, but for that, see
above).

I think the nonnull attribute is not worth it. It might catch statically
provable NULL arguments, but cannot catch such that are not statically
provable. By weakening the function's internall NULL checks, it
introduces new problems for those statically-not-provable-but-still-NULL
code paths.

In fact, assert() *in combination* with attribute nonnull would be the
best: issue warnings at build time in case the NULL arg is statically
provable, use assert() to catch anything that might slip through
dynamically. Unfortunately, attribute nonnull may not live up to the
build-time-warning expectation (dependent on the gcc version), but it
*does* eviscerate assert() -- if I understand correctly. So attribute
nonnull does more harm than good, apparently.

If *all* NULL args were statically provable, then attribute nonnull,
with gcc 12+, would clearly win over assert() -- but not all such args
are statically provable. Therefore we need assert(), and because
attribute nonnull actually weakens assert(), we should *only* use assert().

... And my argument would end here, in case we didn't generate the
python bindings. If the python bindings were a separate project, I'd say
that the symptom

$ nbdsh -c 'h.connect_command([])'
nbdsh: generator/states-connect.c:247:
enter_STATE_CONNECT_COMMAND_START: Assertion `h->argv.ptr[0]' failed.
Aborted (core dumped)

was entirely valid (expected), and that it was up to the implementors of
the python bindings to catch an empty list here, turn it into a python
exception, lest the C function's invariants be violated. So, end-to-end,
that would result in an assert() in the C function, and an "if" in the
Python code.

*But*. Given that we generate the python bindings... we might as well
just move the "if" into the most deeply lying C code, in place of the
assert()s, and then let the generator turn that error into a Python
exception higher up, as usual.

So, purely because of this centralized code generation, I guess I'm
arguing for explicit "ifs" in the deepest (generated, or hand-written)
C
code, and avoiding the nonnull attribute. Again, I don't see a semantic
difference here between vec==NULL and (vec != NULL && vec[0] == NULL).

(There *is* a difference between "setting something to an empty list"
vs. "not setting something at all", but we express that differently
already, I hope! If we do distinguish "nonexistent" (~optional) from
"empty", then I get to redo my whole argument...)

Laszlo