Bug 50138

Summary:	Lack of synchronization in org.apache.catalina.security.SecurityUtil
Product:	Tomcat 6	Reporter:	Dmitry Mikhaylov <mikhailov.dmitry>
Component:	Catalina	Assignee:	Tomcat Developers Mailing List <dev>
Status:	RESOLVED FIXED
Severity:	normal
Priority:	P2
Version:	6.0.29
Target Milestone:	default
Hardware:	PC
OS:	Linux

Description Dmitry Mikhaylov 2010-10-21 09:22:56 UTC

Symptom: all processor threads spin madly in:
==============
"tomcat-processor-20" daemon prio=10 tid=0x09210800 nid=0x51fb runnable [0x61b76000]
   java.lang.Thread.State: RUNNABLE
	at java.util.HashMap.getEntry(HashMap.java:347)
	at java.util.HashMap.containsKey(HashMap.java:335)
	at org.apache.catalina.security.SecurityUtil.doAsPrivilege(SecurityUtil.java:227)
	at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:230)
	at org.apache.catalina.core.ApplicationFilterChain.access$000(ApplicationFilterChain.java:56)
	at org.apache.catalina.core.ApplicationFilterChain$1.run(ApplicationFilterChain.java:189)
	at java.security.AccessController.doPrivileged(Native Method)
	at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:185)
...
==============

Cause: org.apache.catalina.security.SecurityUtil.objectCache is a HashMap, but access to it is not synchronized. The javadoc for HashMap says:
=============
Note that this implementation is not synchronized. If multiple threads access a hash map concurrently, and at least one of the threads modifies the map structurally, it must be synchronized externally.
=============

Proposed solution: change objectCache to ConcurrentHashMap;

Comment 1 Sebb 2010-10-21 09:51:59 UTC

That may not be the only bug. 

There are two instances of the following code:

if(objectCache.containsKey(targetObject)){
            methodsCache = objectCache.get(targetObject);

If the object is removed between the two statements, then an NPE will follow.

Surely the code should just check whether it got a non-null object?

Also, the private static fields should be final.

Comment 2 Mark Thomas 2010-10-21 12:02:00 UTC

Thanks for the report.

Fixed in trunk for 7.0.5 onwards and proposed for 6.0.x

Comment 3 Mark Thomas 2010-10-25 11:46:19 UTC

Fixed in 6.0.x and will be included in 6.0.30 onwards.

Comment 4 Dmitry Mikhaylov 2010-10-27 02:44:00 UTC

Thanks for prompt fix, waiting for 6.0.30.