groovy
  1. groovy
  2. GROOVY-6456

AbstractHttpServlet.applyResourceNameMatcher race condition

    Details

    • Number of attachments :
      3

      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)

      1. AbstractHttpServlet.java
        17 kB
        Andrej Fink
      2. AbstractHttpServlet.perf.java
        17 kB
        Andrej Fink

        Activity

        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 Laforge 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 Laforge 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
        Andrej 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
        Andrej 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
        blackdrag blackdrag 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 blackdrag 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:
            blackdrag blackdrag
            Reporter:
            Andrej Fink
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: