Bug 49985 - Lazy initialization without any synchronization - data race in AstInteger, AstFloatingPoint, AstString
Summary: Lazy initialization without any synchronization - data race in AstInteger, As...
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 6
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 6.0.29
Hardware: PC All
: P2 normal (vote)
Target Milestone: default
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-23 08:55 UTC by Sergey Vorobyev
Modified: 2010-10-04 17:16 UTC (History)
2 users (show)



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Sergey Vorobyev 2010-09-23 08:55:42 UTC
r998053
http://svn.apache.org/repos/asf/tomcat/tc6.0.x/trunk/java/org/apache/el/parser/AstInteger.java
http://svn.apache.org/repos/asf/tomcat/tc6.0.x/trunk/java/org/apache/el/parser/AstFloatingPoint.java
http://svn.apache.org/repos/asf/tomcat/tc6.0.x/trunk/java/org/apache/el/parser/AstString.java

Use lazy init without any synchronization in methods getInteger(), getFloatingPoint(), getString(), respectively.
 
Consider AstInteger:

private Number number;

    protected Number getInteger() {
        if (this.number == null) {
            try {
                this.number = new Long(this.image);
            } catch (ArithmeticException e1) {
                this.number = new BigInteger(this.image);
            }
        }
        return number;
    }


Data races on variable number :37
in method getInteger() :39
concurrent read on line 40
concurrent write on lines 42, 44
Comment 1 Mark Thomas 2010-09-30 18:27:27 UTC
I don't see any way this could cause a problem.

If you have a scenario where this could lead to unexpected behaviour, feel free to re-open this issue and add an explanation.
Comment 2 Sergey Vorobyev 2010-10-01 04:06:16 UTC
First and most obviously, two or more Long/BigInteger objects could be allocated. It's not dangerous, but not good.

Second consider following:

