[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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 redhat com>
>> ---
>> 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;
     }



John
> 
> Martin
> 
>>         VIR_ERROR(_("Too many (%u) FDs passed from caller"), nfds);
>>         return -1;
>> --
>> 1.9.3
>>
>> --
>> libvir-list mailing list
>> libvir-list redhat com
>> https://www.redhat.com/mailman/listinfo/libvir-list


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]