[libvirt] [PATCH 1/2] virObjectUnref: Set pointer to NULL on dispose

[Daniel P. Berrange]

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