Groovy
  1. Groovy
  2. GROOVY-6456

AbstractHttpServlet.applyResourceNameMatcher race condition

    Details

      Description

      groovy.servlet.AbstractHttpServlet#applyResourceNameMatcher uses java.util.regex.Matcher object in this way:

      matcher.reset(uri);

      but Matcher is NOT ThreadSafe!

      javadoc:
      Instances of this class are not safe for use by multiple concurrent threads.

      Pattern class IS thread safe.

      stacktrace from bug demo
      2013-12-01 23:36:50.009:WARN:oejs.ServletHandler:/1Test.groovy
      java.lang.IndexOutOfBoundsException: start 0, end -1, s.length() 14
      at java.lang.AbstractStringBuilder.append(AbstractStringBuilder.java:476)
      at java.lang.StringBuffer.append(StringBuffer.java:309)
      at java.util.regex.Matcher.appendReplacement(Matcher.java:839)
      at java.util.regex.Matcher.replaceAll(Matcher.java:906)
      at groovy.servlet.AbstractHttpServlet.applyResourceNameMatcher(AbstractHttpServlet.java:303)
      at groovy.servlet.AbstractHttpServlet.getScriptUri(AbstractHttpServlet.java:290)
      at groovy.servlet.GroovyServlet.service(GroovyServlet.java:105)
      at javax.servlet.http.HttpServlet.service(HttpServlet.java:847)

        Activity

        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Resolved Resolved
        46d 20h 12m 1 Jochen Theodorou 13/Jan/14 09:27
        Resolved Resolved Closed Closed
        81d 14h 33m 1 Paul King 05/Apr/14 01:00
        Mark Thomas made changes -
        Workflow jira [ 12974099 ] Default workflow, editable Closed status [ 12981258 ]
        Mark Thomas made changes -
        Project Import Mon Apr 06 02:11:23 UTC 2015 [ 1428286283443 ]
        Mark Thomas made changes -
        Workflow jira [ 12735367 ] Default workflow, editable Closed status [ 12747110 ]
        Mark Thomas made changes -
        Project Import Sun Apr 05 13:32:57 UTC 2015 [ 1428240777691 ]
        Paul King made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Jochen Theodorou made changes -
        Fix Version/s 2.3.0 [ 19608 ]
        Assignee blackdrag blackdrag [ blackdrag ]
        Resolution Fixed [ 1 ]
        Status Open [ 1 ] Resolved [ 5 ]
        Hide
        Jochen Theodorou added a comment -

        We decide to apply this even though it is a breaking change. But we think the feature is not used much and the fix is easy to do. So we think it is more important to fix the race condition here. thanks for the contribution

        Show
        Jochen Theodorou added a comment - We decide to apply this even though it is a breaking change. But we think the feature is not used much and the fix is easy to do. So we think it is more important to fix the race condition here. thanks for the contribution
        Jochen Theodorou made changes -
        Labels breaking
        Hide
        Pascal Schumacher added a comment -

        I updated the pull request with the new version (AbstractHttpServlet.perf.java) of the patch.

        Show
        Pascal Schumacher added a comment - I updated the pull request with the new version (AbstractHttpServlet.perf.java) of the patch.
        magicprinc made changes -
        Attachment AbstractHttpServlet.perf.java [ 64635 ]
        magicprinc made changes -
        Attachment AbstractHttpServlet.perf.java [ 64633 ]
        magicprinc made changes -
        Attachment AbstractHttpServlet.perf.java [ 64633 ]
        Hide
        Andrew P Fink added a comment -

        Could You inspect new 'performant' version and do another pull request for it?
        I did some obvious optimizations, so it should be faster.

        AbstractHttpServlet.perf.java
        *Matcher bugfix.
        *NPE in war bugfix.
        *more optimizations

        Show
        Andrew P Fink added a comment - Could You inspect new 'performant' version and do another pull request for it? I did some obvious optimizations, so it should be faster. AbstractHttpServlet.perf.java *Matcher bugfix. *NPE in war bugfix. *more optimizations
        Hide
        Guillaume Delcroix added a comment -

        Regarding the behavior of GroovyScriptEngine, yes, that's its normal behavior, to try and find where DS is coming from. Seeing it looks like it's a capitalized name, Groovy thinks it might be a class, so it tries to find where that DS class could be coming from.

        Show
        Guillaume Delcroix added a comment - Regarding the behavior of GroovyScriptEngine, yes, that's its normal behavior, to try and find where DS is coming from. Seeing it looks like it's a capitalized name, Groovy thinks it might be a class, so it tries to find where that DS class could be coming from.
        Hide
        Pascal Schumacher added a comment -

        Hi Andrej,

        np about the pull request. Thanks for the fixed version. I converted it to a pull request to make reviewing easier.

        Sadly I do not know if the GroovyScripEngine behavior is normal. Maybe blackdrag blackdrag or somebody else can comment on this?

        Show
        Pascal Schumacher added a comment - Hi Andrej, np about the pull request. Thanks for the fixed version. I converted it to a pull request to make reviewing easier. Sadly I do not know if the GroovyScripEngine behavior is normal. Maybe blackdrag blackdrag or somebody else can comment on this?
        Pascal Schumacher made changes -
        Summary AbstractHttpServlet.applyResourceNameMatcher possible race condition? AbstractHttpServlet.applyResourceNameMatcher race condition
        Hide
        Andrew P Fink added a comment -

        during test process I found strange GroovyScripEngine behavior. I doubt: is it normal?

        //Groovlet1.groovy
        println DS //DS exists in binding

        getResourceConnection was called several times:
        //getScriptUri: /nsa/Groovlet1.groovy -> /Groovlet1.groovy
        getResourceConnection: /Groovlet1.groovy
        getResourceConnection: java/lang/DS.groovy
        getResourceConnection: java/io/DS.groovy
        getResourceConnection: java/net/DS.groovy
        getResourceConnection: java/util/DS.groovy
        getResourceConnection: groovy/lang/DS.groovy
        getResourceConnection: groovy\lang\DS.groovy
        getResourceConnection: groovy/util/DS.groovy
        getResourceConnection: groovy\util\DS.groovy
        getResourceConnection: DS.groovy
        getResourceConnection: DS.groovy
        getResourceConnection: java/lang/Groovlet1$DS.groovy
        getResourceConnection: java/io/Groovlet1$DS.groovy
        getResourceConnection: java/net/Groovlet1$DS.groovy
        getResourceConnection: java/util/Groovlet1$DS.groovy
        getResourceConnection: groovy/lang/Groovlet1$DS.groovy
        getResourceConnection: groovy/util/Groovlet1$DS.groovy
        getResourceConnection: Groovlet1$DS.groovy
        getResourceConnection: Groovlet1BeanInfo.groovy
        getResourceConnection: Groovlet1Customizer.groovy

        Show
        Andrew P Fink added a comment - during test process I found strange GroovyScripEngine behavior. I doubt: is it normal? //Groovlet1.groovy println DS //DS exists in binding getResourceConnection was called several times: //getScriptUri: /nsa/Groovlet1.groovy -> /Groovlet1.groovy getResourceConnection: /Groovlet1.groovy getResourceConnection: java/lang/DS.groovy getResourceConnection: java/io/DS.groovy getResourceConnection: java/net/DS.groovy getResourceConnection: java/util/DS.groovy getResourceConnection: groovy/lang/DS.groovy getResourceConnection: groovy\lang\DS.groovy getResourceConnection: groovy/util/DS.groovy getResourceConnection: groovy\util\DS.groovy getResourceConnection: DS.groovy getResourceConnection: DS.groovy getResourceConnection: java/lang/Groovlet1$DS.groovy getResourceConnection: java/io/Groovlet1$DS.groovy getResourceConnection: java/net/Groovlet1$DS.groovy getResourceConnection: java/util/Groovlet1$DS.groovy getResourceConnection: groovy/lang/Groovlet1$DS.groovy getResourceConnection: groovy/util/Groovlet1$DS.groovy getResourceConnection: Groovlet1$DS.groovy getResourceConnection: Groovlet1BeanInfo.groovy getResourceConnection: Groovlet1Customizer.groovy
        magicprinc made changes -
        Attachment AbstractHttpServlet.java [ 64630 ]
        Hide
        Andrew P Fink added a comment -

        Matcher bug fixed.
        optimization.
        'NPE in war' bug fixed.

        Show
        Andrew P Fink added a comment - Matcher bug fixed. optimization. 'NPE in war' bug fixed.
        magicprinc made changes -
        Attachment AbstractHttpServlet.java [ 64624 ]
        Hide
        Andrew P Fink added a comment -

        Sorry, no time for pull, but new version attached.

        Show
        Andrew P Fink added a comment - Sorry, no time for pull, but new version attached.
        magicprinc made changes -
        Attachment AbstractHttpServlet.java [ 64624 ]
        Hide
        Andrew P Fink added a comment -

        bug fixed, little optimized version

        Show
        Andrew P Fink added a comment - bug fixed, little optimized version
        Hide
        Pascal Schumacher added a comment -

        Thanks for reporting.

        If you like to you can send us a pull request with a fix. Else I will take a look in the next few days if nobody else beats me to it.

        Show
        Pascal Schumacher added a comment - Thanks for reporting. If you like to you can send us a pull request with a fix. Else I will take a look in the next few days if nobody else beats me to it.
        magicprinc made changes -
        Description groovy.servlet.AbstractHttpServlet#applyResourceNameMatcher uses java.util.regex.Matcher object in this way:

        matcher.reset(uri);

        but Matcher is not Thread safe!

        javadoc:
        Instances of this class are not safe for use by multiple concurrent threads.

        Pattern class is thread safe.


        stacktrace from bug demo
        2013-12-01 23:36:50.009:WARN:oejs.ServletHandler:/1Test.groovy
        java.lang.IndexOutOfBoundsException: start 0, end -1, s.length() 14
        at java.lang.AbstractStringBuilder.append(AbstractStringBuilder.java:476)
        at java.lang.StringBuffer.append(StringBuffer.java:309)
        at java.util.regex.Matcher.appendReplacement(Matcher.java:839)
        at java.util.regex.Matcher.replaceAll(Matcher.java:906)
        at groovy.servlet.AbstractHttpServlet.applyResourceNameMatcher(AbstractHttpServlet.java:303)
        at groovy.servlet.AbstractHttpServlet.getScriptUri(AbstractHttpServlet.java:290)
        at groovy.servlet.GroovyServlet.service(GroovyServlet.java:105)
        at javax.servlet.http.HttpServlet.service(HttpServlet.java:847)

        groovy.servlet.AbstractHttpServlet#applyResourceNameMatcher uses java.util.regex.Matcher object in this way:

        matcher.reset(uri);

        but Matcher is NOT ThreadSafe!

        javadoc:
        Instances of this class are not safe for use by multiple concurrent threads.

        Pattern class IS thread safe.


        stacktrace from bug demo
        2013-12-01 23:36:50.009:WARN:oejs.ServletHandler:/1Test.groovy
        java.lang.IndexOutOfBoundsException: start 0, end -1, s.length() 14
        at java.lang.AbstractStringBuilder.append(AbstractStringBuilder.java:476)
        at java.lang.StringBuffer.append(StringBuffer.java:309)
        at java.util.regex.Matcher.appendReplacement(Matcher.java:839)
        at java.util.regex.Matcher.replaceAll(Matcher.java:906)
        at groovy.servlet.AbstractHttpServlet.applyResourceNameMatcher(AbstractHttpServlet.java:303)
        at groovy.servlet.AbstractHttpServlet.getScriptUri(AbstractHttpServlet.java:290)
        at groovy.servlet.GroovyServlet.service(GroovyServlet.java:105)
        at javax.servlet.http.HttpServlet.service(HttpServlet.java:847)

        magicprinc made changes -
        Description groovy.servlet.AbstractHttpServlet#applyResourceNameMatcher uses java.util.regex.Matcher object in this way:

        matcher.reset(uri);

        but Matcher is not Thread safe!

        javadoc:
        Instances of this class are not safe for use by multiple concurrent threads.

        Pattern class is thread safe.
        groovy.servlet.AbstractHttpServlet#applyResourceNameMatcher uses java.util.regex.Matcher object in this way:

        matcher.reset(uri);

        but Matcher is not Thread safe!

        javadoc:
        Instances of this class are not safe for use by multiple concurrent threads.

        Pattern class is thread safe.


        stacktrace from bug demo
        2013-12-01 23:36:50.009:WARN:oejs.ServletHandler:/1Test.groovy
        java.lang.IndexOutOfBoundsException: start 0, end -1, s.length() 14
        at java.lang.AbstractStringBuilder.append(AbstractStringBuilder.java:476)
        at java.lang.StringBuffer.append(StringBuffer.java:309)
        at java.util.regex.Matcher.appendReplacement(Matcher.java:839)
        at java.util.regex.Matcher.replaceAll(Matcher.java:906)
        at groovy.servlet.AbstractHttpServlet.applyResourceNameMatcher(AbstractHttpServlet.java:303)
        at groovy.servlet.AbstractHttpServlet.getScriptUri(AbstractHttpServlet.java:290)
        at groovy.servlet.GroovyServlet.service(GroovyServlet.java:105)
        at javax.servlet.http.HttpServlet.service(HttpServlet.java:847)

        magicprinc made changes -
        Attachment groovyServlet_MatcherBug.zip [ 64622 ]
        Hide
        Andrew P Fink added a comment -

        bug demo.
        $ groovy runme

        Show
        Andrew P Fink added a comment - bug demo. $ groovy runme
        magicprinc made changes -
        Priority Minor [ 4 ] Critical [ 2 ]
        magicprinc made changes -
        Field Original Value New Value
        Description groovy.servlet.AbstractHttpServlet#applyResourceNameMatcher uses java.util.regex.Matcher object in this way:

        matcher.reset(uri);

        but Matcher is not Thread safe!

        javadoc:
        Instances of this class are not safe for use by multiple concurrent threads.


        groovy.servlet.AbstractHttpServlet#applyResourceNameMatcher uses java.util.regex.Matcher object in this way:

        matcher.reset(uri);

        but Matcher is not Thread safe!

        javadoc:
        Instances of this class are not safe for use by multiple concurrent threads.

        Pattern class is thread safe.
        Andrew P Fink created issue -

          People

          • Assignee:
            Jochen Theodorou
            Reporter:
            Andrew P Fink
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development