You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@logging.apache.org by rg...@apache.org on 2022/01/03 22:30:38 UTC
[logging-log4j2] 01/02: LOG4J2-3306 - OptionConverter could cause a StackOverflowError
This is an automated email from the ASF dual-hosted git repository.
rgoers pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git
commit dea97d90dd889eb0b138738b0469b4c1626128b0
Author: Ralph Goers <rg...@apache.org>
AuthorDate: Mon Jan 3 15:30:03 2022 -0700
LOG4J2-3306 - OptionConverter could cause a StackOverflowError
---
.../org/apache/log4j/helpers/OptionConverter.java | 113 +++++++++++---------
.../log4j/core/util/OptionConverterTest.java | 95 +++++++++++++++++
.../logging/log4j/core/util/OptionConverter.java | 118 ++++++++++++---------
src/changes/changes.xml | 3 +
4 files changed, 227 insertions(+), 102 deletions(-)
diff --git a/log4j-1.2-api/src/main/java/org/apache/log4j/helpers/OptionConverter.java b/log4j-1.2-api/src/main/java/org/apache/log4j/helpers/OptionConverter.java
index e94fe57..bcb4d76 100644
--- a/log4j-1.2-api/src/main/java/org/apache/log4j/helpers/OptionConverter.java
+++ b/log4j-1.2-api/src/main/java/org/apache/log4j/helpers/OptionConverter.java
@@ -23,13 +23,17 @@ import org.apache.log4j.spi.Configurator;
import org.apache.log4j.spi.LoggerRepository;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
-import org.apache.logging.log4j.spi.StandardLevel;
+import org.apache.logging.log4j.core.lookup.StrSubstitutor;
import org.apache.logging.log4j.util.LoaderUtil;
+import org.apache.logging.log4j.util.PropertiesUtil;
+import org.apache.logging.log4j.util.Strings;
import java.io.InputStream;
import java.io.InterruptedIOException;
import java.lang.reflect.InvocationTargetException;
import java.net.URL;
+import java.util.ArrayList;
+import java.util.List;
import java.util.Properties;
/**
@@ -43,14 +47,14 @@ public class OptionConverter {
static int DELIM_STOP_LEN = 1;
private static final Logger LOGGER = LogManager.getLogger(OptionConverter.class);
private static final CharMap[] charMap = new CharMap[] {
- new CharMap('n', '\n'),
- new CharMap('r', '\r'),
- new CharMap('t', '\t'),
- new CharMap('f', '\f'),
- new CharMap('\b', '\b'),
- new CharMap('\"', '\"'),
- new CharMap('\'', '\''),
- new CharMap('\\', '\\')
+ new CharMap('n', '\n'),
+ new CharMap('r', '\r'),
+ new CharMap('t', '\t'),
+ new CharMap('f', '\f'),
+ new CharMap('\b', '\b'),
+ new CharMap('\"', '\"'),
+ new CharMap('\'', '\''),
+ new CharMap('\\', '\\')
};
/**
@@ -170,10 +174,9 @@ public class OptionConverter {
if (hashIndex == -1) {
if ("NULL".equalsIgnoreCase(value)) {
return null;
- } else {
- // no class name specified : use standard Level class
- return Level.toLevel(value, defaultValue);
}
+ // no class name specified : use standard Level class
+ return Level.toLevel(value, defaultValue);
}
Level result = defaultValue;
@@ -190,13 +193,11 @@ public class OptionConverter {
+ ":pri=[" + levelName + "]");
try {
- Class customLevel = LoaderUtil.loadClass(clazz);
+ Class<?> customLevel = LoaderUtil.loadClass(clazz);
// get a ref to the specified class' static method
// toLevel(String, org.apache.log4j.Level)
- Class[] paramTypes = new Class[]{String.class,
- org.apache.log4j.Level.class
- };
+ Class<?>[] paramTypes = new Class[] { String.class, org.apache.log4j.Level.class };
java.lang.reflect.Method toLevelMethod =
customLevel.getMethod("toLevel", paramTypes);
@@ -299,11 +300,17 @@ public class OptionConverter {
* @throws IllegalArgumentException if <code>val</code> is malformed.
*/
public static String substVars(String val, Properties props) throws IllegalArgumentException {
+ return substVars(val, props, new ArrayList<>());
+ }
+
+ private static String substVars(final String val, final Properties props, List<String> keys)
+ throws IllegalArgumentException {
- StringBuilder sbuf = new StringBuilder();
+ final StringBuilder sbuf = new StringBuilder();
int i = 0;
- int j, k;
+ int j;
+ int k;
while (true) {
j = val.indexOf(DELIM_START, i);
@@ -311,39 +318,45 @@ public class OptionConverter {
// no more variables
if (i == 0) { // this is a simple string
return val;
- } else { // add the tail string which contails no variables and return the result.
- sbuf.append(val.substring(i, val.length()));
- return sbuf.toString();
}
- } else {
- sbuf.append(val.substring(i, j));
- k = val.indexOf(DELIM_STOP, j);
- if (k == -1) {
- throw new IllegalArgumentException('"' + val +
- "\" has no closing brace. Opening brace at position " + j
- + '.');
- } else {
- j += DELIM_START_LEN;
- String key = val.substring(j, k);
- // first try in System properties
- String replacement = getSystemProperty(key, null);
- // then try props parameter
- if (replacement == null && props != null) {
- replacement = props.getProperty(key);
- }
+ // add the tail string which contails no variables and return the result.
+ sbuf.append(val.substring(i, val.length()));
+ return sbuf.toString();
+ }
+ sbuf.append(val.substring(i, j));
+ k = val.indexOf(DELIM_STOP, j);
+ if (k == -1) {
+ throw new IllegalArgumentException(Strings.dquote(val)
+ + " has no closing brace. Opening brace at position " + j
+ + '.');
+ }
+ j += DELIM_START_LEN;
+ final String key = val.substring(j, k);
+ // first try in System properties
+ String replacement = PropertiesUtil.getProperties().getStringProperty(key, null);
+ // then try props parameter
+ if (replacement == null && props != null) {
+ replacement = props.getProperty(key);
+ }
- if (replacement != null) {
- // Do variable substitution on the replacement string
- // such that we can solve "Hello ${x2}" as "Hello p1"
- // the where the properties are
- // x1=p1
- // x2=${x1}
- String recursiveReplacement = substVars(replacement, props);
- sbuf.append(recursiveReplacement);
- }
- i = k + DELIM_STOP_LEN;
+ if (replacement != null) {
+
+ // Do variable substitution on the replacement string
+ // such that we can solve "Hello ${x2}" as "Hello p1"
+ // the where the properties are
+ // x1=p1
+ // x2=${x1}
+ if (!keys.contains(key)) {
+ List<String> usedKeys = new ArrayList<>(keys);
+ usedKeys.add(key);
+ final String recursiveReplacement = substVars(replacement, props, usedKeys);
+ sbuf.append(recursiveReplacement);
+ } else {
+ sbuf.append(replacement);
}
+
}
+ i = k + DELIM_STOP_LEN;
}
}
@@ -405,7 +418,7 @@ public class OptionConverter {
* <p>
* All configurations steps are taken on the <code>hierarchy</code> passed as a parameter.
* </p>
- *
+ *
* @param inputStream The configuration input stream.
* @param clazz The class name, of the log4j configurator which will parse the <code>inputStream</code>. This must be a
* subclass of {@link Configurator}, or null. If this value is null then a default configurator of
@@ -438,14 +451,14 @@ public class OptionConverter {
* <p>
* All configurations steps are taken on the <code>hierarchy</code> passed as a parameter.
* </p>
- *
+ *
* @param url The location of the configuration file or resource.
* @param clazz The classname, of the log4j configurator which will parse the file or resource at <code>url</code>. This
* must be a subclass of {@link Configurator}, or null. If this value is null then a default configurator of
* {@link PropertyConfigurator} is used, unless the filename pointed to by <code>url</code> ends in '.xml', in
* which case {@link org.apache.log4j.xml.DOMConfigurator} is used.
* @param hierarchy The {@link LoggerRepository} to act on.
- *
+ *
* @since 1.1.4
*/
static public void selectAndConfigure(URL url, String clazz, LoggerRepository hierarchy) {
diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/util/OptionConverterTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/util/OptionConverterTest.java
new file mode 100644
index 0000000..65de5d7
--- /dev/null
+++ b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/util/OptionConverterTest.java
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+package org.apache.logging.log4j.core.util;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import java.util.Properties;
+
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests {@link OptionConverter}.
+ */
+public class OptionConverterTest {
+
+ @Test
+ public void testSubstVars() {
+ Properties props = new Properties();
+ props.setProperty("key", "${key}");
+ props.setProperty("testKey", "Log4j");
+ assertEquals("Value of key is ${key}.", OptionConverter.substVars("Value of key is ${key}.", props));
+ assertEquals("Value of key is .", OptionConverter.substVars("Value of key is ${key2}.", props));
+ assertEquals("Value of testKey:testKey is Log4j:Log4j",
+ OptionConverter.substVars("Value of testKey:testKey is ${testKey}:${testKey}", props));
+ }
+
+ /**
+ * StrSubstitutor would resolve ${key} to Key, append the result to "test" and then resolve ${testKey}.
+ * Verify that substVars doesn't construct dynamic keys.
+ */
+ @Test
+ public void testAppend() {
+ Properties props = new Properties();
+ props.setProperty("key", "Key");
+ props.setProperty("testKey", "Hello");
+ assertEquals("Value of testKey is }",
+ OptionConverter.substVars("Value of testKey is ${test${key}}", props));
+ }
+
+ /**
+ * StrSubstitutor would resolve ${key}, append the result to "test" and then resolve ${testKey}.
+ * Verify that substVars will treat the second expression up to the first '}' as part of the key.
+ */
+ @Test
+ public void testAppend2() {
+ Properties props = new Properties();
+ props.setProperty("test${key", "Hello");
+ assertEquals("Value of testKey is Hello}",
+ OptionConverter.substVars("Value of testKey is ${test${key}}", props));
+ }
+
+ @Test
+ public void testRecursion() {
+ Properties props = new RecursiveProperties();
+ props.setProperty("name", "Neo");
+ props.setProperty("greeting", "Hello ${name}");
+
+ String s = props.getProperty("greeting");
+ System.out.println("greeting = '"+s+"'");
+ }
+
+ private static class RecursiveProperties extends Properties {
+ @Override
+ public String getProperty(String key)
+ {
+ System.out.println("getProperty for "+key);
+ try
+ {
+ String val = super.getProperty(key);
+ // The following call works for log4j 2.17.0 and causes StackOverflowError for 2.17.1
+ // This is because substVars change implementation in 2.17.1 to call StrSubstitutor.replace(val, props)
+ // which calls props.getProperty() for EVERY property making it recursive
+ return OptionConverter.substVars(val, this);
+ }
+ catch (Exception e)
+ {
+ return super.getProperty(key);
+ }
+ }
+ }
+}
diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/util/OptionConverter.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/util/OptionConverter.java
index d6a5be0..ad1d1f6 100644
--- a/log4j-core/src/main/java/org/apache/logging/log4j/core/util/OptionConverter.java
+++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/util/OptionConverter.java
@@ -17,6 +17,8 @@
package org.apache.logging.log4j.core.util;
import java.io.InterruptedIOException;
+import java.util.ArrayList;
+import java.util.List;
import java.util.Locale;
import java.util.Properties;
@@ -66,32 +68,32 @@ public final class OptionConverter {
if (c == '\\') {
c = s.charAt(i++);
switch (c) {
- case 'n':
- c = '\n';
- break;
- case 'r':
- c = '\r';
- break;
- case 't':
- c = '\t';
- break;
- case 'f':
- c = '\f';
- break;
- case 'b':
- c = '\b';
- break;
- case '"':
- c = '\"';
- break;
- case '\'':
- c = '\'';
- break;
- case '\\':
- c = '\\';
- break;
- default:
- // there is no default case.
+ case 'n':
+ c = '\n';
+ break;
+ case 'r':
+ c = '\r';
+ break;
+ case 't':
+ c = '\t';
+ break;
+ case 'f':
+ c = '\f';
+ break;
+ case 'b':
+ c = '\b';
+ break;
+ case '"':
+ c = '\"';
+ break;
+ case '\'':
+ c = '\'';
+ break;
+ case '\\':
+ c = '\\';
+ break;
+ default:
+ // there is no default case.
}
}
sbuf.append(c);
@@ -100,7 +102,7 @@ public final class OptionConverter {
}
public static Object instantiateByKey(final Properties props, final String key, final Class<?> superClass,
- final Object defaultValue) {
+ final Object defaultValue) {
// Get the value of the property in string form
final String className = findAndSubst(key, props);
@@ -110,7 +112,7 @@ public final class OptionConverter {
}
// Trim className to avoid trailing spaces that cause problems.
return OptionConverter.instantiateByClassName(className.trim(), superClass,
- defaultValue);
+ defaultValue);
}
/**
@@ -156,14 +158,14 @@ public final class OptionConverter {
return defaultValue;
}
- public static Level toLevel(String value, final Level defaultValue) {
+ public static Level toLevel(String value, Level defaultValue) {
if(value == null) {
return defaultValue;
}
value = value.trim();
- final int hashIndex = value.indexOf('#');
+ int hashIndex = value.indexOf('#');
if (hashIndex == -1) {
if("NULL".equalsIgnoreCase(value)) {
return null;
@@ -175,8 +177,8 @@ public final class OptionConverter {
Level result = defaultValue;
- final String clazz = value.substring(hashIndex+1);
- final String levelName = value.substring(0, hashIndex);
+ String clazz = value.substring(hashIndex+1);
+ String levelName = value.substring(0, hashIndex);
// This is degenerate case but you never know.
if("NULL".equalsIgnoreCase(levelName)) {
@@ -187,36 +189,35 @@ public final class OptionConverter {
+ ":pri=[" + levelName + "]");
try {
- final Class customLevel = Loader.loadClass(clazz);
+ Class<?> customLevel = Loader.loadClass(clazz);
// get a ref to the specified class' static method
// toLevel(String, org.apache.log4j.Level)
- final Class[] paramTypes = new Class[] { String.class, Level.class
- };
- final java.lang.reflect.Method toLevelMethod =
+ Class<?>[] paramTypes = new Class[] { String.class, Level.class };
+ java.lang.reflect.Method toLevelMethod =
customLevel.getMethod("toLevel", paramTypes);
// now call the toLevel method, passing level string + default
- final Object[] params = new Object[] {levelName, defaultValue};
- final Object o = toLevelMethod.invoke(null, params);
+ Object[] params = new Object[] {levelName, defaultValue};
+ Object o = toLevelMethod.invoke(null, params);
result = (Level) o;
- } catch(final ClassNotFoundException e) {
+ } catch(ClassNotFoundException e) {
LOGGER.warn("custom level class [" + clazz + "] not found.");
- } catch(final NoSuchMethodException e) {
+ } catch(NoSuchMethodException e) {
LOGGER.warn("custom level class [" + clazz + "]"
+ " does not have a class function toLevel(String, Level)", e);
- } catch(final java.lang.reflect.InvocationTargetException e) {
+ } catch(java.lang.reflect.InvocationTargetException e) {
if (e.getTargetException() instanceof InterruptedException
|| e.getTargetException() instanceof InterruptedIOException) {
Thread.currentThread().interrupt();
}
LOGGER.warn("custom level class [" + clazz + "]" + " could not be instantiated", e);
- } catch(final ClassCastException e) {
+ } catch(ClassCastException e) {
LOGGER.warn("class [" + clazz + "] is not a subclass of org.apache.log4j.Level", e);
- } catch(final IllegalAccessException e) {
+ } catch(IllegalAccessException e) {
LOGGER.warn("class ["+clazz+ "] cannot be instantiated due to access restrictions", e);
- } catch(final RuntimeException e) {
+ } catch(RuntimeException e) {
LOGGER.warn("class ["+clazz+"], level [" + levelName + "] conversion failed.", e);
}
return result;
@@ -290,15 +291,15 @@ public final class OptionConverter {
* @return The created object.
*/
public static Object instantiateByClassName(final String className, final Class<?> superClass,
- final Object defaultValue) {
+ final Object defaultValue) {
if (className != null) {
try {
final Class<?> classObj = Loader.loadClass(className);
if (!superClass.isAssignableFrom(classObj)) {
LOGGER.error("A \"{}\" object is not assignable to a \"{}\" variable.", className,
- superClass.getName());
+ superClass.getName());
LOGGER.error("The class \"{}\" was loaded by [{}] whereas object of type [{}] was loaded by [{}].",
- superClass.getName(), superClass.getClassLoader(), classObj.getTypeName(), classObj.getName());
+ superClass.getName(), superClass.getClassLoader(), classObj.getTypeName(), classObj.getName());
return defaultValue;
}
return classObj.newInstance();
@@ -347,7 +348,12 @@ public final class OptionConverter {
* @throws IllegalArgumentException if <code>val</code> is malformed.
*/
public static String substVars(final String val, final Properties props) throws
- IllegalArgumentException {
+ IllegalArgumentException {
+ return substVars(val, props, new ArrayList<>());
+ }
+
+ private static String substVars(final String val, final Properties props, List<String> keys)
+ throws IllegalArgumentException {
final StringBuilder sbuf = new StringBuilder();
@@ -370,8 +376,8 @@ public final class OptionConverter {
k = val.indexOf(DELIM_STOP, j);
if (k == -1) {
throw new IllegalArgumentException(Strings.dquote(val)
- + " has no closing brace. Opening brace at position " + j
- + '.');
+ + " has no closing brace. Opening brace at position " + j
+ + '.');
}
j += DELIM_START_LEN;
final String key = val.substring(j, k);
@@ -383,13 +389,21 @@ public final class OptionConverter {
}
if (replacement != null) {
+
// Do variable substitution on the replacement string
// such that we can solve "Hello ${x2}" as "Hello p1"
// the where the properties are
// x1=p1
// x2=${x1}
- final String recursiveReplacement = substVars(replacement, props);
- sbuf.append(recursiveReplacement);
+ if (!keys.contains(key)) {
+ List<String> usedKeys = new ArrayList<>(keys);
+ usedKeys.add(key);
+ final String recursiveReplacement = substVars(replacement, props, usedKeys);
+ sbuf.append(recursiveReplacement);
+ } else {
+ sbuf.append(replacement);
+ }
+
}
i = k + DELIM_STOP_LEN;
}
diff --git a/src/changes/changes.xml b/src/changes/changes.xml
index 943fadc..8647319 100644
--- a/src/changes/changes.xml
+++ b/src/changes/changes.xml
@@ -176,6 +176,9 @@
</release>
<release version="2.17.2" date="20YY-MM-DD" description="GA Release 2.17.2">
<!-- FIXES -->
+ <action issue="LOG4J2-3306" dev="rgoers" type="fix">
+ OptionConverter could cause a StackOverflowError.
+ </action>
<action dev="ggregory" type="fix">
Log4j 1.2 bridge class ConsoleAppender should extend WriterAppender and provide better compatibility with custom appenders.
</action>