Re: [Libguestfs] [PATCH libnbd 4/5] generator: Check that more parameters are not NULL
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