[libvirt] [PATCH] nwfilter: Fix the test for the result of atomic dec and test

[Stefan Berger]

Martin Kletzander <mkletzan at redhat.com> wrote on 03/19/2014 09:13:51 AM:

> 
> On Tue, Mar 18, 2014 at 09:45:14PM -0400, Stefan Berger wrote:
> > From: Stefan Berger <stefanb at linux.vnet.ibm.com>
> >
> > https://bugzilla.redhat.com/show_bug.cgi?id=1071181
> >
> > Commit 49b59a15 fixed one problem but masks another one related to 
pointer
> > freeing.
> >
> > Use virAtomicIntGet() to test for 0 rather than trying to test for 
'true'
> > after  virAtomicIntDecAndTest().
> >
> > Avoid putting of the virNWFilterSnoopReq once the thread has been 
started.
> >
> > Signed-off-by: Stefan Berger <stefanb at linux.vnet.ibm.com>
> > ---
> >  src/nwfilter/nwfilter_dhcpsnoop.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/
> nwfilter_dhcpsnoop.c
> > index d2a8062..32ca304 100644
> > --- a/src/nwfilter/nwfilter_dhcpsnoop.c
> > +++ b/src/nwfilter/nwfilter_dhcpsnoop.c
> > @@ -720,7 +720,10 @@ virNWFilterSnoopReqPut(virNWFilterSnoopReqPtr 
req)
> >
> >      virNWFilterSnoopLock();
> >
> > -    if (virAtomicIntDecAndTest(&req->refctr)) {
> > +    virAtomicIntDecAndTest(&req->refctr);
> > +
> > +    /* make sure it's 0; virAtomitIntDecAndTest may return true on 
'1' */
> > +    if (virAtomicIntGet(&req->refctr) == 0) {
> 
> NACK, using two atomic functions, you are making this non-atomic.
> between these two atomic operations many things can happen an it's not
> what you want I bet.  The virAtomicIntDecAndTest() uses
> __sync_fetch_and_sub(atomic, 1) and compares it to 1, that's true, but
> since it is fetch_and_sub (and not the other way around, the value
> being compared to 1 is the value that the atomic had before it was
> decremented.  That means it returns 1 (true) if and only if the
> current value is 0.  virAtomicIntDecAndTest() is what you really want
> and should use here.

Right. I initially thought there was something wrong in this part here 
until I found the double free error is related to the other part. So this 
part you comment on doesn't need to be changed.

   Stefan
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140319/9c83aee8/attachment-0001.htm>