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

        Hide
        Andrew P Fink added a comment -

        bug demo.
        $ groovy runme

        Show
        Andrew P Fink added a comment - bug demo. $ groovy runme
        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.
        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
        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.
        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.
        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
        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?
        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
        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
        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.
        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

          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