You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by kk...@apache.org on 2014/11/09 15:04:37 UTC

svn commit: r1637679 - in /tomcat/tc6.0.x/trunk: STATUS.txt java/org/apache/tomcat/util/http/mapper/Mapper.java webapps/docs/changelog.xml

Author: kkolinko
Date: Sun Nov  9 14:04:37 2014
New Revision: 1637679

URL: http://svn.apache.org/r1637679
Log:
Assert that MappingData object is empty before performing mapping work.
It is backport of r1604663

Motivation: Remove dead branches. Protect Mapper from operating on
non-recycled MappingData. Using non-recycled MappingData might result in
mapping request onto a different target, like an issue that prevented us
from releasing 8.0.4 and fixed by r1580080/r1580083. I do not know such
bugs in Tomcat 6, but I want the code to be more safe.
Just a single (mappingData.host != null) check is enough to discern
recycled vs. non-recycled data. Checks for other MappingData fields are
removed by this patch, simplifying the code.

Modified:
    tomcat/tc6.0.x/trunk/STATUS.txt
    tomcat/tc6.0.x/trunk/java/org/apache/tomcat/util/http/mapper/Mapper.java
    tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml

Modified: tomcat/tc6.0.x/trunk/STATUS.txt
URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/STATUS.txt?rev=1637679&r1=1637678&r2=1637679&view=diff
==============================================================================
--- tomcat/tc6.0.x/trunk/STATUS.txt (original)
+++ tomcat/tc6.0.x/trunk/STATUS.txt Sun Nov  9 14:04:37 2014
@@ -28,26 +28,6 @@ None
 PATCHES PROPOSED TO BACKPORT:
   [ New proposals should be added at the end of the list ]
 
-* Assert that MappingData object is empty before performing mapping work.
-  It is backport of r1604663
-
-  Motivation: Remove dead branches. Protect Mapper from operating on
-  non-recycled MappingData. Using non-recycled MappingData might result in
-  mapping request onto a different target, like an issue that prevented us
-  from releasing 8.0.4 and fixed by r1580080/r1580083. I do not know such
-  bugs in Tomcat 6, but I want the code to be more safe.
-  Just a single (mappingData.host != null) check is enough to discern
-  recycled vs. non-recycled data. Checks for other MappingData fields are
-  removed by this patch, simplifying the code.
-
-  A patch generated with "svn diff -x --ignore-space-change" for easier
-  overview of the change is
-    https://people.apache.org/~kkolinko/patches/2014-06-23_tc6_Mapper_diff-x-b.patch
-
-  https://people.apache.org/~kkolinko/patches/2014-06-23_tc6_Mapper.patch
-  +1: kkolinko, markt, remm
-  -1:
-
 * Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=56780
   Enable Tomcat to start when using a IBM JRE in strict SP800-131a mode
   This back-ports the fix as well as some additional changes to more closely

