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 <[EMAIL PROTECTED]>: > > 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: [EMAIL PROTECTED] > For additional commands, e-mail: [EMAIL PROTECTED] > > --------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
