6:39 p.m.
This is just a start to see if upstream likes it or not. If it turns out we
want this I can supply more patches - there are plenty of vir.*Free() functions
which doesn't set ptr to NULL.
My aim is to make it more visible where are we using stale pointer. Because if
the pointer is not set to NULL it may happen that the page isn't unmapped, so
on write access we are overwriting something else (perhaps glibc internal
structs). We may get SIGSEGV later on, but it will not show the root cause but
some random location instead. So you often have to run it under valgrind to get
to the real root cause. And I don't need to say how tricky is that, do I?
Of course, it won't catch every stale pointer bug we have, but my aim is to
catch a few at least.
Michal Privoznik (2):
virObjectUnref: Set pointer to NULL on dispose
virCommandFree: Set pointer to NULL
src/fdstream.c | 1 -
src/libvirt_private.syms | 4 ++--
src/qemu/qemu_driver.c | 1 -
src/storage/storage_backend_logical.c | 3 ---
src/util/vircommand.c | 11 ++++++++---
src/util/vircommand.h | 4 +++-
src/util/viridentity.c | 2 +-
src/util/virnetdevveth.c | 1 -
src/util/virobject.c | 13 ++++++++-----
src/util/virobject.h | 5 ++++-
10 files changed, 26 insertions(+), 19 deletions(-)
--
1.8.1.5
6:39 p.m.
New subject: [libvirt] [PATCH 1/2] virObjectUnref: Set pointer to NULL on dispose
Similarly to VIR_FREE() we can set the pointer passed to virObjectUnref
to NULL in case of disposing the object. However, to avoid overwriting
nearly thousands line of code, the virObjectUnref is turned into a macro
which passes the address of pointer and calls virObjectUnrefInternal
(the modified version of original virObjectUnref).
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/libvirt_private.syms | 2 +-
src/util/viridentity.c | 2 +-
src/util/virobject.c | 13 ++++++++-----
src/util/virobject.h | 5 ++++-
4 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 76016ca..25beda2 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1551,7 +1551,7 @@ virObjectLockableNew;
virObjectNew;
virObjectRef;
virObjectUnlock;
-virObjectUnref;
+virObjectUnrefInternal;
# util/virpci.h
diff --git a/src/util/viridentity.c b/src/util/viridentity.c
index 4f5127c..ae18f7c 100644
--- a/src/util/viridentity.c
+++ b/src/util/viridentity.c
@@ -60,7 +60,7 @@ static int virIdentityOnceInit(void)
return -1;
if (virThreadLocalInit(&virIdentityCurrent,
- (virThreadLocalCleanup)virObjectUnref) < 0) {
+ (virThreadLocalCleanup)virObjectUnrefInternal) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Cannot initialize thread local for current
identity"));
return -1;
diff --git a/src/util/virobject.c b/src/util/virobject.c
index 61b5413..dab2500 100644
--- a/src/util/virobject.c
+++ b/src/util/virobject.c
@@ -235,23 +235,25 @@ static void virObjectLockableDispose(void *anyobj)
}
/**
- * virObjectUnref:
+ * virObjectUnrefInternal:
* @anyobj: any instance of virObjectPtr
*
* Decrement the reference count on @anyobj and if
* it hits zero, runs the "dispose" callback associated
- * with the object class and frees @anyobj.
+ * with the object class, frees @anyobj and set it to NULL.
*
* Returns true if the remaining reference count is
* non-zero, false if the object was disposed of
*/
-bool virObjectUnref(void *anyobj)
+bool virObjectUnrefInternal(void **anyobj)
{
- virObjectPtr obj = anyobj;
+ virObjectPtr obj;
- if (!obj)
+ if (!anyobj || !*anyobj)
return false;
+ obj = *anyobj;
+
bool lastRef = virAtomicIntDecAndTest(&obj->refs);
PROBE(OBJECT_UNREF, "obj=%p", obj);
if (lastRef) {
@@ -268,6 +270,7 @@ bool virObjectUnref(void *anyobj)
obj->magic = 0xDEADBEEF;
obj->klass = (void*)0xDEADBEEF;
VIR_FREE(obj);
+ *anyobj = NULL;
}
return !lastRef;
diff --git a/src/util/virobject.h b/src/util/virobject.h
index 3a08f10..8dc50ba 100644
--- a/src/util/virobject.h
+++ b/src/util/virobject.h
@@ -69,7 +69,10 @@ bool virClassIsDerivedFrom(virClassPtr klass,
void *virObjectNew(virClassPtr klass)
ATTRIBUTE_NONNULL(1);
-bool virObjectUnref(void *obj);
+
+# define virObjectUnref(ptr) \
+ virObjectUnrefInternal((void *) (1 ? (const void *) &(ptr) : (ptr)))
+bool virObjectUnrefInternal(void **obj);
void *virObjectRef(void *obj);
bool virObjectIsClass(void *obj,
--
1.8.1.5
7:47 p.m.
New subject: [libvirt] [PATCH 1/2] virObjectUnref: Set pointer to NULL on dispose
On 11/07/2013 12:39 PM, Michal Privoznik wrote:
Similarly to VIR_FREE() we can set the pointer passed to
virObjectUnref
to NULL in case of disposing the object. However, to avoid overwriting
nearly thousands line of code, the virObjectUnref is turned into a macro
which passes the address of pointer and calls virObjectUnrefInternal
(the modified version of original virObjectUnref).
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
+
+# define virObjectUnref(ptr) \
+ virObjectUnrefInternal((void *) (1 ? (const void *) &(ptr) : (ptr)))
This could help catch some bugs caused by using stale pointers, that
part is good.
I normally don't expect a C function call to have side effects on
something that is passed by value rather than reference. Not the case
for macros, where everything is fair game. But in libvirt we generally
follow the convention of defining macros in ALL_CAPS() to provide a
simple visual cue that there may be side effects.
I do recognize though that updating every use of every vir*Free in the
tree would be a huge diff and cause lots of merge conflicts later when
anyone tried to backport anything. So as usual I'm on the fence about this.
1:27 p.m.
New subject: [libvirt] [PATCH 1/2] virObjectUnref: Set pointer to NULL on dispose
On Thu, Nov 07, 2013 at 11:39:27AM +0100, Michal Privoznik wrote:
Similarly to VIR_FREE() we can set the pointer passed to
virObjectUnref
to NULL in case of disposing the object. However, to avoid overwriting
nearly thousands line of code, the virObjectUnref is turned into a macro
which passes the address of pointer and calls virObjectUnrefInternal
(the modified version of original virObjectUnref).
I have to say I'm not really liking this, and your impl is not race
free since you're not atomically updating the point.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
3:54 p.m.
New subject: [libvirt] [PATCH 1/2] virObjectUnref: Set pointer to NULL on dispose
On 08.11.2013 06:27, Daniel P. Berrange wrote:
On Thu, Nov 07, 2013 at 11:39:27AM +0100, Michal Privoznik wrote:
> Similarly to VIR_FREE() we can set the pointer passed to virObjectUnref
> to NULL in case of disposing the object. However, to avoid overwriting
> nearly thousands line of code, the virObjectUnref is turned into a macro
> which passes the address of pointer and calls virObjectUnrefInternal
> (the modified version of original virObjectUnref).
I have to say I'm not really liking this, and your impl is not race
free since you're not atomically updating the point.
Daniel
I don't think I follow you there. AFAIU, the whole 'if' body is executed
exactly once iff obj->refs is zero after decrement. And I don't see how
can I possibly race with others.
If two threads calls virObjectUnref on the very same object with
refcount = 1, do you expect them both to have the *ptr = NULL?
Michal
6:40 p.m.
New subject: [libvirt] [PATCH 1/2] virObjectUnref: Set pointer to NULL on dispose
On Fri, Nov 08, 2013 at 08:54:39AM +0100, Michal Privoznik wrote:
On 08.11.2013 06:27, Daniel P. Berrange wrote:
> On Thu, Nov 07, 2013 at 11:39:27AM +0100, Michal Privoznik wrote:
>> Similarly to VIR_FREE() we can set the pointer passed to virObjectUnref
>> to NULL in case of disposing the object. However, to avoid overwriting
>> nearly thousands line of code, the virObjectUnref is turned into a macro
>> which passes the address of pointer and calls virObjectUnrefInternal
>> (the modified version of original virObjectUnref).
>
> I have to say I'm not really liking this, and your impl is not race
> free since you're not atomically updating the point.
>
> Daniel
>
I don't think I follow you there. AFAIU, the whole 'if' body is executed
exactly once iff obj->refs is zero after decrement. And I don't see how
can I possibly race with others.
If two threads calls virObjectUnref on the very same object with
refcount = 1, do you expect them both to have the *ptr = NULL?
The first thread decrements refcount, so it hits zero. Now a short time
later it sets *obj = NULL, but at the same time another thread is running
virObjectUnref(). It will check *obj != NULL, which races with setting
*obj = NULL.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
7:19 p.m.
New subject: [libvirt] [PATCH 1/2] virObjectUnref: Set pointer to NULL on dispose
On Mon, Nov 11, 2013 at 10:40:11AM +0000, Daniel P. Berrange wrote:
On Fri, Nov 08, 2013 at 08:54:39AM +0100, Michal Privoznik wrote:
> On 08.11.2013 06:27, Daniel P. Berrange wrote:
> > On Thu, Nov 07, 2013 at 11:39:27AM +0100, Michal Privoznik wrote:
> >> Similarly to VIR_FREE() we can set the pointer passed to virObjectUnref
> >> to NULL in case of disposing the object. However, to avoid overwriting
> >> nearly thousands line of code, the virObjectUnref is turned into a macro
> >> which passes the address of pointer and calls virObjectUnrefInternal
> >> (the modified version of original virObjectUnref).
> >
> > I have to say I'm not really liking this, and your impl is not race
> > free since you're not atomically updating the point.
> >
> > Daniel
> >
>
>
> I don't think I follow you there. AFAIU, the whole 'if' body is
executed
> exactly once iff obj->refs is zero after decrement. And I don't see how
> can I possibly race with others.
>
> If two threads calls virObjectUnref on the very same object with
> refcount = 1, do you expect them both to have the *ptr = NULL?
The first thread decrements refcount, so it hits zero. Now a short time
later it sets *obj = NULL, but at the same time another thread is running
virObjectUnref(). It will check *obj != NULL, which races with setting
*obj = NULL.
IIUC, the only problem here is our invalid usage, two threads both
working with one object that has refcount = 1, right? The race can
happen either way if you are in such kind of situation (and have
VIR_FREE() after virObjectUnref()).
Martin
7:47 p.m.
New subject: [libvirt] [PATCH 1/2] virObjectUnref: Set pointer to NULL on dispose
On Mon, Nov 11, 2013 at 12:19:04PM +0100, Martin Kletzander wrote:
On Mon, Nov 11, 2013 at 10:40:11AM +0000, Daniel P. Berrange wrote:
> On Fri, Nov 08, 2013 at 08:54:39AM +0100, Michal Privoznik wrote:
> > On 08.11.2013 06:27, Daniel P. Berrange wrote:
> > > On Thu, Nov 07, 2013 at 11:39:27AM +0100, Michal Privoznik wrote:
> > >> Similarly to VIR_FREE() we can set the pointer passed to
virObjectUnref
> > >> to NULL in case of disposing the object. However, to avoid
overwriting
> > >> nearly thousands line of code, the virObjectUnref is turned into a
macro
> > >> which passes the address of pointer and calls virObjectUnrefInternal
> > >> (the modified version of original virObjectUnref).
> > >
> > > I have to say I'm not really liking this, and your impl is not race
> > > free since you're not atomically updating the point.
> > >
> > > Daniel
> > >
> >
> >
> > I don't think I follow you there. AFAIU, the whole 'if' body is
executed
> > exactly once iff obj->refs is zero after decrement. And I don't see how
> > can I possibly race with others.
> >
> > If two threads calls virObjectUnref on the very same object with
> > refcount = 1, do you expect them both to have the *ptr = NULL?
>
> The first thread decrements refcount, so it hits zero. Now a short time
> later it sets *obj = NULL, but at the same time another thread is running
> virObjectUnref(). It will check *obj != NULL, which races with setting
> *obj = NULL.
>
IIUC, the only problem here is our invalid usage, two threads both
working with one object that has refcount = 1, right? The race can
happen either way if you are in such kind of situation (and have
VIR_FREE() after virObjectUnref()).
Yes, it would be a case of broken caller ref count usage. Your patch,
however, is claiming to protect against such broken callers. With the
race here, the protection does not actually work reliably. What we
would need is to atomically clear & check the pointer, as well as the
ref count.
I'm still, however, not happy about the idea of silently ignoring bugs
in reference counting though. If two threads try to unref an object
which has a refcount==1, I'd prefer to see some kind of diagnostic
that highlights the flaw in the calling code.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
6:39 p.m.
New subject: [libvirt] [PATCH 2/2] virCommandFree: Set pointer to NULL
Just like we have VIR_FREE() which sets pointer to NULL,
we should have virCommandFree() to act the same.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/fdstream.c | 1 -
src/libvirt_private.syms | 2 +-
src/qemu/qemu_driver.c | 1 -
src/storage/storage_backend_logical.c | 3 ---
src/util/vircommand.c | 11 ++++++++---
src/util/vircommand.h | 4 +++-
src/util/virnetdevveth.c | 1 -
7 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/src/fdstream.c b/src/fdstream.c
index f7dae39..4acb821 100644
--- a/src/fdstream.c
+++ b/src/fdstream.c
@@ -321,7 +321,6 @@ virFDStreamCloseInt(virStreamPtr st, bool streamAbort)
ret = -1;
}
virCommandFree(fdst->cmd);
- fdst->cmd = NULL;
}
if (VIR_CLOSE(fdst->errfd) < 0)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 25beda2..1ecef26 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1065,7 +1065,7 @@ virCommandClearCaps;
virCommandDaemonize;
virCommandDoAsyncIO;
virCommandExec;
-virCommandFree;
+virCommandFreeInternal;
virCommandHandshakeNotify;
virCommandHandshakeWait;
virCommandNew;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 9c3daad..688c535 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -11605,7 +11605,6 @@ qemuDomainSnapshotCreateInactiveExternal(virQEMUDriverPtr driver,
goto cleanup;
virCommandFree(cmd);
- cmd = NULL;
}
/* update disk definitions */
diff --git a/src/storage/storage_backend_logical.c
b/src/storage/storage_backend_logical.c
index 10966cc..ddeec29 100644
--- a/src/storage/storage_backend_logical.c
+++ b/src/storage/storage_backend_logical.c
@@ -644,7 +644,6 @@ virStorageBackendLogicalDeletePool(virConnectPtr conn
ATTRIBUTE_UNUSED,
if (virCommandRun(cmd, NULL) < 0)
goto cleanup;
virCommandFree(cmd);
- cmd = NULL;
/* now remove the pv devices and clear them out */
ret = 0;
@@ -657,7 +656,6 @@ virStorageBackendLogicalDeletePool(virConnectPtr conn
ATTRIBUTE_UNUSED,
break;
}
virCommandFree(cmd);
- cmd = NULL;
}
cleanup:
@@ -720,7 +718,6 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn,
goto error;
virCommandFree(cmd);
- cmd = NULL;
if ((fd = virStorageBackendVolOpen(vol->target.path)) < 0)
goto error;
diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index 8dcf9e7..2f8c534 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -2561,7 +2561,7 @@ int virCommandHandshakeNotify(virCommandPtr cmd)
/**
- * virCommandFree:
+ * virCommandFreeInternal:
* @cmd: optional command to free
*
* Release all resources. The only exception is that if you called
@@ -2569,12 +2569,16 @@ int virCommandHandshakeNotify(virCommandPtr cmd)
* is not reaped, and you must call virProcessWait() or virProcessAbort() yourself.
*/
void
-virCommandFree(virCommandPtr cmd)
+virCommandFreeInternal(virCommandPtr *ptr)
{
+ virCommandPtr cmd;
size_t i;
- if (!cmd)
+
+ if (!ptr || !*ptr)
return;
+ cmd = *ptr;
+
for (i = 0; i < cmd->npassfd; i++) {
if (cmd->passfd[i].flags & VIR_COMMAND_PASS_FD_CLOSE_PARENT)
VIR_FORCE_CLOSE(cmd->passfd[i].fd);
@@ -2621,6 +2625,7 @@ virCommandFree(virCommandPtr cmd)
#endif
VIR_FREE(cmd);
+ *ptr = NULL;
}
/**
diff --git a/src/util/vircommand.h b/src/util/vircommand.h
index e977f93..ce7e9ab 100644
--- a/src/util/vircommand.h
+++ b/src/util/vircommand.h
@@ -181,7 +181,9 @@ int virCommandHandshakeNotify(virCommandPtr cmd)
void virCommandAbort(virCommandPtr cmd);
-void virCommandFree(virCommandPtr cmd);
+# define virCommandFree(ptr) \
+ virCommandFreeInternal(&(ptr))
+void virCommandFreeInternal(virCommandPtr *cmd);
void virCommandDoAsyncIO(virCommandPtr cmd);
#endif /* __VIR_COMMAND_H__ */
diff --git a/src/util/virnetdevveth.c b/src/util/virnetdevveth.c
index 25eb282..737563f 100644
--- a/src/util/virnetdevveth.c
+++ b/src/util/virnetdevveth.c
@@ -171,7 +171,6 @@ int virNetDevVethCreate(char** veth1, char** veth2)
VIR_FREE(veth1auto);
VIR_FREE(veth2auto);
virCommandFree(cmd);
- cmd = NULL;
}
virReportError(VIR_ERR_INTERNAL_ERROR,
--
1.8.1.5
4007
days inactive
4011
days old
8 comments
4 participants
participants (4)
-
Daniel P. Berrange -
Laine Stump -
Martin Kletzander -
Michal Privoznik