Modified: tomcat/tc6.0.x/trunk/java/org/apache/tomcat/util/http/mapper/Mapper.java
URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/java/org/apache/tomcat/util/http/mapper/Mapper.java?rev=1637679&r1=1637678&r2=1637679&view=diff
==============================================================================
--- tomcat/tc6.0.x/trunk/java/org/apache/tomcat/util/http/mapper/Mapper.java (original)
+++ tomcat/tc6.0.x/trunk/java/org/apache/tomcat/util/http/mapper/Mapper.java Sun Nov  9 14:04:37 2014
@@ -588,82 +588,86 @@ public final class Mapper {
                                    MappingData mappingData)
         throws Exception {
 
+        if (mappingData.host != null) {
+            // The legacy code (dating down at least to Tomcat 4.1) just
+            // skipped all mapping work in this case. That behaviour has a risk
+            // of returning an inconsistent result.
+            // I do not see a valid use case for it.
+            throw new AssertionError();
+        }
+
         uri.setLimit(-1);
 
-        Context[] contexts = null;
+        Context[] contexts;
         Context context = null;
         int nesting = 0;
 
         // Virtual host mapping
-        if (mappingData.host == null) {
-            Host[] hosts = this.hosts;
-            int pos = findIgnoreCase(hosts, host);
-            if ((pos != -1) && (host.equalsIgnoreCase(hosts[pos].name))) {
+        Host[] hosts = this.hosts;
+        int pos = findIgnoreCase(hosts, host);
+        if ((pos != -1) && (host.equalsIgnoreCase(hosts[pos].name))) {
+            mappingData.host = hosts[pos].object;
+            contexts = hosts[pos].contextList.contexts;
+            nesting = hosts[pos].contextList.nesting;
+        } else {
+            if (defaultHostName == null) {
+                return;
+            }
+            pos = find(hosts, defaultHostName);
+            if ((pos != -1) && (defaultHostName.equals(hosts[pos].name))) {
                 mappingData.host = hosts[pos].object;
                 contexts = hosts[pos].contextList.contexts;
                 nesting = hosts[pos].contextList.nesting;
             } else {
-                if (defaultHostName == null) {
-                    return;
-                }
-                pos = find(hosts, defaultHostName);
-                if ((pos != -1) && (defaultHostName.equals(hosts[pos].name))) {
-                    mappingData.host = hosts[pos].object;
-                    contexts = hosts[pos].contextList.contexts;
-                    nesting = hosts[pos].contextList.nesting;
-                } else {
-                    return;
-                }
+                return;
             }
         }
 
         // Context mapping
-        if (mappingData.context == null) {
-            int pos = find(contexts, uri);
-            if (pos == -1) {
-                return;
-            }
+        pos = find(contexts, uri);
+        if (pos == -1) {
+            return;
+        }
 
-            int lastSlash = -1;
-            int uriEnd = uri.getEnd();
-            int length = -1;
-            boolean found = false;
-            while (pos >= 0) {
-                if (uri.startsWith(contexts[pos].name)) {
-                    length = contexts[pos].name.length();
-                    if (uri.getLength() == length) {
-                        found = true;
-                        break;
-                    } else if (uri.startsWithIgnoreCase("/", length)) {
-                        found = true;
-                        break;
-                    }
-                }
-                if (lastSlash == -1) {
-                    lastSlash = nthSlash(uri, nesting + 1);
-                } else {
-                    lastSlash = lastSlash(uri);
+        int lastSlash = -1;
+        int uriEnd = uri.getEnd();
+        int length = -1;
+        boolean found = false;
+        while (pos >= 0) {
+            if (uri.startsWith(contexts[pos].name)) {
+                length = contexts[pos].name.length();
+                if (uri.getLength() == length) {
+                    found = true;
+                    break;
+                } else if (uri.startsWithIgnoreCase("/", length)) {
+                    found = true;
+                    break;
                 }
-                uri.setEnd(lastSlash);
-                pos = find(contexts, uri);
             }
-            uri.setEnd(uriEnd);
-
-            if (!found) {
-                if (contexts[0].name.equals("")) {
-                    context = contexts[0];
-                }
+            if (lastSlash == -1) {
+                lastSlash = nthSlash(uri, nesting + 1);
             } else {
-                context = contexts[pos];
+                lastSlash = lastSlash(uri);
             }
-            if (context != null) {
-                mappingData.context = context.object;
-                mappingData.contextPath.setString(context.name);
+            uri.setEnd(lastSlash);
+            pos = find(contexts, uri);
+        }
+        uri.setEnd(uriEnd);
+
+        if (!found) {
+            if (contexts[0].name.equals("")) {
+                context = contexts[0];
             }
+        } else {
+            context = contexts[pos];
+        }
+        if (context != null) {
+            mappingData.context = context.object;
+            mappingData.contextPath.setString(context.name);
         }
 
         // Wrapper mapping
-        if ((context != null) && (mappingData.wrapper == null)) {
+        if (context != null) {
             internalMapWrapper(context, uri, mappingData);
         }
 

Modified: tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml
URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml?rev=1637679&r1=1637678&r2=1637679&view=diff
==============================================================================
--- tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml (original)
+++ tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml Sun Nov  9 14:04:37 2014
@@ -44,6 +44,14 @@
  General, Catalina, Coyote, Jasper, Cluster, Web applications, Other
 -->
 <section name="Tomcat 6.0.43 (markt)">
+  <subsection name="Catalina">
+    <changelog>
+      <fix>
+        Assert that mapping result object is empty before performing mapping
+        work in <code>Mapper</code>. (kkolinko)
+      </fix>
+    </changelog>
+  </subsection>
   <subsection name="Coyote">
     <changelog>
       <fix>



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org