Bug 57434 - Race condition in EL1.0 validation
Summary: Race condition in EL1.0 validation
Alias: None
Product: Taglibs
Classification: Unclassified
Component: Standard Taglib (show other bugs)
Version: 1.2.1
Hardware: All All
: P2 normal (vote)
Target Milestone: ---
Assignee: Tomcat Developers Mailing List
Depends on:
Reported: 2015-01-11 21:52 UTC by Jeremy Boynes
Modified: 2018-04-09 14:49 UTC (History)
1 user (show)

Patch (813 bytes, patch)
2017-12-11 11:21 UTC, Tomas Hofman
Details | Diff
Adds new public method that allows the cache to be bypassed when parsing expressions. (6.79 KB, patch)
2018-04-05 16:03 UTC, Jeremy Boynes
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Boynes 2015-01-11 21:52:15 UTC

Evaluator uses a single static instance of an ELEvaluator. As a result enabling and disabling the cache would apply to all validate and evaluate operations from all threads. This will result in unexpected behaviour if validation is performed on multiple threads concurrently.

This is not an issue when the library is included with a web application as multiple copies of the Evaluator will be created for the different ClassLoaders (although that may have other unanticipated consequences).

This may be an issue if the library is shared between applications.
Comment 1 Tomas Hofman 2017-12-11 11:20:03 UTC
This concurrency issue was also hit in EAP/Wildfly. It is an issue even if the library is included inside a web application, e.g. when two different JSPs are requested and evaluated concurrently.

Tested with taglibs-standard-jstlel 1.2.5.

I'm attaching a suggested patch.
Comment 2 Tomas Hofman 2017-12-11 11:21:42 UTC
Created attachment 35602 [details]
Comment 3 Jeremy Boynes 2017-12-11 13:35:22 UTC
I believe this patch reduces the window for the race but does not fully close it as another thread could still modify the field between when it is set and then read. It would be unlikely due to variable caching but still possible.
Comment 4 Jeremy Boynes 2017-12-11 13:43:05 UTC
The current implementation disables caching while expressions are being validated during the translation process. This has the side effect of disabling the cache for evaluations that occur concurrently i.e. if a JSP is being translated at the same time a request is occurring. I don't think this was the intent but hesitate to change the behaviour given the age of this library.

If we think this is safe, we could simply pass the flag as a parameter to parseExpressionString and eliminate the field entirely.
Comment 5 Tomas Hofman 2018-01-04 11:45:13 UTC
Sorry for not elaborating before - in fact my main motivation was that currently the parseExpressionString() method can throw a NPE because of the race, which breaks the request processing. Moreover when the NPE occurs, the mBypassCache flag is not reset to false by the calling method Evaluator#validate() and that affects later calls.

The patch seemed like a safe solution to prevent the NPE. The caching could still be affected by the race condition, but that seemed like a smaller problem.

Passing the flag as a method parameter would be ideal. Perhaps an overloaded method with an extra parameter could be introduced while the old one is kept, to stay backward compatible in case someone is calling the methods directly. I opened a PR in that regard in our fork:


(I overloaded also convertStaticValueToExpectedType(), which is too using the flag.)

I understand the reluctance though.
Comment 6 Tomas Hofman 2018-01-04 11:56:14 UTC
Linking the JBoss issue for reference: https://issues.jboss.org/browse/JBEE-183
Comment 7 Tomas Hofman 2018-04-03 13:50:31 UTC
Jeremy, what do you think about the PR?


If you don't want to change current behaviour, what about at least preventing the NPE from happening (the patch in my first attachment would do that)?
Comment 8 Jeremy Boynes 2018-04-05 16:03:56 UTC
Created attachment 35845 [details]
Adds new public method that allows the cache to be bypassed when parsing expressions.

Based on Tomas's PR, but rather then change the public method signature this adds a new method that allows the cache to be bypassed. It also tightens up the package-private access and synchronization around access the expression cache.
Comment 9 Jeremy Boynes 2018-04-05 17:56:14 UTC
Patch applied in revision 1828458

@Tomas, do you have a reliable repro that can be used to test it?
Comment 10 Tomas Hofman 2018-04-06 11:25:30 UTC
Yes, I think I do, but I'm afraid it's gonna involve Wildfly. I will check it out and get back here.
Comment 11 Tomas Hofman 2018-04-09 08:38:22 UTC
I can no longer reproduce the issue with patched version of jstlel. Thanks Jeremy!
Comment 12 Jeremy Boynes 2018-04-09 14:49:53 UTC
Fix will be included in 1.2.6