Uploaded image for project: '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
        blackdrag 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
        blackdrag 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
        Hide
        pschumacher Pascal Schumacher added a comment -

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

        Show
        pschumacher Pascal Schumacher added a comment - I updated the pull request with the new version (AbstractHttpServlet.perf.java) of the patch.
        Hide
        rybin.andrey@gmail.com 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
        rybin.andrey@gmail.com 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 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 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
        pschumacher 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
        pschumacher 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
        rybin.andrey@gmail.com 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
        rybin.andrey@gmail.com 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
        rybin.andrey@gmail.com Andrew P Fink added a comment -

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

        Show
        rybin.andrey@gmail.com Andrew P Fink added a comment - Matcher bug fixed. optimization. 'NPE in war' bug fixed.
        Hide
        rybin.andrey@gmail.com Andrew P Fink added a comment -

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

        Show
        rybin.andrey@gmail.com Andrew P Fink added a comment - Sorry, no time for pull, but new version attached.
        Hide
        rybin.andrey@gmail.com Andrew P Fink added a comment -

        bug fixed, little optimized version

        Show
        rybin.andrey@gmail.com Andrew P Fink added a comment - bug fixed, little optimized version
        Hide
        pschumacher 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
        pschumacher 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
        rybin.andrey@gmail.com Andrew P Fink added a comment -

        bug demo.
        $ groovy runme

        Show
        rybin.andrey@gmail.com Andrew P Fink added a comment - bug demo. $ groovy runme

          People

          • Assignee:
            blackdrag Jochen Theodorou
            Reporter:
            rybin.andrey@gmail.com Andrew P Fink
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development