Bug 56784

Summary: Possible atomicity violations
Product: Tomcat 6 Reporter: diogogsousa
Component: CatalinaAssignee: Tomcat Developers Mailing List <dev>
Status: RESOLVED FIXED    
Severity: normal    
Priority: P2    
Version: 6.0.41   
Target Milestone: default   
Hardware: All   
OS: All   

Description diogogsousa 2014-07-29 07:28:44 UTC
I'm developing a tool for atomicity violation detection and I think it have found a few atomicity violations in catalina.  These are issues are due to non-atomic compositions of calls to a certain object.

In org.apache.catalina.core.ContainerBase, method getChildren(), lines 1526-1534:

>     public ObjectName[] getChildren() {
> 1526:   ObjectName result[]=new ObjectName[children.size()];
>         Iterator it=children.values().iterator();
>         int i=0;
>         while( it.hasNext() ) {
>             Object next=it.next();
>             if( next instanceof ContainerBase ) {
> 1531:            result[i++]=((ContainerBase)next).getJmxName();
>             }
>         }
>         return result;
>     }

Here "result" is alloced with size "children.size()".  A concurrent thread may add more objects to "children" possibly causing an invalid access to array "result" in line 1531.

__

In org.apache.catalina.core.StandardEngine, method start(), lines 438-439:

>     public void start() throws LifecycleException {
>         ...
> 438:            if( mserver.isRegistered(realmName ) ) {
> 439:                mserver.invoke(realmName, "init", 
>                             new Object[] {},
>                             new String[] {}
>                     );            
>                 }
>         ...
>     }

Can the "init" method be unregistered between lines 438 and 439?

__

In org.apache.catalina.core.StandardContext, method addParameter(), lines 2659-2672:

>     public void addParameter(String name, String value) {
>         ...
> 2664:   if (parameters.get(name) != null)
>             throw new IllegalArgumentException
>                 (sm.getString("standardContext.parameter.duplicate", name));
> 
>         // Add this parameter to our defined set
>         synchronized (parameters) {
> 2670:       parameters.put(name, value);
>         }
>         fireContainerEvent("addParameter", name);
>     }

A concurrent thread may add the same parameter between the execution of line 2664 and 2670.  In that case both threads will think they succeeded, when the expected behavior is for one of them to receive the exception thrown in line 2665.

Thank you
Comment 1 Mark Thomas 2014-08-05 15:04:23 UTC
(In reply to diogogsousa from comment #0)
> In org.apache.catalina.core.ContainerBase, method getChildren(), lines
> 1526-1534:

Technically this is valid although no-one has ever reported an issue. It is likely that the error would only be seen by JMX clients and a refresh would fix it. I'll get this fixed in 8.0.x for 8.0.11 onwards.

> In org.apache.catalina.core.StandardEngine, method start(), lines 438-439:

This only affects 6.0.x and I don't see any way for this to occur in normal (or even fairly abnormal) usage. It is probably possible to trigger this if you try really hard but I don't see a need for changes here.

> In org.apache.catalina.core.StandardContext, method addParameter(), lines
> 2659-2672:

Again this one is valid but no-one has reported an issue. I wouldn't expect to see an issue in normal usage and concurrent threads configuring a Context would be pretty unusual. A ConcurrentHashMap looks like it will be cleaner here so I'll get that fixed in 8.0.x for 8.0.11 as well.

In terms of back-ports, given that we haven't had reports of any issues, I'm leaning towards only fixing these in 8.0.x and not back-porting the changes to earlier versions.
Comment 2 Mark Thomas 2014-08-05 15:15:46 UTC
Fixes applied to 8.0.x as discussed for 8.0.11 onwards.

I do not plan on back-porting them.