https://issues.apache.org/bugzilla/show_bug.cgi?id=51249

Reimplement system properties replacement code in ClassLoaderLogManager of JULI
1. Do not use recursion.
2. Do not stop on the first unrecognized property, but continue with the rest of the string.
3. Do not call System.getProperty() on an empty key, because it throws IllegalArgumentException. Threat "${}" as unrecognized property.

git-svn-id: https://svn.apache.org/repos/asf/tomcat/trunk@1133857 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/java/org/apache/juli/ClassLoaderLogManager.java b/java/org/apache/juli/ClassLoaderLogManager.java
index 248334f..3367460 100644 (file)
--- a/java/org/apache/juli/ClassLoaderLogManager.java
+++ b/java/org/apache/juli/ClassLoaderLogManager.java
@@ -566,25 +566,32 @@ public class ClassLoaderLogManager extends LogManager {
      */
     protected String replace(String str) {
         String result = str;
-        int pos_start = result.indexOf("${");
-        if (pos_start != -1) {
-            int pos_end = result.indexOf('}', pos_start);
-            if (pos_end != -1) {
-                String propName = result.substring(pos_start + 2, pos_end);
-                String replacement = System.getProperty(propName);
+        int pos_start = str.indexOf("${");
+        if (pos_start >= 0) {
+            StringBuilder builder = new StringBuilder();
+            int pos_end = -1;
+            while (pos_start >= 0) {
+                builder.append(str, pos_end + 1, pos_start);
+                pos_end = str.indexOf('}', pos_start + 2);
+                if (pos_end < 0) {
+                    pos_end = pos_start - 1;
+                    break;
+                }
+                String propName = str.substring(pos_start + 2, pos_end);
+                String replacement = propName.length() > 0 ? System
+                        .getProperty(propName) : null;
                 if (replacement != null) {
-                    if(pos_start >0) {
-                        result = result.substring(0,pos_start) + 
-                            replacement + replace(result.substring(pos_end + 1));
-                    } else {                       
-                        result = replacement + replace(result.substring(pos_end + 1));
-                    }
+                    builder.append(replacement);
+                } else {
+                    builder.append(str, pos_start, pos_end + 1);
                 }
+                pos_start = str.indexOf("${", pos_end + 1);
             }
+            builder.append(str, pos_end + 1, str.length());
+            result = builder.toString();
         }
         return result;
     }
-    
 
     // ---------------------------------------------------- LogNode Inner Class
 

diff --git a/test/org/apache/juli/TestClassLoaderLogManager.java b/test/org/apache/juli/TestClassLoaderLogManager.java
index bb86953..3ecf3e8 100644 (file)
--- a/test/org/apache/juli/TestClassLoaderLogManager.java
+++ b/test/org/apache/juli/TestClassLoaderLogManager.java
@@ -28,8 +28,8 @@ public class TestClassLoaderLogManager extends TestCase {
         ClassLoaderLogManager logManager = new ClassLoaderLogManager();
         assertEquals("", logManager.replace(""));
         assertEquals("${", logManager.replace("${"));
-        assertEquals("${undefinedsystemproperty}",
-                logManager.replace("${undefinedsystemproperty}"));
+        assertEquals("${undefinedproperty}",
+                logManager.replace("${undefinedproperty}"));
         assertEquals(
                 System.getProperty("line.separator")
                         + System.getProperty("path.separator")
@@ -46,6 +46,13 @@ public class TestClassLoaderLogManager extends TestCase {
         assertEquals(
                 "%{file.separator}" + System.getProperty("file.separator"),
                 logManager.replace("%{file.separator}${file.separator}"));
+        assertEquals(
+                System.getProperty("file.separator") + "${undefinedproperty}"
+                        + System.getProperty("file.separator"),
+                logManager
+                        .replace("${file.separator}${undefinedproperty}${file.separator}"));
+        assertEquals("${}" + System.getProperty("path.separator"),
+                logManager.replace("${}${path.separator}"));
     }
 
 }

diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index ffac5c6..0cf128a 100644 (file)
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -76,6 +76,11 @@
         files in parallel. Apache Tomcat does not do this but products that
         embed it may. (markt)
       </fix>
+      <fix>
+        <bug>51249</bug>: Further improve system property replacement code
+        in ClassLoaderLogManager of Tomcat JULI to cover some corner cases.
+        (kkolinko)
+      </fix>
     </changelog>
   </subsection>
   <subsection name="Coyote">
@@ -140,7 +145,7 @@
       </fix>
       <fix>
         <bug>51249</bug>: Correct ClassLoaderLogManager system property
-        replacement code so properties of the form ${...}${...} can be used
+        replacement code so properties of the form "}${...}" can be used
         without error. (markt) 
       </fix>
       <fix>