You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Mayur Naik <mh...@cs.stanford.edu> on 2005/10/23 10:04:41 UTC
[pool] synchronization issues in Pool
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/generic/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/softref/index.html
http://suif.stanford.edu/~mhn/commons-pool-1.2/build/classes/PASS1/generic_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/generic/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 of
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 factory
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 close
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 would
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
==============================================
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 it
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
============================================
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
====================================================
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
===================================================
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
=================================================
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
Re: [pool] synchronization issues in Pool
Posted by Oliver Zeigermann <ol...@gmail.com>.
Cool, looking forward to both the release of you tool as well as your
report for commons transaction :)
Cheers and thanks
Oliver
2005/10/25, Mayur Naik <ma...@gmail.com>:
> Hi Oliver,
>
> The tool is still a research prototype but I intend to release it in
> the near future. I took a quick look and it seems like my tool might
> not be very useful on the commons transaction code because that code
> uses sophisticated locking primitives which the current version of my
> tool does not handle and hence will produce lots of false alarms
> (though we have plans to extend it to handle those primitives).
> Nevertheless, I will run my tool on that code in the near future and
> get back to you if I find any issues.
>
> Best,
> -- Mayur
>
> On 10/23/05, Oliver Zeigermann <ol...@gmail.com> wrote:
> > 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
>
> ---------------------------------------------------------------------
> 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
Re: [pool] synchronization issues in Pool
Posted by robert burrell donkin <ro...@blueyonder.co.uk>.
On Mon, 2005-10-24 at 17:56 -0700, Mayur Naik wrote:
> Hi Oliver,
>
> The tool is still a research prototype but I intend to release it in
> the near future.
cool
please drop us a note here when you do :)
- robert
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [pool] synchronization issues in Pool
Posted by Mayur Naik <ma...@gmail.com>.
Hi Oliver,
The tool is still a research prototype but I intend to release it in
the near future. I took a quick look and it seems like my tool might
not be very useful on the commons transaction code because that code
uses sophisticated locking primitives which the current version of my
tool does not handle and hence will produce lots of false alarms
(though we have plans to extend it to handle those primitives).
Nevertheless, I will run my tool on that code in the near future and
get back to you if I find any issues.
Best,
-- Mayur
On 10/23/05, Oliver Zeigermann <ol...@gmail.com> wrote:
> 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
---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org
Re: [pool] synchronization issues in Pool
Posted by Oliver Zeigermann <ol...@gmail.com>.
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 <mh...@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/generic/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/softref/index.html
> http://suif.stanford.edu/~mhn/commons-pool-1.2/build/classes/PASS1/generic_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/generic/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 of
> 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 factory
> 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 close
> 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 would
> 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
> ==============================================
>
> 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 it
> 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
> ============================================
>
> 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
> ====================================================
>
> 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
> ===================================================
>
> 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
> =================================================
>
> 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