Thread1:
protected Number getInteger() {
        if (this.number == null) {
            try {
->              this.number = new Long(this.image); // this.number isn't null, but not fully complete.

Thread2:
protected Number getInteger() {
        if (this.number == null) { // break, because this.number isn't null already.
            ...
        }
->      return number; // publish not fully complete object link.
    }

For more reasons see http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html
(It's about dcl, but about lazy initialization generally two. First example become to this code exactly)
Comment 3 Sebb 2010-10-01 07:56:05 UTC
AIUI making "number" volatile would prevent the re-ordering that can result in the object reference being set up before construction is complete. Yes?
Comment 4 Sergey Vorobyev 2010-10-01 08:07:19 UTC
(In reply to comment #3)
> AIUI making "number" volatile would prevent the re-ordering that can result in
> the object reference being set up before construction is complete. Yes?

Yes, the reason is object reference being set up before construction is complete. One of possible solutions is making "number" volatile.
Comment 5 Mark Thomas 2010-10-01 08:15:36 UTC
I've yet to be convinced that there is an real issue here. I don't see any multi-threaded code paths into instances of those classes.

If we change anything (on the basis there might be a way for multiple threads to use one of these objects at the same time) then using volatile should prevent misbehaviour with minimal impact. I'm not concerned about possible additional object creation.
Comment 6 Sergey Vorobyev 2010-10-01 08:28:06 UTC
Lock at this stack-traces to see how this method can be on invoked from a number of threads:
(I'm using dacapo tomcat benchmark)

AstInteger:

   Thread 22
    #0  org/apache/el/parser/AstInteger.getInteger AstInteger.java:42
    #1  org/apache/el/parser/AstInteger.getValue AstInteger.java:57
    #2  org/apache/el/parser/AstDiv.getValue AstDiv.java:39
    #3  org/apache/el/parser/AstGreaterThan.getValue AstGreaterThan.java:41
    #4  org/apache/el/ValueExpressionImpl.getValue ValueExpressionImpl.java:186
    #5  org/apache/jasper/runtime/PageContextImpl.proprietaryEvaluate PageContextImpl.java:935
    #6  org/apache/jsp/jsp/jsp2/el/basic_002dcomparisons_jsp._jspService basic_002dcomparisons_jsp.java:115
    #7  org/apache/jasper/runtime/HttpJspBase.service HttpJspBase.java:70
    #8  javax/servlet/http/HttpServlet.service HttpServlet.java:717
    #9  org/apache/jasper/servlet/JspServletWrapper.service JspServletWrapper.java:377
    #10 org/apache/jasper/servlet/JspServlet.serviceJspFile JspServlet.java:313
    #11 org/apache/jasper/servlet/JspServlet.service JspServlet.java:260
  
 Thread 28
    #0  org/apache/el/parser/AstInteger.getInteger AstInteger.java:40
    #1  org/apache/el/parser/AstInteger.getValue AstInteger.java:57
    #2  org/apache/el/parser/AstDiv.getValue AstDiv.java:39
    #3  org/apache/el/parser/AstGreaterThan.getValue AstGreaterThan.java:41
    #4  org/apache/el/ValueExpressionImpl.getValue ValueExpressionImpl.java:186
    #5  org/apache/jasper/runtime/PageContextImpl.proprietaryEvaluate PageContextImpl.java:935
    #6  org/apache/jsp/jsp/jsp2/el/basic_002dcomparisons_jsp._jspService basic_002dcomparisons_jsp.java:115
    #7  org/apache/jasper/runtime/HttpJspBase.service HttpJspBase.java:70
    #8  javax/servlet/http/HttpServlet.service HttpServlet.java:717
    #9  org/apache/jasper/servlet/JspServletWrapper.service JspServletWrapper.java:377
  

AstFloatingPoint:

   Thread 22
    #0  org/apache/el/parser/AstFloatingPoint.getFloatingPoint AstFloatingPoint.java:40
    #1  org/apache/el/parser/AstFloatingPoint.getValue AstFloatingPoint.java:52
    #2  org/apache/el/parser/AstGreaterThanEqual.getValue AstGreaterThanEqual.java:37
    #3  org/apache/el/ValueExpressionImpl.getValue ValueExpressionImpl.java:186
    #4  org/apache/jasper/runtime/PageContextImpl.proprietaryEvaluate PageContextImpl.java:935
    #5  org/apache/jsp/jsp/jsp2/el/basic_002dcomparisons_jsp._jspService basic_002dcomparisons_jsp.java:121
    #6  org/apache/jasper/runtime/HttpJspBase.service HttpJspBase.java:70
    #7  javax/servlet/http/HttpServlet.service HttpServlet.java:717
    #8  org/apache/jasper/servlet/JspServletWrapper.service JspServletWrapper.java:377
    #9  org/apache/jasper/servlet/JspServlet.serviceJspFile JspServlet.java:313
    #10 org/apache/jasper/servlet/JspServlet.service JspServlet.java:260
    #11 javax/servlet/http/HttpServlet.service HttpServlet.java:717
  
 Thread 28
    #0  org/apache/el/parser/AstFloatingPoint.getFloatingPoint AstFloatingPoint.java:42
    #1  org/apache/el/parser/AstFloatingPoint.getValue AstFloatingPoint.java:52
    #2  org/apache/el/parser/AstGreaterThanEqual.getValue AstGreaterThanEqual.java:37
    #3  org/apache/el/ValueExpressionImpl.getValue ValueExpressionImpl.java:186
    #4  org/apache/jasper/runtime/PageContextImpl.proprietaryEvaluate PageContextImpl.java:935
    #5  org/apache/jsp/jsp/jsp2/el/basic_002dcomparisons_jsp._jspService basic_002dcomparisons_jsp.java:121
    #6  org/apache/jasper/runtime/HttpJspBase.service HttpJspBase.java:70
    #7  javax/servlet/http/HttpServlet.service HttpServlet.java:717
    #8  org/apache/jasper/servlet/JspServletWrapper.service JspServletWrapper.java:377
    #9  org/apache/jasper/servlet/JspServlet.serviceJspFile JspServlet.java:313
 
AstString:
   Thread 22
    #0  org/apache/el/parser/AstString.getString AstString.java:38
    #1  org/apache/el/parser/AstString.getValue AstString.java:51
    #2  org/apache/el/parser/AstLessThan.getValue AstLessThan.java:37
    #3  org/apache/el/ValueExpressionImpl.getValue ValueExpressionImpl.java:186
    #4  org/apache/jasper/runtime/PageContextImpl.proprietaryEvaluate PageContextImpl.java:935
    #5  org/apache/jsp/jsp/jsp2/el/basic_002dcomparisons_jsp._jspService basic_002dcomparisons_jsp.java:179
    #6  org/apache/jasper/runtime/HttpJspBase.service HttpJspBase.java:70
    #7  javax/servlet/http/HttpServlet.service HttpServlet.java:717
    #8  org/apache/jasper/servlet/JspServletWrapper.service JspServletWrapper.java:377
    #9  org/apache/jasper/servlet/JspServlet.serviceJspFile JspServlet.java:313
    #10 org/apache/jasper/servlet/JspServlet.service JspServlet.java:260
    #11 javax/servlet/http/HttpServlet.service HttpServlet.java:717
 
  Thread 28
    #0  org/apache/el/parser/AstString.getString AstString.java:39
    #1  org/apache/el/parser/AstString.getValue AstString.java:51
    #2  org/apache/el/parser/AstLessThan.getValue AstLessThan.java:37
    #3  org/apache/el/ValueExpressionImpl.getValue ValueExpressionImpl.java:186
    #4  org/apache/jasper/runtime/PageContextImpl.proprietaryEvaluate PageContextImpl.java:935
    #5  org/apache/jsp/jsp/jsp2/el/basic_002dcomparisons_jsp._jspService basic_002dcomparisons_jsp.java:179
    #6  org/apache/jasper/runtime/HttpJspBase.service HttpJspBase.java:70
    #7  javax/servlet/http/HttpServlet.service HttpServlet.java:717
    #8  org/apache/jasper/servlet/JspServletWrapper.service JspServletWrapper.java:377
    #9  org/apache/jasper/servlet/JspServlet.serviceJspFile JspServlet.java:313
Comment 7 Chuck Caldarale 2010-10-01 09:25:13 UTC
(In reply to comment #6)
> Lock at this stack-traces to see how this method can be on invoked from a
> number of threads:

But is each thread in the pair using the same instance?  There's no evidence of that in the published trace.
Comment 8 Sergey Vorobyev 2010-10-01 09:34:45 UTC
(In reply to comment #7)
> (In reply to comment #6)
> > Lock at this stack-traces to see how this method can be on invoked from a
> > number of threads:
> 
> But is each thread in the pair using the same instance?  There's no evidence of
> that in the published trace.

I omit first line of race report, sorry.

WARNING: Possible data race during write of size 1 at 0x76a1569c: {{{
 ...

Where 0x76a1569c is hashCode of object and field.

Also I always add something like this in critical sections:

int hash = System.identityHashCode(this);
long tid = Thread.currentThread().getId();
System.out.println(">>VER>> START WRITE/READ " + tid + " " + hash);
try {
	Thread.sleep(10);
} catch (InterruptedException e) {
throw new RuntimeException(e);
}

System.out.println(">>VER>> END WRITE/READ " + tid + " " + hash);    

And analise stdout. 
This race was verified.
Comment 9 Remy Maucherat 2010-10-01 09:51:41 UTC
Really a pity some attention has been paid to the drones during Tomcat 7 development, I knew it would just attract more of them :)
Comment 10 Mark Thomas 2010-10-02 14:56:56 UTC
Fixed in trunk (will be in 7.0.3 onwards) and proposed for 6.0.x.

There are many places were static analysis incorrectly reports thread safety issues in the Tomcat code base. Bug reports that look like copy and pasted output from these tools tends to get ignored / resolved as invalid. In this case it took a close inspection of the EL parser to spot the place where these objects were cached to confirm this as a potential issue.

If you have done the analysis that demonstrates that there really is an issue it saves a lot of time all round if you include that in the initial bug report.
Comment 11 Kostya Serebryany 2010-10-02 16:10:45 UTC
(In reply to comment #10)
> Fixed in trunk (will be in 7.0.3 onwards) and proposed for 6.0.x.
> 
> There are many places were static analysis 
This is why we use dynamic analysis :) 
We are experimenting with ThreadSanitizer for Java (search for it if curious). 
Not something we can offer to users yet, but already 'sort of works for us'. 

> incorrectly reports thread safety
> issues in the Tomcat code base. Bug reports that look like copy and pasted
> output from these tools tends to get ignored / resolved as invalid. In this
> case it took a close inspection of the EL parser to spot the place where these
> objects were cached to confirm this as a potential issue.
> 
> If you have done the analysis that demonstrates that there really is an issue
> it saves a lot of time all round if you include that in the initial bug report.
Agree.
Comment 12 Konstantin Kolinko 2010-10-03 14:48:42 UTC
(In reply to comment #10)
> (...) In this
> case it took a close inspection of the EL parser to spot the place where these
> objects were cached to confirm this as a potential issue.
> 
For reference: the caching of EL nodes is performed by
o.a.el.lang.ExpressionBuilder#createNodeInternal(String)
(using o.a.el.lang.ExpressionBuilder#cache)
Comment 13 Mark Thomas 2010-10-04 17:16:33 UTC
This has been fixed in 6.0.x and will be included in 6.0.30 onwards.