org.owasp.esapi.ESAPI singletons in 1.4 are not thread-safe

[GoogleCodeExporter]

There is no synchronization on the 1.4 branch in the ESAPI class with respect 
to getting and setting the various singletons within it.

This has lead to some live-lock 100% CPU usage conditions for us when the 
HTMLEntityCodec.initializeMaps() call is run by multiple threads in parallel as 
a result of the DefaultEncoder constructor being run in parallel.

The lack of synchronization could lead to some threads seeing partially 
constructed objects and other nastiness.

I have attached a patch against r1465 on the 1.4 branch which adds a lock to 
each singleton to address this issue.

The additional locks are used rather than synchronizing the methods so that the 
locking protocol cannot be interfered with by external code and to allow 
separate portions of the ESAPI library to initialize in parallel.

Original issue reported on code.google.com by patrick....@gmail.com on 26 Aug 2010 at 12:36

Attachments:

• 0002-Made-org.owasp.esapi.ESAPI-singletons-thread-safe.patch

[GoogleCodeExporter]

Thank you for this Patrick. We will apply this to 1.4 before the next release.

Original comment by manico.james@gmail.com on 2 Nov 2010 at 8:09

• Added labels: Milestone-Release1.4

[GoogleCodeExporter]

Is this fix in the 2.0 GA release (or later)?

Original comment by skwee...@gmail.com on 3 Aug 2011 at 11:49

[GoogleCodeExporter]

The singletons are implemented using a double-checking mechanism and a volatile 
reference variable. This approach is thread-safe for JDK 1.5 and higher, but it 
is not for previous versions (see 
http://java.dzone.com/articles/design-patterns-revisited-1-si). Therefore I 
would recommend to use a static instance variable (has the additional benefit 
of less lines of code) or if performance is a concern to use the 
"initialization-on-demand-idiom" 
(http://en.wikipedia.org/wiki/Initialization_on_demand_holder_idiom) 

Original comment by wettstei...@gmail.com on 18 Aug 2011 at 12:20

[GoogleCodeExporter]

Were you referring to the patch or to what's in the 1.4 source control 
repository? I haven't checked the source repo but the patch does not use 
volatiles. It synchronizes on a lock before doing any checking, which is what 
1.4 requires to do lazy initialization without introducing a holder class.

Original comment by patrick....@gmail.com on 18 Aug 2011 at 1:35

[GoogleCodeExporter]

I was talking about the actual code in the trunk (see DefaultEncoder for 
example)

Original comment by wettstei...@gmail.com on 31 Aug 2011 at 3:41

[GoogleCodeExporter]

We are experiencing this issue. What do you suggest since it does not look like 
this will be fixed any time soon?

Original comment by nemi5...@gmail.com on 7 Aug 2012 at 6:48

[GoogleCodeExporter]

nemi5150: you can apply my patch (attached to this issue).

wettstein.frank: I don't understand your comment. The patch is against the 1.4 
branch, not trunk, and already uses static instance variables. Perhaps the 
initialization-on-demand-idiom would be best here, but it's more complex. I 
assume the patch was never applied because of your concerns, but I don't 
understand how they are relevant to the 1.4 branch.

Original comment by patrick....@gmail.com on 7 Aug 2012 at 7:38

[GoogleCodeExporter]

Patrick, I have looked at your patch, but it does not seem to be addressing the 
root cause of the problem. Isn't the root cause that a constructor is 
initializing a static variable? This is straight from do-not-do 101. I do not 
understand why this static variable is not initialized in a static block? That 
way it will only ever be initialized once and only when the class is loaded the 
first time by the classloader. 

Original comment by nemi5...@gmail.com on 7 Aug 2012 at 8:36

[GoogleCodeExporter]

Wait, I see. Maybe you should reopen issue 143? It may be fixed in a later 
build, but 1.4.4 is the latest stable, correct? If so, then 1.4.5 needs to be 
stabilized. 1.4.5 in Early Release Only since Aug-2010? Really?

Original comment by nemi5...@gmail.com on 7 Aug 2012 at 8:40

[GoogleCodeExporter]

I was using ESAPI for output encoding only, and switched to the OWASP Reform 
library. If you only need encoding, I would check it out: 
https://www.owasp.org/index.php/Category:OWASP_Encoding_Project

Original comment by patrick....@gmail.com on 8 Aug 2012 at 7:29

[GoogleCodeExporter]

This issue is already fixed in ESAPI 2.x and ESAPI 1.4.x is no longer 
supported. Even if this issue were to be fixed in ESAPI 1.4.x, it would still 
leave many other bugs--some of which are security issues--as unfixed. 
Therefore, this bug nor any others that are specific to ESAPI 1.4.x, will be 
fixed.

Original comment by kevin.w.wall@gmail.com on 2 Sep 2014 at 12:35

• Changed state: WontFix