[libvirt] [PATCH 0/2] Make vir*Free to set ptr to NULL
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
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@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
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@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.
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 :|
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
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 :|
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
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 :|
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@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
participants (4)
-
Daniel P. Berrange -
Laine Stump -
Martin Kletzander -
Michal Privoznik