[libvirt] [PATCH] add an assert, to avoid a false-positive NULL-deref warning from clang
Here's a case in which using an assertion appears to be the only way to tell clang that "client" really is non-NULL at that point. I'm sure clang's analyzers will eventually improve, and hence avoid this sort of false positive, so have marked this with a FIXME comment, to help ensure we eventually remove this otherwise unnecessary assertion.
From ba26bad7aec8713ded0fd3300e951eac3673cc72 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Mon, 1 Mar 2010 15:19:48 +0100 Subject: [PATCH] add an assert, to avoid a false-positive NULL-deref warning from clang
* daemon/libvirtd.c (qemudWorker): Assert. --- daemon/libvirtd.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 9bdbecb..a357914 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -24,34 +24,35 @@ #include <config.h> #include <sys/types.h> #include <sys/wait.h> #include <sys/stat.h> #include <unistd.h> #include <fcntl.h> #include <limits.h> #include <sys/socket.h> #include <sys/un.h> #include <sys/poll.h> #include <netinet/in.h> #include <netinet/tcp.h> #include <netdb.h> #include <stdlib.h> #include <pwd.h> #include <stdio.h> +#include <assert.h> #include <stdarg.h> #include <syslog.h> #include <string.h> #include <errno.h> #include <getopt.h> #include <fnmatch.h> #include <grp.h> #include <signal.h> #include <netdb.h> #include "libvirt_internal.h" #include "virterror_internal.h" #define VIR_FROM_THIS VIR_FROM_QEMU #include "libvirtd.h" #include "dispatch.h" @@ -1504,34 +1505,38 @@ static void *qemudWorker(void *data) virMutexLock(&server->lock); while (((client = qemudPendingJob(server)) == NULL) && !worker->quitRequest) { if (virCondWait(&server->job, &server->lock) < 0) { virMutexUnlock(&server->lock); return NULL; } } if (worker->quitRequest) { if (client) virMutexUnlock(&client->lock); virMutexUnlock(&server->lock); return NULL; } worker->processingCall = 1; virMutexUnlock(&server->lock); + /* Tell clang we know what we're doing. + FIXME: remove when clang improves. */ + assert (client); + /* We own a locked client now... */ client->refs++; /* Remove our message from dispatch queue while we use it */ msg = qemudClientMessageQueueServe(&client->dx); /* This function drops the lock during dispatch, * and re-acquires it before returning */ if (remoteDispatchClientRequest (server, client, msg) < 0) { VIR_FREE(msg); qemudDispatchClientFailure(client); client->refs--; virMutexUnlock(&client->lock); continue; } client->refs--; -- 1.7.0.1.414.g89213d
According to Jim Meyering on 3/1/2010 10:13 AM:
Here's a case in which using an assertion appears to be the only way to tell clang that "client" really is non-NULL at that point. I'm sure clang's analyzers will eventually improve, and hence avoid this sort of false positive, so have marked this with a FIXME comment, to help ensure we eventually remove this otherwise unnecessary assertion.
Thanks for the extra context; it makes in-line review a breeze.
@@ -1504,34 +1505,38 @@ static void *qemudWorker(void *data) virMutexLock(&server->lock); while (((client = qemudPendingJob(server)) == NULL) && !worker->quitRequest) { if (virCondWait(&server->job, &server->lock) < 0) { virMutexUnlock(&server->lock); return NULL; } }
Indeed, the only way client can be NULL at this point is if worker->quitRequest is true...
if (worker->quitRequest) { if (client) virMutexUnlock(&client->lock); virMutexUnlock(&server->lock); return NULL; }
But that means we exit here.
worker->processingCall = 1; virMutexUnlock(&server->lock);
+ /* Tell clang we know what we're doing. + FIXME: remove when clang improves. */ + assert (client);
So this assertion is valid. ACK, if assert() is okay. On the other hand, perhaps a more invasive rewrite would also work while also avoiding assert(), by hoisting the worker->quitRequest into the while loop, something like: while ((client = qemudPendingJob(server)) == NULL) { if (worker->quitRequest || virCondWait(&server->job, &server->lock) < 0) { virMutexUnlock(&server->lock); return NULL; } } if (worker->quitRequest) { virMutexUnlock(&client->lock); virMutexUnlock(&server->lock); return NULL; } Should I write that into patch format? -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On the other hand, perhaps a more invasive rewrite would also work while also avoiding assert(), by hoisting the worker->quitRequest into the while loop, something like:
while ((client = qemudPendingJob(server)) == NULL) { if (worker->quitRequest || virCondWait(&server->job,&server->lock)< 0) { virMutexUnlock(&server->lock); return NULL; } } if (worker->quitRequest) { virMutexUnlock(&client->lock); virMutexUnlock(&server->lock); return NULL; }
Should I write that into patch format?
I think it's best to report the bug to clang's bugzilla and not clutter libvirt too much. Paolo
On Mon, Mar 01, 2010 at 06:36:47PM +0100, Paolo Bonzini wrote:
On the other hand, perhaps a more invasive rewrite would also work while also avoiding assert(), by hoisting the worker->quitRequest into the while loop, something like:
while ((client = qemudPendingJob(server)) == NULL) { if (worker->quitRequest || virCondWait(&server->job,&server->lock)< 0) { virMutexUnlock(&server->lock); return NULL; } } if (worker->quitRequest) { virMutexUnlock(&client->lock); virMutexUnlock(&server->lock); return NULL; }
Should I write that into patch format?
I think it's best to report the bug to clang's bugzilla and not clutter libvirt too much.
Agreed, I rather we just reported it to clang with the demo code & not do anything in libvirt. It is easy enough to ignore this on subsequent clang runs Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Mon, Mar 01, 2010 at 10:27:43AM -0700, Eric Blake wrote:
According to Jim Meyering on 3/1/2010 10:13 AM:
Here's a case in which using an assertion appears to be the only way to tell clang that "client" really is non-NULL at that point. I'm sure clang's analyzers will eventually improve, and hence avoid this sort of false positive, so have marked this with a FIXME comment, to help ensure we eventually remove this otherwise unnecessary assertion.
Thanks for the extra context; it makes in-line review a breeze.
@@ -1504,34 +1505,38 @@ static void *qemudWorker(void *data) virMutexLock(&server->lock); while (((client = qemudPendingJob(server)) == NULL) && !worker->quitRequest) { if (virCondWait(&server->job, &server->lock) < 0) { virMutexUnlock(&server->lock); return NULL; } }
Indeed, the only way client can be NULL at this point is if worker->quitRequest is true...
if (worker->quitRequest) { if (client) virMutexUnlock(&client->lock); virMutexUnlock(&server->lock); return NULL; }
But that means we exit here.
worker->processingCall = 1; virMutexUnlock(&server->lock);
+ /* Tell clang we know what we're doing. + FIXME: remove when clang improves. */ + assert (client);
So this assertion is valid. ACK, if assert() is okay.
In general we really try to avoid assert() basically if we exit that means that the clients loose their access to their VM so that's a really really bad situation that we really try to avoid. Without refactoring assert() in that case would be acceptable because basically that would mean the compiler generated broken code and well, that something we don't try to gard against...
On the other hand, perhaps a more invasive rewrite would also work while also avoiding assert(), by hoisting the worker->quitRequest into the while loop, something like:
while ((client = qemudPendingJob(server)) == NULL) { if (worker->quitRequest || virCondWait(&server->job, &server->lock) < 0) { virMutexUnlock(&server->lock); return NULL; } } if (worker->quitRequest) { virMutexUnlock(&client->lock); virMutexUnlock(&server->lock); return NULL; }
Should I write that into patch format?
But if you can rewrite the loops to avoid the assert and keep clang happy, I think it's an even better solution, thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Eric Blake wrote:
According to Jim Meyering on 3/1/2010 10:13 AM:
Here's a case in which using an assertion appears to be the only way to tell clang that "client" really is non-NULL at that point. I'm sure clang's analyzers will eventually improve, and hence avoid this sort of false positive, so have marked this with a FIXME comment, to help ensure we eventually remove this otherwise unnecessary assertion. ... On the other hand, perhaps a more invasive rewrite would also work while also avoiding assert(), by hoisting the worker->quitRequest into the while loop, something like:
while ((client = qemudPendingJob(server)) == NULL) { if (worker->quitRequest || virCondWait(&server->job, &server->lock) < 0) { virMutexUnlock(&server->lock); return NULL; } } if (worker->quitRequest) { virMutexUnlock(&client->lock); virMutexUnlock(&server->lock); return NULL; }
Should I write that into patch format?
Yes, please. That is equivalent, placates clang, and is more readable. Thanks!
* daemon/libvirtd.c (qemudWorker): Rewrite loop to silence a warning. --- Here's the rewrite in patch form. daemon/libvirtd.c | 9 ++++----- 1 files changed, 4 insertions(+), 5 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 9bdbecb..9e16883 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -1502,16 +1502,15 @@ static void *qemudWorker(void *data) struct qemud_client_message *msg; virMutexLock(&server->lock); - while (((client = qemudPendingJob(server)) == NULL) && - !worker->quitRequest) { - if (virCondWait(&server->job, &server->lock) < 0) { + while ((client = qemudPendingJob(server)) == NULL) { + if (worker->quitRequest || + virCondWait(&server->job, &server->lock) < 0) { virMutexUnlock(&server->lock); return NULL; } } if (worker->quitRequest) { - if (client) - virMutexUnlock(&client->lock); + virMutexUnlock(&client->lock); virMutexUnlock(&server->lock); return NULL; } -- 1.6.6.1
Eric Blake wrote:
* daemon/libvirtd.c (qemudWorker): Rewrite loop to silence a warning. ---
Here's the rewrite in patch form.
daemon/libvirtd.c | 9 ++++----- 1 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 9bdbecb..9e16883 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -1502,16 +1502,15 @@ static void *qemudWorker(void *data) struct qemud_client_message *msg;
virMutexLock(&server->lock); - while (((client = qemudPendingJob(server)) == NULL) && - !worker->quitRequest) { - if (virCondWait(&server->job, &server->lock) < 0) { + while ((client = qemudPendingJob(server)) == NULL) { + if (worker->quitRequest || + virCondWait(&server->job, &server->lock) < 0) { virMutexUnlock(&server->lock); return NULL; } } if (worker->quitRequest) { - if (client) - virMutexUnlock(&client->lock); + virMutexUnlock(&client->lock); virMutexUnlock(&server->lock); return NULL; }
That's just what I'd tested. ACK. Thanks again. I'll push this tomorrow if no one beats me to it.
participants (5)
-
Daniel P. Berrange -
Daniel Veillard -
Eric Blake -
Jim Meyering -
Paolo Bonzini