[PATCH libnbd 0/5] generator: Add attribute((nonnull)) annotations
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
On Tue, Sep 27, 2022 at 03:46:19PM +0100, Richard W.M. Jones wrote:
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(-)
ACK 1 and 2 regardless of the rest of the series. For this one...
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 ]
It's a little bit odd to see UIntPtr called a 'non-pointer type' - but technically, it is an integer rather than a pointer. And the clincher is that even if it represents a pointer, for our purposes it is an opaque type, and the user can indeed pass in NULL (cast to uintptr_t) and we should not complain. So nothing wrong here other than maybe a confusing comment, although I don't have wording suggestions to help.
@@ -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)
I'm still getting used to OCaml's ability to rebind a variable as many iterations as we want, even with different typing! But this makes sense.
@@ -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";
Does disabling the warning actually force the compiler to emit the nonnull check, or can it still be optimized away in spite of us silencing the warning? Maybe we better off writing it so that for _this_ .c file, we pre-define LIBNBD_ATTRIBUTE_NONNULL() to be a no-op regardless of what the included .h files say. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On Tue, Sep 27, 2022 at 01:25:55PM -0500, Eric Blake wrote:
On Tue, Sep 27, 2022 at 03:46:19PM +0100, Richard W.M. Jones wrote:
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(-)
ACK 1 and 2 regardless of the rest of the series.
For this one...
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 ]
It's a little bit odd to see UIntPtr called a 'non-pointer type' - but technically, it is an integer rather than a pointer. And the clincher is that even if it represents a pointer, for our purposes it is an opaque type, and the user can indeed pass in NULL (cast to uintptr_t) and we should not complain. So nothing wrong here other than maybe a confusing comment, although I don't have wording suggestions to help.
@@ -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)
I'm still getting used to OCaml's ability to rebind a variable as many iterations as we want, even with different typing! But this makes sense.
Some programmers will write it as: let nns = ... in let nns' = ... in let nns'' = ... in let nns''' = ... in where the ' character is pronounced "prime". Whether that's more or less confusing I'll leave up to you to decide :-)
@@ -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";
Does disabling the warning actually force the compiler to emit the nonnull check, or can it still be optimized away in spite of us silencing the warning?
So firstly this pragma is necessary in order to get rid of a warning that would otherwise cause an error when using -Werror mode. It only disables the warning and GCC may still compile away the checks. I checked the asm just now and ... it does appear to be getting rid of the checks! How annoying is that?
Maybe we better off writing it so that for _this_ .c file, we pre-define LIBNBD_ATTRIBUTE_NONNULL() to be a no-op regardless of what the included .h files say.
Let me try something like that. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
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
On Tue, Sep 27, 2022 at 03:46:20PM +0100, Richard W.M. Jones wrote:
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.
Overlaps with part of my v3 1/18 patch, but I agreed to factor that part out of that patch, and I like that you were more consistent to all other pointer types where we think it is worth decorating the .h file, whereas mine had only touched just StringList.
--- 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
If we want this, you should also touch API.ml where we check whether "!may_set_error is incompatible with certain parameters because we have to do a NULL-check on those which may return an error". I'm a fan of being consistent, but I'm also still on the fence as to whether letting the library segfault is actually nicer for diagnosing a user's egregious misuse of the API. I still think we should beef up the documentation of the affected functions to make it explicit that passing NULL is undefined behavior, whether or not we choose to go with EFAULT/-1 result or segv. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
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 Tue, Sep 27, 2022 at 03:46:21PM +0100, Richard W.M. Jones wrote:
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; + } +
This is basically half of my v3 1/18 patch - the part that was noncontroversial. Comparing my version to yours, I picked a different error message: nbdsh: command line script failed: nbd_connect_command: missing command name in argv list: Invalid argument and centralized things into a single helper function in utils.c instead of open-coding it at each affected nbd*_connect_* command.
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
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
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
On Tue, Sep 27, 2022 at 06:24:17PM +0200, Laszlo Ersek wrote:
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)
argv[] for nbd_connect_command
- 0 or more (optional list)
queries[] for nbd_opt_list_meta_context_queries (in my v3 series)
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).
Yes, we need to document whether NULL triggers undefined behavior, regardless of how we then decide to further deal with it (either adding attributes to the .h, adding in explicit NULL checks, or letting the code segv are all okay if we have first documented that NULL gives unspecified results).
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().
Your argument of multiplicity is interesting. Extrapolating it a bit more, I think you are arguing that in Python, h.connect_command([]) - error, since list must be non-empty h.connect_command(None) - error should be same as [], rather than complaining that 'None' is not a list type h.opt_list_meta_context_queries([], func) - success, since empty list makes sense h.opt_list_meta_context_queries(None, func) - same effect (whereas in my v3 patches, it complains that 'None' is not a list type)
(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.)
We undef it during unit tests, but I don't know if we have been brave enough to declare that we mandate that assertions remain live in the library itself.
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).
Agree with all of those points.
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.
That was the conclusion libvirt had several years ago - avoiding attribute nonnull was better than trying to use it, at least for public interfaces (it is still useful for Coverity analysis, though). I don't know if the state of the art gcc is doing better at it now.
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)
This assertion is NOT because we violated the non-NULL parameter, but because we weren't checking for non-empty list soon enough. The python equivalent to allowing a NULL pointer would be accepting h.connect_command(None) (right now, that generates a python TypeError).
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...)
So far, none of our list arguments have been optional, and none of our optional arguments have been lists. The addition of nbd_opt_list_meta_context_queries, where it is often desirable to pass an empty list of queries, may be the first case where we do want to represent the queries as an optional list (in my v3 series, it was a mandatory argument). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On 09/27/22 20:59, Eric Blake wrote:
On Tue, Sep 27, 2022 at 06:24:17PM +0200, Laszlo Ersek wrote:
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().
Your argument of multiplicity is interesting. Extrapolating it a bit more, I think you are arguing that in Python,
h.connect_command([]) - error, since list must be non-empty h.connect_command(None) - error should be same as [], rather than complaining that 'None' is not a list type
h.opt_list_meta_context_queries([], func) - success, since empty list makes sense h.opt_list_meta_context_queries(None, func) - same effect (whereas in my v3 patches, it complains that 'None' is not a list type)
Well, I don't know. In C, where we represent the string list with a NULL-terminated (char**), I really feel like there isn't a semantic difference between vec==NULL and vec[0]==NULL. It's a very rough representation anyway. But in Python and OCaml, where we can distinguish "None" from "[]", and "None" from "Some []" respectively, I'm not so sure myself. Python and OCaml seem to imbue the representation with more meaning / type information than C does.
(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.)
We undef it during unit tests, but I don't know if we have been brave enough to declare that we mandate that assertions remain live in the library itself.
I think removing assertions from a library is the more courageous option! :) Is it better to produce garbage (or to set up a real, but long-distance crash) than to stop immediately, when we know something has gone wrong? I think the only alternative to live assertions is to code up some kind of recovery in every possible spot. That's a lot of work. (I've heard this argument wrt. kernel modules -- panicking the whole kernel due to hardware misbehavior in a driver is frowned upon, to say the least. So the kernel driver apparently needs to, at the least, disable itself dynamically, when a device behaves out of spec. That's a lot of work.) What projects consume libnbd primarily? I think in the context of a virt-v2v / virt-p2v conversion, it's better for the conversion to crash early, than to carry on with garbage data (for example).
So far, none of our list arguments have been optional, and none of our optional arguments have been lists. The addition of nbd_opt_list_meta_context_queries, where it is often desirable to pass an empty list of queries, may be the first case where we do want to represent the queries as an optional list (in my v3 series, it was a mandatory argument).
I'm out of ideas. I'd like to ask "can we get away with just modifying the documentation, and let the user deal with undefined behavior afterwards?" But given python and ocaml, I don't think this is a good idea, because those languages simply don't allow "undefined behavior" (definitely not to the extent that C does). In other words, python and ocaml promise the user so much more than C does that the python and ocaml bindings would have to be *much* thicker (contain much more logic and checking) than the C binding. I keep returning to my impression that the "thin" (?) binding generation is a very leaky abstraction. I apologize, I know that this is not constructive. I'm just out of ideas. Laszlo
participants (3)
-
Eric Blake -
Laszlo Ersek -
Richard W.M. Jones