Re: [libvirt] [PATCH 8/8] daemon: Resolve Coverity FORWARD_NULL

On 09/15/2014 04:36 AM, Martin Kletzander wrote: > On Sat, Sep 13, 2014 at 09:27:45AM -0400, John Ferlan wrote: >> Coverity complains that the comparison: >> >> if (nfds && nfds > ((int)!!sock_path + (int)!!sock_path_ro)) >> >> could mean 'sock_path' is NULL. Later in virNetSocketNewListenUNIX >> there's a direct dereference of path in the error path: >> >> if (path[0] != '@') >> >> A bit of sleuthing proves that upon entry to daemonSetupNetworking >> there is no way for 'sock_path' to be NULL since daemonUnixSocketPaths >> will set up 'sock_file' (although it may not set up 'sock_file_ro') >> in all 3 paths. >> >> So to keep Coverity quiet - add an sa_assert prior to the call. >> >> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; >> --- >> daemon/libvirtd.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c >> index 05ee50e..028145e 100644 >> --- a/daemon/libvirtd.c >> +++ b/daemon/libvirtd.c >> @@ -502,6 +502,7 @@ static int daemonSetupNetworking(virNetServerPtr srv, >> return -1; >> } >> >> + sa_assert(sock_path); >> if (nfds && nfds > ((int)!!sock_path + (int)!!sock_path_ro)) { > > Well, if sock_path cannot be NULL, then why not change it to: > > if (nfds > (1 + !!sock_path_ro)) { > > or something like that? Sure, but while I ensured that "today" - who's to say tomorrow someone doesn't make a change in the future that alters that assumption? Just removing "sock_path" still "hides" the fact that someone could incorrectly pass a NULL sock_path here. At least the sa_assert() will cover that. The "real" reason Coverity complains is because sock_path is eventually dereferenced elsewhere without first checking if it's NULL, but Coverity believes this code is check for it being NULL. It's a double edged sword.

Another "option" would be to adjust the code in that place to ensure path is valid before dereferencing. Of course, knowing that there is another change pending in this area that I reviewed and that it also makes the same deref, I think not using sa_assert here would be the gift that keeps on giving with respect to Coverity complaints.

So, how about the following instead (for equally challenging ternary operations): $ git diff diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 893aacf..5b8f28e 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -442,12 +442,13 @@ static void daemonInitialize(void) } -static int daemonSetupNetworking(virNetServerPtr srv, - struct daemonConfig *config, - const char *sock_path, - const char *sock_path_ro, - bool ipsock, - bool privileged) +static int ATTRIBUTE_NONNULL(3) +daemonSetupNetworking(virNetServerPtr srv, + struct daemonConfig *config, + const char *sock_path, + const char *sock_path_ro, + bool ipsock, + bool privileged) { virNetServerServicePtr svc = NULL; virNetServerServicePtr svcRO = NULL; @@ -467,8 +468,7 @@ static int daemonSetupNetworking(virNetServerPtr srv, return -1; } - sa_assert(sock_path); - if (nfds && nfds > ((int)!!sock_path + (int)!!sock_path_ro)) { + if (nfds && nfds > (sock_path_ro ? 2 : 1)) { VIR_ERROR(_("Too many (%u) FDs passed from caller"), nfds); return -1; }