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

[Martin Kletzander]

On Mon, Sep 15, 2014 at 07:49:09AM -0400, John Ferlan wrote:
>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 at 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.
>

Well, then that sa_assert() would make sense being there (where the
dereference happens) or having a check only for that in some
meaningful place.  But I believe we're trying to solve a completely
different problem.  I was just stupid when I wrote the code :)

>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.
>

But having it where you proposed it doesn't really make sense to me
(or is not visible at first).

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

Yes, and you can remove the "nfds &&" I believe (I have no idea why I
used it there).  ACK with this squashed in.

Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140915/0cd331f0/attachment-0001.sig>