Mailing-List: contact commons-dev-help@jakarta.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "Jakarta Commons Developers List" <commons-dev@jakarta.apache.org>
Received-SPF: pass (asf.osuosl.org: domain of oliver.zeigermann@gmail.com
 designates 64.233.182.192 as permitted sender)
DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws;
        s=beta; d=gmail.com;
        h=received:message-id:date:from:to:subject:in-reply-to:mime-version:content-type:content-transfer-encoding:content-disposition:references;
        b=DtNtWHi3CCEqpR5cmJIgUDBX0QwbC/JJwoYb62VuWXRZrg3/APDeKmEOFRnB+hJRzgTciubr2YI7TZKyof21Ubr/psCkTfLEx4u6LcvzHoj4InWQeN/DJxW0XzhVHkymIfosX3v06pn8QC6iFYr3CtICLD6vt52lX/mh/iH8o34=
Message-ID: <9da4f4520510230852j5c74097ei@mail.gmail.com>
Date: Sun, 23 Oct 2005 08:52:32 -0700
From: Oliver Zeigermann <oliver.zeigermann@gmail.com>
To: Jakarta Commons Developers List <commons-dev@jakarta.apache.org>
Subject: Re: [pool] synchronization issues in Pool
In-Reply-To: <Pine.LNX.4.62.0510222122120.17991@moa.Stanford.EDU>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable
Content-Disposition: inline
References: <Pine.LNX.4.62.0510222122120.17991@moa.Stanford.EDU>

Hi Mayur,

that really sounds like a cool tool. You results are also quite
impressive. I actually can not comment on them, but would like to know
if your tool is available anywhere?

If not - and I suppose so - could you also run it on the commons
transaction code where any race condition really would be very
harmful. I already know of one that has been fixed in CVS, but not in
a final release.

Thanks in advance

Oliver

2005/10/23, Mayur Naik <mhn@cs.stanford.edu>:
>
> Hello,
>
> I'm a PhD student in Computer Science at Stanford University, evaluating =
a
> static race detection tool I'm developing on open source Java programs.
> I ran it on the 5 implementations of pools in Apache Commons Pool, and
> found some bugs.  The output of the tool is here:
>
> http://suif.stanford.edu/~mhn/commons-pool-1.2/build/classes/PASS1/generi=
c/index.html
> http://suif.stanford.edu/~mhn/commons-pool-1.2/build/classes/PASS1/stack/=
index.html
> http://suif.stanford.edu/~mhn/commons-pool-1.2/build/classes/PASS1/softre=
f/index.html
> http://suif.stanford.edu/~mhn/commons-pool-1.2/build/classes/PASS1/generi=
c_keyed/index.html
> http://suif.stanford.edu/~mhn/commons-pool-1.2/build/classes/PASS1/stack_=
keyed/index.html
>
> I will explain one race in detail.  Following the [grouped by field] link
> on the first link above and then the [details] link under the heading
> "Races on field org.apache.commons.pool.BaseObjectPool: boolean closed"
> leads you to:
>
> http://suif.stanford.edu/~mhn/commons-pool-1.2/build/classes/PASS1/generi=
c/R8_trace.html
>
> Traversing the [up] links on this page yields at least one pair of paths
> in the call graph along which a common lock is not held and hence a race
> can occur if that pair of paths is executed by different threads:
>
> PATH 1:
>
> GenericHarness:27
> org.apache.commons.pool.impl.GenericObjectPool:853
> org.apache.commons.pool.BaseObjectPool:77
>
> leading to a read access of the field 'closed' at
> org.apache.commons.pool.BaseObjectPool:73
>
> PATH 2:
>
> GenericHarness:33
> org.apache.commons.pool.impl.GenericObjectPool:901
>
> leading to a write access of the field 'closed' at
> org.apache.commons.pool.BaseObjectPool:62
>
> [GenericHarness is a test harness that my tool automatically generates.]
>
> The above race is due to missing synchronization in method returnObject o=
f
> class GenericObjectPool.  In fact, it can cause a null pointer exception:
>
> thread 1 calls the returnObject method which executes the assertOpen
> method and finds the pool to be open.
>
> thread 2 calls the close method which sets _factory to null.
>
> thread 1 executes the addObjectToPool method which deferences _factory.
>
> Many other races are because parts of some methods implementing the pool
> interfaces (ex. invalidateObject below) that access _factory are not
> synchronized:
>
> public void invalidateObject(Object obj) throws Exception {
>       assertOpen();
>       try {
>           _factory.destroyObject(obj);
>       }
>       finally {
>           synchronized(this) {
>               _numActive--;
>               notifyAll();
>           }
>       }
> }
>
> Perhaps, the developer expects the client methods implementing the factor=
y
> interface to be synchronized.  If this is indeed the case, it should
> perhaps be documented somewhere so that unwitting users don't implement
> the factory interface without synchronization.  Even if users implement
> the factory interface with synchronization, there is a problem: if one
> thread executes the invalidateObject method and another executes the clos=
e
> method (which sets _factory to null), then there can be a null pointer
> exception similar to the scenario outlined above.
>
> Most of the races I found are harmful (ex. causing null pointer
> exceptions) if the close method is called concurrently with some other
> pool interface method.  Perhaps, it is highly unlikely that a client woul=
d
> do that.  However, since the pool interface is intended to be thread-safe=
,
> I think it's implementations should handle this scenario gracefully.
>
> I have summarized below all possible bugs I found by inspecting the above
> reports generated by my race detection tool.  I would be happy if you can
> confirm/refute these bugs.
>
> Thanks!
> -- Mayur
>
> org.apache.commons.pool.impl.GenericObjectPool
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>
> The returnObject(Object) and addObject() methods must be synchronized.
> The synchronization inside the body of the method addObjectToPool is then
> unnecessary since all callers of this method are synchronized.
>
> The entire invalidateObject(Object) method must be synchronized, not just
> parts of it.
>
> The entire borrowObject() method must be synchronized, not just parts of
> it.  Note that there are dereferences of _factory in the unsynchronized
> portions of this method, so there will be a null pointer dereference if i=
t
> is called concurrently with the close method.  Once the entire
> borrowObject method is synchronized, you can also move the assertOpen()
> call at the start of each iteration of the loop in this method to outside
> the loop.
>
> org.apache.commons.pool.impl.StackObjectPool
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>
> The entire returnObject(Object) method must be synchronized, not just
> parts of it.
>
> The getNumIdle() method must be synchronized.
>
> The getNumActive() method must be synchronized.
>
> The entire addObject() method must be synchronized, not just parts of it.
>
> org.apache.commons.pool.impl.SoftReferenceObjectPool
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D
>
> The entire returnObject(Object) method must be synchronized, not just
> parts of it.
>
> The entire addObject() method must be synchronized, not just parts of it.
>
> The getNumIdle() method must be synchronized.
>
> org.apache.commons.pool.impl.GenericKeyedObjectPool
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D
>
> The entire returnObject(Object,Object) method must be synchronized, not
> just parts of it.
>
> The entire invalidateObject(Object,Object) method must be synchronized,
> not just parts of it.
>
> The entire addObject(Object) method must be synchronized, not just parts
> of it.
>
> org.apache.commons.pool.impl.StackKeyedObjectPool
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>
> The entire addObject(Object) method must be synchronized, not just parts
> of it.
>
> The getNumActive(Object) method must be synchronized.
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: commons-dev-help@jakarta.apache.org
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org