11:13 a.m.
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(a)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
11:27 a.m.
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
11:36 a.m.
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
11:46 a.m.
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 :|
11:42 a.m.
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
11:56 a.m.
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!
12:59 p.m.
New subject: [libvirt] [PATCH] libvirtd: avoid false-positive NULL-deref warning from clang
* 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
2:44 p.m.
New subject: [libvirt] [PATCH] libvirtd: avoid false-positive NULL-deref warning from clang
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.
5414
days inactive
5414
days old
7 comments
5 participants
participants (5)
-
Daniel P. Berrange -
Daniel Veillard -
Eric Blake -
Jim Meyering -
Paolo Bonzini