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 <[email protected]> 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>
