Re: [Libguestfs] [libnbd PATCH v3 01/18] api: Fix crashes on nbd_connect_command with bad argv

On Tue, Sep 27, 2022 at 12:14:28PM +0100, Richard W.M. Jones wrote: More of a rule of thumb. Letting a library kill an entire program with SIGSEGV because of a bug in the outer program is not nice. If we document it well, we can say it's the program's own fault; but right now, we don't document that a StringList parameter must be non-NULL, either in our prose, or with __attribute__((nonnull)) in our public .h files. However, note that the use of the compiler attribute has its own drawbacks - gcc treats it as license to optimize the implementation to skip the NULL check, but does not always warn about callers that violate the invariant and actually pass NULL in - if you use the attribute, you actually lose out on the ability to write a library that tries to document that passing NULL is dumb but which still sanely handles NULL - because the sane handling is optimized away. Libvirt tried doing this a while back, and ended up with their current practice of enabling attribute((nonnull)) for Coverity scanning, but disabling it for gcc compilation. Yes, these are cases where the documentation is explicit that passing in NULL leads to undefined behavior (part of the C standard). Whether gcc actually allows the glibc implementation to actually check for NULL and turn it into errno EINVAL, or optimized it out and results in a SIGSEGV, is then a question for how the library itself was compiled while those .h attributes are present. Correct. The python bindings always pass a non-NULL array; but when the array is empty, there is no argv[0]. This fix is worth having no matter what. The other fix is whether we should special case passing in NULL instead of an empty array (the python bindings can't trigger it). We already do this sort of NULL checking for String arguments (again, where python bindings can't trigger that, either), so we do have precedence; but we could also take alternatives of better documenting that C code should not pass in NULL (it's no longer our fault if the program didn't obey the documentation invariants and gets a crash because they passed in NULL) and/or try to decorate our public .h with attribute((nonnull)) to allow compilers that are capable of warning users about bad inputs about their mistake (I'm not yet sure if gcc has quite reached that point; it wasn't there a few years ago when libvirt decided NOT to use attribute((nonnull)) in the public headers). I can split this into two parts; the obvious avoidance of the missing argv[0] (we want that no matter what), and the less-obvious behavior of what to do when NULL is passed to StringList (where we have several options of what we want it to do, and where a segv + static compiler flags that will warn users about suspect coding may indeed be nicer than blindly failing with EFAULT, for forcing the user to not write bad code - but I'm not yet sure if we have reliable compiler setups that can be used to get that environment). This is the hunk that enforces StringList must not be NULL (the one where attribute((nonnull)) may be worth exploring)... While this hunk (and its helper function and updates to all callsites) is where diagnosing empty arrays is the noncontroversial fix. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org