This is an automated email from the ASF dual-hosted git repository. rgoers pushed a commit to branch log4j-2.12 in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git
The following commit(s) were added to refs/heads/log4j-2.12 by this push: new bf7e916 Fix string substitution recursion (#641) bf7e916 is described below commit bf7e916df6335713fe2219c7b3b523fb509deabc Author: Carter Kozak <cko...@apache.org> AuthorDate: Sun Dec 19 01:42:37 2021 -0500 Fix string substitution recursion (#641) cherry-pick of 806023265f8c905b2dd1d81fd2458f64b2ea0b5e --- .../log4j/config/Log4j1ConfigurationParser.java | 5 +- .../apache/logging/log4j/spi/AbstractLogger.java | 10 +- .../log4j/core/config/AbstractConfiguration.java | 18 +- .../logging/log4j/core/config/AppenderControl.java | 4 +- .../log4j/core/config/ConfigurationFactory.java | 3 +- .../config/composite/CompositeConfiguration.java | 2 +- .../log4j/core/config/json/JsonConfiguration.java | 2 +- .../core/config/plugins/util/PluginBuilder.java | 17 +- .../log4j/core/config/xml/XmlConfiguration.java | 2 +- .../core/lookup/ConfigurationStrSubstitutor.java | 63 ++++ .../log4j/core/lookup/ContextMapLookup.java | 2 +- .../logging/log4j/core/lookup/DateLookup.java | 2 +- .../logging/log4j/core/lookup/Interpolator.java | 2 +- .../log4j/core/lookup/RuntimeStrSubstitutor.java | 61 ++++ .../logging/log4j/core/lookup/StrSubstitutor.java | 342 +++++++++++++++------ ...rnResolverDoesNotEvaluateThreadContextTest.java | 116 +++++++ .../RoutingAppenderKeyLookupEvaluationTest.java | 94 ++++++ .../log4j/core/lookup/StrSubstitutorTest.java | 140 ++++++++- ...{log4j-routing.xml => log4j-routing-lookup.xml} | 34 +- .../src/test/resources/log4j-routing-purge.xml | 8 +- log4j-core/src/test/resources/log4j-routing.json | 5 +- .../src/test/resources/log4j-routing.properties | 4 +- log4j-core/src/test/resources/log4j-routing.xml | 5 +- log4j-core/src/test/resources/log4j-routing2.json | 5 +- .../log4j2-pattern-layout-with-context.xml | 34 ++ .../apache/logging/log4j/lookup/CustomLookup.java | 3 + .../logging/log4j/lookup/MapMessageLookup.java | 3 + .../logging/log4j/web/Log4jWebInitializerImpl.java | 3 +- src/changes/changes.xml | 3 + 29 files changed, 812 insertions(+), 180 deletions(-) diff --git a/log4j-1.2-api/src/main/java/org/apache/log4j/config/Log4j1ConfigurationParser.java b/log4j-1.2-api/src/main/java/org/apache/log4j/config/Log4j1ConfigurationParser.java index 112ab42..3630ce9 100644 --- a/log4j-1.2-api/src/main/java/org/apache/log4j/config/Log4j1ConfigurationParser.java +++ b/log4j-1.2-api/src/main/java/org/apache/log4j/config/Log4j1ConfigurationParser.java @@ -39,6 +39,7 @@ import org.apache.logging.log4j.core.config.builder.api.LayoutComponentBuilder; import org.apache.logging.log4j.core.config.builder.api.LoggerComponentBuilder; import org.apache.logging.log4j.core.config.builder.api.RootLoggerComponentBuilder; import org.apache.logging.log4j.core.config.builder.impl.BuiltConfiguration; +import org.apache.logging.log4j.core.lookup.ConfigurationStrSubstitutor; import org.apache.logging.log4j.core.lookup.StrSubstitutor; import org.apache.logging.log4j.status.StatusLogger; import org.apache.logging.log4j.util.Strings; @@ -89,8 +90,8 @@ public class Log4j1ConfigurationParser { throws IOException { try { properties.load(input); - strSubstitutorProperties = new StrSubstitutor(properties); - strSubstitutorSystem = new StrSubstitutor(System.getProperties()); + strSubstitutorProperties = new ConfigurationStrSubstitutor(properties); + strSubstitutorSystem = new ConfigurationStrSubstitutor(System.getProperties()); final String rootCategoryValue = getLog4jValue(ROOTCATEGORY); final String rootLoggerValue = getLog4jValue(ROOTLOGGER); if (rootCategoryValue == null && rootLoggerValue == null) { diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/spi/AbstractLogger.java b/log4j-api/src/main/java/org/apache/logging/log4j/spi/AbstractLogger.java index 897dcdc..65eff86 100644 --- a/log4j-api/src/main/java/org/apache/logging/log4j/spi/AbstractLogger.java +++ b/log4j-api/src/main/java/org/apache/logging/log4j/spi/AbstractLogger.java @@ -2089,8 +2089,8 @@ public abstract class AbstractLogger implements ExtendedLogger, LocationAwareLog try { incrementRecursionDepth(); log(level, marker, fqcn, location, message, throwable); - } catch (Exception ex) { - handleLogMessageException(ex, fqcn, message); + } catch (Throwable t) { + handleLogMessageException(t, fqcn, message); } finally { decrementRecursionDepth(); ReusableMessageFactory.release(message); @@ -2188,9 +2188,9 @@ public abstract class AbstractLogger implements ExtendedLogger, LocationAwareLog final Throwable throwable) { try { log(level, marker, fqcn, location, msg, throwable); - } catch (final Exception e) { + } catch (final Throwable t) { // LOG4J2-1990 Log4j2 suppresses all exceptions that occur once application called the logger - handleLogMessageException(e, fqcn, msg); + handleLogMessageException(t, fqcn, msg); } } @@ -2203,7 +2203,7 @@ public abstract class AbstractLogger implements ExtendedLogger, LocationAwareLog // LOG4J2-1990 Log4j2 suppresses all exceptions that occur once application called the logger // TODO Configuration setting to propagate exceptions back to the caller *if requested* - private void handleLogMessageException(final Exception exception, final String fqcn, final Message msg) { + private void handleLogMessageException(final Throwable exception, final String fqcn, final Message msg) { if (exception instanceof LoggingException) { throw (LoggingException) exception; } diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java index 42e9940..c45d7fc 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java @@ -54,8 +54,10 @@ import org.apache.logging.log4j.core.config.plugins.util.PluginManager; import org.apache.logging.log4j.core.config.plugins.util.PluginType; import org.apache.logging.log4j.core.filter.AbstractFilterable; import org.apache.logging.log4j.core.layout.PatternLayout; +import org.apache.logging.log4j.core.lookup.ConfigurationStrSubstitutor; import org.apache.logging.log4j.core.lookup.Interpolator; import org.apache.logging.log4j.core.lookup.MapLookup; +import org.apache.logging.log4j.core.lookup.RuntimeStrSubstitutor; import org.apache.logging.log4j.core.lookup.StrLookup; import org.apache.logging.log4j.core.lookup.StrSubstitutor; import org.apache.logging.log4j.core.net.Advertiser; @@ -127,7 +129,8 @@ public abstract class AbstractConfiguration extends AbstractFilterable implement private List<CustomLevelConfig> customLevels = Collections.emptyList(); private final ConcurrentMap<String, String> propertyMap = new ConcurrentHashMap<>(); private final StrLookup tempLookup = new Interpolator(propertyMap); - private final StrSubstitutor subst = new StrSubstitutor(tempLookup); + private final StrSubstitutor subst = new RuntimeStrSubstitutor(tempLookup); + private final StrSubstitutor configurationStrSubstitutor = new ConfigurationStrSubstitutor(subst); private LoggerConfig root = new LoggerConfig(); private final ConcurrentMap<String, Object> componentMap = new ConcurrentHashMap<>(); private final ConfigurationSource configurationSource; @@ -215,6 +218,7 @@ public abstract class AbstractConfiguration extends AbstractFilterable implement public void initialize() { LOGGER.debug(Version.getProductString() + " initializing configuration {}", this); subst.setConfiguration(this); + configurationStrSubstitutor.setConfiguration(this); try { scriptManager = new ScriptManager(this, watchManager); } catch (final LinkageError | Exception e) { @@ -532,12 +536,16 @@ public abstract class AbstractConfiguration extends AbstractFilterable implement final Node first = rootNode.getChildren().get(0); createConfiguration(first, null); if (first.getObject() != null) { - subst.setVariableResolver((StrLookup) first.getObject()); + StrLookup lookup = (StrLookup) first.getObject(); + subst.setVariableResolver(lookup); + configurationStrSubstitutor.setVariableResolver(lookup); } } else { final Map<String, String> map = this.getComponent(CONTEXT_PROPERTIES); final StrLookup lookup = map == null ? null : new MapLookup(map); - subst.setVariableResolver(new Interpolator(lookup, pluginPackages)); + Interpolator interpolator = new Interpolator(lookup, pluginPackages); + subst.setVariableResolver(interpolator); + configurationStrSubstitutor.setVariableResolver(interpolator); } boolean setLoggers = false; @@ -714,6 +722,10 @@ public abstract class AbstractConfiguration extends AbstractFilterable implement return subst; } + public StrSubstitutor getConfigurationStrSubstitutor() { + return configurationStrSubstitutor; + } + @Override public void setAdvertiser(final Advertiser advertiser) { this.advertiser = advertiser; diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/AppenderControl.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/AppenderControl.java index 8194ffc..a17b359 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/AppenderControl.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/AppenderControl.java @@ -156,8 +156,8 @@ public class AppenderControl extends AbstractFilterable { appender.append(event); } catch (final RuntimeException ex) { handleAppenderError(event, ex); - } catch (final Exception ex) { - handleAppenderError(event, new AppenderLoggingException(ex)); + } catch (final Throwable t) { + handleAppenderError(event, new AppenderLoggingException(t)); } } diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationFactory.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationFactory.java index c0f2b23..dd89e81 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationFactory.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationFactory.java @@ -37,6 +37,7 @@ import org.apache.logging.log4j.core.config.builder.api.ConfigurationBuilderFact import org.apache.logging.log4j.core.config.composite.CompositeConfiguration; import org.apache.logging.log4j.core.config.plugins.util.PluginManager; import org.apache.logging.log4j.core.config.plugins.util.PluginType; +import org.apache.logging.log4j.core.lookup.ConfigurationStrSubstitutor; import org.apache.logging.log4j.core.lookup.Interpolator; import org.apache.logging.log4j.core.lookup.StrSubstitutor; import org.apache.logging.log4j.core.net.UrlConnectionFactory; @@ -130,7 +131,7 @@ public abstract class ConfigurationFactory extends ConfigurationBuilderFactory { private static ConfigurationFactory configFactory = new Factory(); - protected final StrSubstitutor substitutor = new StrSubstitutor(new Interpolator()); + protected final StrSubstitutor substitutor = new ConfigurationStrSubstitutor(new Interpolator()); private static final Lock LOCK = new ReentrantLock(); diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/composite/CompositeConfiguration.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/composite/CompositeConfiguration.java index c4324d0..dc1f180 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/composite/CompositeConfiguration.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/composite/CompositeConfiguration.java @@ -79,7 +79,7 @@ public class CompositeConfiguration extends AbstractConfiguration implements Rec .withStatus(getDefaultStatus()); for (final Map.Entry<String, String> entry : rootNode.getAttributes().entrySet()) { final String key = entry.getKey(); - final String value = getStrSubstitutor().replace(entry.getValue()); + final String value = getConfigurationStrSubstitutor().replace(entry.getValue()); if ("status".equalsIgnoreCase(key)) { statusConfig.withStatus(value.toUpperCase()); } else if ("dest".equalsIgnoreCase(key)) { diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/json/JsonConfiguration.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/json/JsonConfiguration.java index c41a032..3e9dd9c 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/json/JsonConfiguration.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/json/JsonConfiguration.java @@ -71,7 +71,7 @@ public class JsonConfiguration extends AbstractConfiguration implements Reconfig int monitorIntervalSeconds = 0; for (final Map.Entry<String, String> entry : rootNode.getAttributes().entrySet()) { final String key = entry.getKey(); - final String value = getStrSubstitutor().replace(entry.getValue()); + final String value = getConfigurationStrSubstitutor().replace(entry.getValue()); // TODO: this duplicates a lot of the XmlConfiguration constructor if ("status".equalsIgnoreCase(key)) { statusConfig.withStatus(value); diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/util/PluginBuilder.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/util/PluginBuilder.java index f726a3a..0216b24 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/util/PluginBuilder.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/plugins/util/PluginBuilder.java @@ -40,6 +40,7 @@ import org.apache.logging.log4j.core.config.plugins.validation.ConstraintValidat import org.apache.logging.log4j.core.config.plugins.validation.ConstraintValidators; import org.apache.logging.log4j.core.config.plugins.visitors.PluginVisitor; import org.apache.logging.log4j.core.config.plugins.visitors.PluginVisitors; +import org.apache.logging.log4j.core.lookup.ConfigurationStrSubstitutor; import org.apache.logging.log4j.core.util.Builder; import org.apache.logging.log4j.core.util.ReflectionUtil; import org.apache.logging.log4j.core.util.TypeUtil; @@ -124,20 +125,20 @@ public class PluginBuilder implements Builder<Object> { } catch (final ConfigurationException e) { // LOG4J2-1908 LOGGER.error("Could not create plugin of type {} for element {}", this.clazz, node.getName(), e); return null; // no point in trying the factory method - } catch (final Exception e) { + } catch (final Throwable t) { LOGGER.error("Could not create plugin of type {} for element {}: {}", this.clazz, node.getName(), - (e instanceof InvocationTargetException ? ((InvocationTargetException) e).getCause() : e).toString(), e); + (t instanceof InvocationTargetException ? ((InvocationTargetException) t).getCause() : t).toString(), t); } // or fall back to factory method if no builder class is available try { final Method factory = findFactoryMethod(this.clazz); final Object[] params = generateParameters(factory); return factory.invoke(null, params); - } catch (final Exception e) { + } catch (final Throwable t) { LOGGER.error("Unable to invoke factory method in {} for element {}: {}", this.clazz, this.node.getName(), - (e instanceof InvocationTargetException ? ((InvocationTargetException) e).getCause() : e).toString(), e); + (t instanceof InvocationTargetException ? ((InvocationTargetException) t).getCause() : t).toString(), t); return null; } } @@ -180,7 +181,9 @@ public class PluginBuilder implements Builder<Object> { final Object value = visitor.setAliases(aliases) .setAnnotation(a) .setConversionType(field.getType()) - .setStrSubstitutor(configuration.getStrSubstitutor()) + .setStrSubstitutor(event == null + ? new ConfigurationStrSubstitutor(configuration.getStrSubstitutor()) + : configuration.getStrSubstitutor()) .setMember(field) .visit(configuration, node, event, log); // don't overwrite default values if the visitor gives us no value to inject @@ -253,7 +256,9 @@ public class PluginBuilder implements Builder<Object> { final Object value = visitor.setAliases(aliases) .setAnnotation(a) .setConversionType(types[i]) - .setStrSubstitutor(configuration.getStrSubstitutor()) + .setStrSubstitutor(event == null + ? new ConfigurationStrSubstitutor(configuration.getStrSubstitutor()) + : configuration.getStrSubstitutor()) .setMember(factory) .visit(configuration, node, event, log); // don't overwrite existing values if the visitor gives us no value to inject diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/xml/XmlConfiguration.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/xml/XmlConfiguration.java index 6a6c116..939c36c 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/xml/XmlConfiguration.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/xml/XmlConfiguration.java @@ -110,7 +110,7 @@ public class XmlConfiguration extends AbstractConfiguration implements Reconfigu int monitorIntervalSeconds = 0; for (final Map.Entry<String, String> entry : attrs.entrySet()) { final String key = entry.getKey(); - final String value = getStrSubstitutor().replace(entry.getValue()); + final String value = getConfigurationStrSubstitutor().replace(entry.getValue()); if ("status".equalsIgnoreCase(key)) { statusConfig.withStatus(value); } else if ("dest".equalsIgnoreCase(key)) { diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/ConfigurationStrSubstitutor.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/ConfigurationStrSubstitutor.java new file mode 100644 index 0000000..1287721 --- /dev/null +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/ConfigurationStrSubstitutor.java @@ -0,0 +1,63 @@ +/* + * 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.lookup; + +import java.util.Map; +import java.util.Properties; + +/** + * {@link RuntimeStrSubstitutor} is a {@link StrSubstitutor} which only supports recursive evaluation of lookups. + * This can be dangerous when combined with user-provided inputs, and should only be used on data directly from + * a configuration. + */ +public final class ConfigurationStrSubstitutor extends StrSubstitutor { + + public ConfigurationStrSubstitutor() { + } + + public ConfigurationStrSubstitutor(final Map<String, String> valueMap) { + super(valueMap); + } + + public ConfigurationStrSubstitutor(final Properties properties) { + super(properties); + } + + public ConfigurationStrSubstitutor(final StrLookup lookup) { + super(lookup); + } + + public ConfigurationStrSubstitutor(final StrSubstitutor other) { + super(other); + } + + @Override + boolean isRecursiveEvaluationAllowed() { + return true; + } + + @Override + void setRecursiveEvaluationAllowed(final boolean recursiveEvaluationAllowed) { + throw new UnsupportedOperationException( + "recursiveEvaluationAllowed cannot be modified within ConfigurationStrSubstitutor"); + } + + @Override + public String toString() { + return "ConfigurationStrSubstitutor{" + super.toString() + "}"; + } +} diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/ContextMapLookup.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/ContextMapLookup.java index 220d17a..ef60616 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/ContextMapLookup.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/ContextMapLookup.java @@ -55,6 +55,6 @@ public class ContextMapLookup implements StrLookup { */ @Override public String lookup(final LogEvent event, final String key) { - return event.getContextData().getValue(key); + return event == null ? null : event.getContextData().<String>getValue(key); } } diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/DateLookup.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/DateLookup.java index 3e630b0..3bcecf7 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/DateLookup.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/DateLookup.java @@ -54,7 +54,7 @@ public class DateLookup implements StrLookup { */ @Override public String lookup(final LogEvent event, final String key) { - return formatDate(event.getTimeMillis(), key); + return event == null ? lookup(key) : formatDate(event.getTimeMillis(), key); } private String formatDate(final long date, final String format) { diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/Interpolator.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/Interpolator.java index 01ee511..c8409ab 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/Interpolator.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/Interpolator.java @@ -47,7 +47,7 @@ public class Interpolator extends AbstractConfigurationAwareLookup { private static final Logger LOGGER = StatusLogger.getLogger(); /** Constant for the prefix separator. */ - private static final char PREFIX_SEPARATOR = ':'; + static final char PREFIX_SEPARATOR = ':'; private final Map<String, StrLookup> strLookupMap = new HashMap<>(); diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/RuntimeStrSubstitutor.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/RuntimeStrSubstitutor.java new file mode 100644 index 0000000..f002c6e --- /dev/null +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/RuntimeStrSubstitutor.java @@ -0,0 +1,61 @@ +/* + * 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.lookup; + +import java.util.Map; +import java.util.Properties; + +/** + * {@link RuntimeStrSubstitutor} is a {@link StrSubstitutor} which only supports evaluation of top-level lookups. + */ +public final class RuntimeStrSubstitutor extends StrSubstitutor { + + public RuntimeStrSubstitutor() { + } + + public RuntimeStrSubstitutor(final Map<String, String> valueMap) { + super(valueMap); + } + + public RuntimeStrSubstitutor(final Properties properties) { + super(properties); + } + + public RuntimeStrSubstitutor(final StrLookup lookup) { + super(lookup); + } + + public RuntimeStrSubstitutor(final StrSubstitutor other) { + super(other); + } + + @Override + boolean isRecursiveEvaluationAllowed() { + return false; + } + + @Override + void setRecursiveEvaluationAllowed(final boolean recursiveEvaluationAllowed) { + throw new UnsupportedOperationException( + "recursiveEvaluationAllowed cannot be modified within RuntimeStrSubstitutor"); + } + + @Override + public String toString() { + return "RuntimeStrSubstitutor{" + super.toString() + "}"; + } +} diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/StrSubstitutor.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/StrSubstitutor.java index 405ca28..4818ab0 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/StrSubstitutor.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/StrSubstitutor.java @@ -22,11 +22,13 @@ import java.util.HashMap; import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Properties; import org.apache.logging.log4j.core.LogEvent; import org.apache.logging.log4j.core.config.Configuration; import org.apache.logging.log4j.core.config.ConfigurationAware; +import org.apache.logging.log4j.status.StatusLogger; import org.apache.logging.log4j.util.Strings; /** @@ -158,7 +160,11 @@ public class StrSubstitutor implements ConfigurationAware { /** * Constant for the default value delimiter of a variable. */ - public static final StrMatcher DEFAULT_VALUE_DELIMITER = StrMatcher.stringMatcher(":-"); + public static final String DEFAULT_VALUE_DELIMITER_STRING = ":-"; + public static final StrMatcher DEFAULT_VALUE_DELIMITER = StrMatcher.stringMatcher(DEFAULT_VALUE_DELIMITER_STRING); + + public static final String ESCAPE_DELIMITER_STRING = ":\\-"; + public static final StrMatcher DEFAULT_VALUE_ESCAPE_DELIMITER = StrMatcher.stringMatcher(ESCAPE_DELIMITER_STRING); private static final int BUF_SIZE = 256; @@ -180,9 +186,15 @@ public class StrSubstitutor implements ConfigurationAware { /** * Stores the default variable value delimiter */ + private String valueDelimiterString; private StrMatcher valueDelimiterMatcher; /** + * Escape string to avoid matching the value delimiter matcher; + */ + private StrMatcher valueEscapeDelimiterMatcher; + + /** * Variable resolution is delegated to an implementer of VariableResolver. */ private StrLookup variableResolver; @@ -197,6 +209,8 @@ public class StrSubstitutor implements ConfigurationAware { */ private Configuration configuration; + private boolean recursiveEvaluationAllowed; + //----------------------------------------------------------------------- /** * Creates a new instance with defaults for variable prefix and suffix @@ -323,7 +337,9 @@ public class StrSubstitutor implements ConfigurationAware { public StrSubstitutor(final StrLookup variableResolver, final StrMatcher prefixMatcher, final StrMatcher suffixMatcher, final char escape) { - this(variableResolver, prefixMatcher, suffixMatcher, escape, DEFAULT_VALUE_DELIMITER); + this(variableResolver, prefixMatcher, suffixMatcher, escape, DEFAULT_VALUE_DELIMITER, + DEFAULT_VALUE_ESCAPE_DELIMITER); + this.valueDelimiterString = DEFAULT_VALUE_DELIMITER_STRING; } /** @@ -336,8 +352,8 @@ public class StrSubstitutor implements ConfigurationAware { * @param valueDelimiterMatcher the variable default value delimiter matcher, may be null * @throws IllegalArgumentException if the prefix or suffix is null */ - public StrSubstitutor( - final StrLookup variableResolver, final StrMatcher prefixMatcher, final StrMatcher suffixMatcher, final char escape, final StrMatcher valueDelimiterMatcher) { + public StrSubstitutor(final StrLookup variableResolver, final StrMatcher prefixMatcher, + final StrMatcher suffixMatcher, final char escape, final StrMatcher valueDelimiterMatcher) { this.setVariableResolver(variableResolver); this.setVariablePrefixMatcher(prefixMatcher); this.setVariableSuffixMatcher(suffixMatcher); @@ -345,6 +361,42 @@ public class StrSubstitutor implements ConfigurationAware { this.setValueDelimiterMatcher(valueDelimiterMatcher); } + /** + * Creates a new instance and initializes it. + * + * @param variableResolver the variable resolver, may be null + * @param prefixMatcher the prefix for variables, not null + * @param suffixMatcher the suffix for variables, not null + * @param escape the escape character + * @param valueDelimiterMatcher the variable default value delimiter matcher, may be null + * @param valueEscapeMatcher the matcher to escape defaulting, may be null. + * @throws IllegalArgumentException if the prefix or suffix is null + */ + public StrSubstitutor(final StrLookup variableResolver, final StrMatcher prefixMatcher, + final StrMatcher suffixMatcher, final char escape, final StrMatcher valueDelimiterMatcher, + final StrMatcher valueEscapeMatcher) { + this.setVariableResolver(variableResolver); + this.setVariablePrefixMatcher(prefixMatcher); + this.setVariableSuffixMatcher(suffixMatcher); + this.setEscapeChar(escape); + this.setValueDelimiterMatcher(valueDelimiterMatcher); + valueEscapeDelimiterMatcher = valueEscapeMatcher; + } + + StrSubstitutor(final StrSubstitutor other) { + Objects.requireNonNull(other, "other"); + this.setVariableResolver(other.getVariableResolver()); + this.setVariablePrefixMatcher(other.getVariablePrefixMatcher()); + this.setVariableSuffixMatcher(other.getVariableSuffixMatcher()); + this.setEscapeChar(other.getEscapeChar()); + this.setValueDelimiterMatcher(other.valueDelimiterMatcher); + this.valueEscapeDelimiterMatcher = other.valueEscapeDelimiterMatcher; + this.configuration = other.configuration; + this.recursiveEvaluationAllowed = other.isRecursiveEvaluationAllowed(); + this.enableSubstitutionInVariables = other.isEnableSubstitutionInVariables(); + this.valueDelimiterString = other.valueDelimiterString; + } + //----------------------------------------------------------------------- /** * Replaces all the occurrences of variables in the given source object with @@ -405,6 +457,11 @@ public class StrSubstitutor implements ConfigurationAware { return map; } + private static String handleFailedReplacement(String input, Throwable throwable) { + StatusLogger.getLogger().error("Replacement failed on {}", input, throwable); + return input; + } + //----------------------------------------------------------------------- /** * Replaces all the occurrences of variables with their matching values @@ -430,8 +487,12 @@ public class StrSubstitutor implements ConfigurationAware { return null; } final StringBuilder buf = new StringBuilder(source); - if (!substitute(event, buf, 0, source.length())) { - return source; + try { + if (!substitute(event, buf, 0, source.length())) { + return source; + } + } catch (Throwable t) { + return handleFailedReplacement(source, t); } return buf.toString(); } @@ -472,8 +533,12 @@ public class StrSubstitutor implements ConfigurationAware { return null; } final StringBuilder buf = new StringBuilder(length).append(source, offset, length); - if (!substitute(event, buf, 0, length)) { - return source.substring(offset, offset + length); + try { + if (!substitute(event, buf, 0, length)) { + return source.substring(offset, offset + length); + } + } catch (Throwable t) { + return handleFailedReplacement(source, t); } return buf.toString(); } @@ -506,7 +571,11 @@ public class StrSubstitutor implements ConfigurationAware { return null; } final StringBuilder buf = new StringBuilder(source.length).append(source); - substitute(event, buf, 0, source.length); + try { + substitute(event, buf, 0, source.length); + } catch (Throwable t) { + return handleFailedReplacement(new String(source), t); + } return buf.toString(); } @@ -548,7 +617,11 @@ public class StrSubstitutor implements ConfigurationAware { return null; } final StringBuilder buf = new StringBuilder(length).append(source, offset, length); - substitute(event, buf, 0, length); + try { + substitute(event, buf, 0, length); + } catch (Throwable t) { + return handleFailedReplacement(new String(source, offset, length), t); + } return buf.toString(); } @@ -580,7 +653,11 @@ public class StrSubstitutor implements ConfigurationAware { return null; } final StringBuilder buf = new StringBuilder(source.length()).append(source); - substitute(event, buf, 0, buf.length()); + try { + substitute(event, buf, 0, buf.length()); + } catch (Throwable t) { + return handleFailedReplacement(source.toString(), t); + } return buf.toString(); } @@ -622,7 +699,11 @@ public class StrSubstitutor implements ConfigurationAware { return null; } final StringBuilder buf = new StringBuilder(length).append(source, offset, length); - substitute(event, buf, 0, length); + try { + substitute(event, buf, 0, length); + } catch (Throwable t) { + return handleFailedReplacement(source.substring(offset, offset + length), t); + } return buf.toString(); } @@ -654,7 +735,11 @@ public class StrSubstitutor implements ConfigurationAware { return null; } final StringBuilder buf = new StringBuilder(source.length()).append(source); - substitute(event, buf, 0, buf.length()); + try { + substitute(event, buf, 0, buf.length()); + } catch (Throwable t) { + return handleFailedReplacement(source.toString(), t); + } return buf.toString(); } /** @@ -695,7 +780,11 @@ public class StrSubstitutor implements ConfigurationAware { return null; } final StringBuilder buf = new StringBuilder(length).append(source, offset, length); - substitute(event, buf, 0, length); + try { + substitute(event, buf, 0, length); + } catch (Throwable t) { + return handleFailedReplacement(source.substring(offset, offset + length), t); + } return buf.toString(); } @@ -725,8 +814,13 @@ public class StrSubstitutor implements ConfigurationAware { if (source == null) { return null; } - final StringBuilder buf = new StringBuilder().append(source); - substitute(event, buf, 0, buf.length()); + String stringValue = String.valueOf(source); + final StringBuilder buf = new StringBuilder(stringValue.length()).append(stringValue); + try { + substitute(event, buf, 0, buf.length()); + } catch (Throwable t) { + return handleFailedReplacement(stringValue, t); + } return buf.toString(); } @@ -784,7 +878,12 @@ public class StrSubstitutor implements ConfigurationAware { return false; } final StringBuilder buf = new StringBuilder(length).append(source, offset, length); - if (!substitute(event, buf, 0, length)) { + try { + if (!substitute(event, buf, 0, length)) { + return false; + } + } catch (Throwable t) { + StatusLogger.getLogger().error("Replacement failed on {}", source, t); return false; } source.replace(offset, offset + length, buf.toString()); @@ -909,100 +1008,123 @@ public class StrSubstitutor implements ConfigurationAware { final int startMatchLen = prefixMatcher.isMatch(chars, pos, offset, bufEnd); if (startMatchLen == 0) { pos++; + } else // found variable start marker + if (pos > offset && chars[pos - 1] == escape) { + // escaped + buf.deleteCharAt(pos - 1); + chars = getChars(buf); + lengthChange--; + altered = true; + bufEnd--; } else { - // found variable start marker - if (pos > offset && chars[pos - 1] == escape) { - // escaped - buf.deleteCharAt(pos - 1); - chars = getChars(buf); - lengthChange--; - altered = true; - bufEnd--; - } else { - // find suffix - final int startPos = pos; - pos += startMatchLen; - int endMatchLen = 0; - int nestedVarCount = 0; - while (pos < bufEnd) { - if (substitutionInVariablesEnabled - && (endMatchLen = prefixMatcher.isMatch(chars, pos, offset, bufEnd)) != 0) { - // found a nested variable start - nestedVarCount++; - pos += endMatchLen; - continue; - } + // find suffix + final int startPos = pos; + pos += startMatchLen; + int endMatchLen = 0; + int nestedVarCount = 0; + while (pos < bufEnd) { + if (substitutionInVariablesEnabled + && (endMatchLen = prefixMatcher.isMatch(chars, pos, offset, bufEnd)) != 0) { + // found a nested variable start + nestedVarCount++; + pos += endMatchLen; + continue; + } - endMatchLen = suffixMatcher.isMatch(chars, pos, offset, bufEnd); - if (endMatchLen == 0) { - pos++; - } else { - // found variable end marker - if (nestedVarCount == 0) { - String varNameExpr = new String(chars, startPos + startMatchLen, pos - startPos - startMatchLen); - if (substitutionInVariablesEnabled) { - final StringBuilder bufName = new StringBuilder(varNameExpr); - substitute(event, bufName, 0, bufName.length()); - varNameExpr = bufName.toString(); + endMatchLen = suffixMatcher.isMatch(chars, pos, offset, bufEnd); + if (endMatchLen == 0) { + pos++; + } else { + // found variable end marker + if (nestedVarCount == 0) { + String varNameExpr = new String(chars, startPos + startMatchLen, pos - startPos - startMatchLen); + if (substitutionInVariablesEnabled) { + // initialize priorVariables if they're not already set + if (priorVariables == null) { + priorVariables = new ArrayList<>(); } - pos += endMatchLen; - final int endPos = pos; - - String varName = varNameExpr; - String varDefaultValue = null; - - if (valueDelimiterMatcher != null) { - final char [] varNameExprChars = varNameExpr.toCharArray(); - int valueDelimiterMatchLen = 0; - for (int i = 0; i < varNameExprChars.length; i++) { - // if there's any nested variable when nested variable substitution disabled, then stop resolving name and default value. - if (!substitutionInVariablesEnabled - && prefixMatcher.isMatch(varNameExprChars, i, i, varNameExprChars.length) != 0) { + final StringBuilder bufName = new StringBuilder(varNameExpr); + substitute(event, bufName, 0, bufName.length(), priorVariables); + varNameExpr = bufName.toString(); + } + pos += endMatchLen; + final int endPos = pos; + + String varName = varNameExpr; + String varDefaultValue = null; + + if (valueDelimiterMatcher != null) { + final char [] varNameExprChars = varNameExpr.toCharArray(); + int valueDelimiterMatchLen = 0; + for (int i = 0; i < varNameExprChars.length; i++) { + // if there's any nested variable when nested variable substitution disabled, then stop resolving name and default value. + if (!substitutionInVariablesEnabled + && prefixMatcher.isMatch(varNameExprChars, i, i, varNameExprChars.length) != 0) { + break; + } + if (valueEscapeDelimiterMatcher != null) { + int matchLen = valueEscapeDelimiterMatcher.isMatch(varNameExprChars, i); + if (matchLen != 0) { + String varNamePrefix = varNameExpr.substring(0, i) + Interpolator.PREFIX_SEPARATOR; + varName = varNamePrefix + varNameExpr.substring(i + matchLen - 1); + for (int j = i + matchLen; j < varNameExprChars.length; ++j){ + if ((valueDelimiterMatchLen = valueDelimiterMatcher.isMatch(varNameExprChars, j)) != 0) { + varName = varNamePrefix + varNameExpr.substring(i + matchLen, j); + varDefaultValue = varNameExpr.substring(j + valueDelimiterMatchLen); + break; + } + } break; - } - if ((valueDelimiterMatchLen = valueDelimiterMatcher.isMatch(varNameExprChars, i)) != 0) { + } else if ((valueDelimiterMatchLen = valueDelimiterMatcher.isMatch(varNameExprChars, i)) != 0) { varName = varNameExpr.substring(0, i); varDefaultValue = varNameExpr.substring(i + valueDelimiterMatchLen); break; } + } else if ((valueDelimiterMatchLen = valueDelimiterMatcher.isMatch(varNameExprChars, i)) != 0) { + varName = varNameExpr.substring(0, i); + varDefaultValue = varNameExpr.substring(i + valueDelimiterMatchLen); + break; } } + } - // on the first call initialize priorVariables - if (priorVariables == null) { - priorVariables = new ArrayList<>(); - priorVariables.add(new String(chars, offset, length + lengthChange)); - } + // on the first call initialize priorVariables + if (priorVariables == null) { + priorVariables = new ArrayList<>(); + priorVariables.add(new String(chars, offset, length + lengthChange)); + } - // handle cyclic substitution - checkCyclicSubstitution(varName, priorVariables); - priorVariables.add(varName); + // handle cyclic substitution + boolean isCyclic = isCyclicSubstitution(varName, priorVariables); - // resolve the variable - String varValue = resolveVariable(event, varName, buf, startPos, endPos); - if (varValue == null) { - varValue = varDefaultValue; - } - if (varValue != null) { - // recursive replace - final int varLen = varValue.length(); - buf.replace(startPos, endPos, varValue); - altered = true; - int change = substitute(event, buf, startPos, varLen, priorVariables); - change = change + (varLen - (endPos - startPos)); - pos += change; - bufEnd += change; - lengthChange += change; - chars = getChars(buf); // in case buffer was altered - } + // resolve the variable + String varValue = isCyclic ? null : resolveVariable(event, varName, buf, startPos, endPos); + if (varValue == null) { + varValue = varDefaultValue; + } + if (varValue != null) { + // recursive replace + final int varLen = varValue.length(); + buf.replace(startPos, endPos, varValue); + altered = true; + int change = isRecursiveEvaluationAllowed() + ? substitute(event, buf, startPos, varLen, priorVariables) + : 0; + change = change + (varLen - (endPos - startPos)); + pos += change; + bufEnd += change; + lengthChange += change; + chars = getChars(buf); // in case buffer was altered + } - // remove variable from the cyclic stack + // remove variable from the cyclic stack + if (!isCyclic) { priorVariables.remove(priorVariables.size() - 1); - break; } - nestedVarCount--; - pos += endMatchLen; + break; } + nestedVarCount--; + pos += endMatchLen; } } } @@ -1014,21 +1136,23 @@ public class StrSubstitutor implements ConfigurationAware { } /** - * Checks if the specified variable is already in the stack (list) of variables. + * Checks if the specified variable is already in the stack (list) of variables, adding the value + * if it's not already present. * * @param varName the variable name to check * @param priorVariables the list of prior variables + * @return true if this is a cyclic substitution */ - private void checkCyclicSubstitution(final String varName, final List<String> priorVariables) { + private boolean isCyclicSubstitution(final String varName, final List<String> priorVariables) { if (!priorVariables.contains(varName)) { - return; + priorVariables.add(varName); + return false; } final StringBuilder buf = new StringBuilder(BUF_SIZE); buf.append("Infinite loop in property interpolation of "); - buf.append(priorVariables.remove(0)); - buf.append(": "); appendWithSeparators(buf, priorVariables, "->"); - throw new IllegalStateException(buf.toString()); + StatusLogger.getLogger().warn(buf); + return true; } /** @@ -1057,7 +1181,12 @@ public class StrSubstitutor implements ConfigurationAware { if (resolver == null) { return null; } - return resolver.lookup(event, variableName); + try { + return resolver.lookup(event, variableName); + } catch (Throwable t) { + StatusLogger.getLogger().error("Resolver failed to lookup {}", variableName, t); + return null; + } } // Escape @@ -1294,6 +1423,9 @@ public class StrSubstitutor implements ConfigurationAware { setValueDelimiterMatcher(null); return this; } + String escapeValue = valueDelimiter.substring(0, valueDelimiter.length() - 1) + "\\" + + valueDelimiter.substring(valueDelimiter.length() - 1); + valueEscapeDelimiterMatcher = StrMatcher.stringMatcher(escapeValue); return setValueDelimiterMatcher(StrMatcher.stringMatcher(valueDelimiter)); } @@ -1343,6 +1475,14 @@ public class StrSubstitutor implements ConfigurationAware { this.enableSubstitutionInVariables = enableSubstitutionInVariables; } + boolean isRecursiveEvaluationAllowed() { + return recursiveEvaluationAllowed; + } + + void setRecursiveEvaluationAllowed(final boolean recursiveEvaluationAllowed) { + this.recursiveEvaluationAllowed = recursiveEvaluationAllowed; + } + private char[] getChars(final StringBuilder sb) { final char[] chars = new char[sb.length()]; sb.getChars(0, sb.length(), chars, 0); diff --git a/log4j-core/src/test/java/org/apache/logging/log4j/core/PatternResolverDoesNotEvaluateThreadContextTest.java b/log4j-core/src/test/java/org/apache/logging/log4j/core/PatternResolverDoesNotEvaluateThreadContextTest.java new file mode 100644 index 0000000..fc65e14 --- /dev/null +++ b/log4j-core/src/test/java/org/apache/logging/log4j/core/PatternResolverDoesNotEvaluateThreadContextTest.java @@ -0,0 +1,116 @@ +/* + * 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; + +import org.apache.logging.log4j.ThreadContext; +import org.apache.logging.log4j.junit.LoggerContextRule; +import org.apache.logging.log4j.test.appender.ListAppender; +import org.junit.Before; +import org.junit.ClassRule; +import org.junit.Test; + +import java.util.List; + +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.assertEquals; + +public class PatternResolverDoesNotEvaluateThreadContextTest { + + + private static final String CONFIG = "log4j2-pattern-layout-with-context.xml"; + private static final String PARAMETER = "user"; + private ListAppender listAppender; + + @ClassRule + public static LoggerContextRule context = new LoggerContextRule(CONFIG); + + @Before + public void before() { + listAppender = context.getRequiredAppender("list", ListAppender.class); + listAppender.clear(); + } + + @Test + public void testNoUserSet() { + Logger logger = context.getLogger(getClass()); + logger.info("This is a test"); + List<String> messages = listAppender.getMessages(); + assertTrue("No messages returned", messages != null && messages.size() > 0); + String message = messages.get(0); + assertEquals("INFO org.apache.logging.log4j.core." + + "PatternResolverDoesNotEvaluateThreadContextTest ${ctx:user} This is a test", message); + } + + @Test + public void testMessageIsNotLookedUp() { + Logger logger = context.getLogger(getClass()); + logger.info("This is a ${upper:test}"); + List<String> messages = listAppender.getMessages(); + assertTrue("No messages returned", messages != null && messages.size() > 0); + String message = messages.get(0); + assertEquals("INFO org.apache.logging.log4j.core." + + "PatternResolverDoesNotEvaluateThreadContextTest ${ctx:user} This is a ${upper:test}", message); + } + + @Test + public void testUser() { + Logger logger = context.getLogger(getClass()); + ThreadContext.put(PARAMETER, "123"); + try { + logger.info("This is a test"); + } finally { + ThreadContext.remove(PARAMETER); + } + List<String> messages = listAppender.getMessages(); + assertTrue("No messages returned", messages != null && messages.size() > 0); + String message = messages.get(0); + assertEquals("INFO org.apache.logging.log4j.core." + + "PatternResolverDoesNotEvaluateThreadContextTest 123 This is a test", message); + } + + @Test + public void testUserIsLookup() { + Logger logger = context.getLogger(getClass()); + ThreadContext.put(PARAMETER, "${java:version}"); + try { + logger.info("This is a test"); + } finally { + ThreadContext.remove(PARAMETER); + } + List<String> messages = listAppender.getMessages(); + assertTrue("No messages returned", messages != null && messages.size() > 0); + String message = messages.get(0); + assertEquals("INFO org.apache.logging.log4j.core." + + "PatternResolverDoesNotEvaluateThreadContextTest ${java:version} This is a test", message); + } + + @Test + public void testUserHasLookup() { + Logger logger = context.getLogger(getClass()); + ThreadContext.put(PARAMETER, "user${java:version}name"); + try { + logger.info("This is a test"); + } finally { + ThreadContext.remove(PARAMETER); + } + List<String> messages = listAppender.getMessages(); + assertTrue("No messages returned",messages != null && messages.size() > 0); + String message = messages.get(0); + assertEquals("INFO org.apache.logging.log4j.core." + + "PatternResolverDoesNotEvaluateThreadContextTest user${java:version}name This is a test", message); + } +} diff --git a/log4j-core/src/test/java/org/apache/logging/log4j/core/appender/routing/RoutingAppenderKeyLookupEvaluationTest.java b/log4j-core/src/test/java/org/apache/logging/log4j/core/appender/routing/RoutingAppenderKeyLookupEvaluationTest.java new file mode 100644 index 0000000..963240f --- /dev/null +++ b/log4j-core/src/test/java/org/apache/logging/log4j/core/appender/routing/RoutingAppenderKeyLookupEvaluationTest.java @@ -0,0 +1,94 @@ +/* + * 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.appender.routing; + +import org.apache.logging.log4j.ThreadContext; +import org.apache.logging.log4j.core.Logger; +import org.apache.logging.log4j.junit.LoggerContextRule; +import org.apache.logging.log4j.test.appender.ListAppender; +import org.junit.After; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +public class RoutingAppenderKeyLookupEvaluationTest { + private static final String CONFIG = "log4j-routing-lookup.xml"; + + private static final String KEY = "user"; + private ListAppender app; + + @Rule + public final LoggerContextRule loggerContextRule = new LoggerContextRule(CONFIG); + + @Before + public void setUp() throws Exception { + ThreadContext.remove(KEY); + this.app = this.loggerContextRule.getListAppender("List"); + } + + @After + public void tearDown() throws Exception { + this.app.clear(); + this.loggerContextRule.getLoggerContext().stop(); + ThreadContext.remove(KEY); + } + + @Test + public void testRoutingNoUser() { + Logger logger = loggerContextRule.getLogger(getClass()); + logger.warn("no user"); + String message = app.getMessages().get(0); + assertEquals("WARN ${ctx:user} no user", message); + } + + @Test + public void testRoutingDoesNotMatchRoute() { + Logger logger = loggerContextRule.getLogger(getClass()); + ThreadContext.put(KEY, "noRouteExists"); + logger.warn("unmatched user"); + assertTrue(app.getMessages().isEmpty()); + } + + @Test + public void testRoutingContainsLookup() { + Logger logger = loggerContextRule.getLogger(getClass()); + ThreadContext.put(KEY, "${java:version}"); + logger.warn("naughty user"); + String message = app.getMessages().get(0); + assertEquals("WARN ${java:version} naughty user", message); + } + + @Test + public void testRoutingMatchesEscapedLookup() { + Logger logger = loggerContextRule.getLogger(getClass()); + ThreadContext.put(KEY, "${upper:name}"); + logger.warn("naughty user"); + String message = app.getMessages().get(0); + assertEquals("WARN ${upper:name} naughty user", message); + } + + @Test + public void testRoutesThemselvesNotEvaluated() { + Logger logger = loggerContextRule.getLogger(getClass()); + ThreadContext.put(KEY, "NAME"); + logger.warn("unmatched user"); + assertTrue(app.getMessages().isEmpty()); + } +} diff --git a/log4j-core/src/test/java/org/apache/logging/log4j/core/lookup/StrSubstitutorTest.java b/log4j-core/src/test/java/org/apache/logging/log4j/core/lookup/StrSubstitutorTest.java index 41b7979..afb3bdd 100644 --- a/log4j-core/src/test/java/org/apache/logging/log4j/core/lookup/StrSubstitutorTest.java +++ b/log4j-core/src/test/java/org/apache/logging/log4j/core/lookup/StrSubstitutorTest.java @@ -20,15 +20,13 @@ import java.util.HashMap; import java.util.Map; import org.apache.logging.log4j.ThreadContext; +import org.apache.logging.log4j.core.LogEvent; import org.junit.AfterClass; import org.junit.BeforeClass; import org.junit.Test; import static org.junit.Assert.*; -/** - * - */ public class StrSubstitutorTest { private static final String TESTKEY = "TestKey"; @@ -48,7 +46,7 @@ public class StrSubstitutorTest { @Test public void testLookup() { - final Map<String, String> map = new HashMap<>(); + final Map<String, String> map = new HashMap<String, String>(); map.put(TESTKEY, TESTVAL); final StrLookup lookup = new Interpolator(new MapLookup(map)); final StrSubstitutor subst = new StrSubstitutor(lookup); @@ -68,7 +66,7 @@ public class StrSubstitutorTest { @Test public void testDefault() { - final Map<String, String> map = new HashMap<>(); + final Map<String, String> map = new HashMap<String, String>(); map.put(TESTKEY, TESTVAL); final StrLookup lookup = new Interpolator(new MapLookup(map)); final StrSubstitutor subst = new StrSubstitutor(lookup); @@ -77,4 +75,136 @@ public class StrSubstitutorTest { final String value = subst.replace("${sys:TestKey1:-${ctx:TestKey}}"); assertEquals("TestValue", value); } + + @Test + public void testDefaultReferencesLookupValue() { + final Map<String, String> map = new HashMap<String, String>(); + map.put(TESTKEY, "${java:version}"); + final StrLookup lookup = new Interpolator(new MapLookup(map)); + final StrSubstitutor subst = new StrSubstitutor(lookup); + subst.setRecursiveEvaluationAllowed(false); + final String value = subst.replace("${sys:TestKey1:-${ctx:TestKey}}"); + assertEquals("${java:version}", value); + } + + @Test + public void testInfiniteSubstitutionOnString() { + final StrLookup lookup = new Interpolator(new MapLookup(new HashMap<String, String>())); + final StrSubstitutor subst = new StrSubstitutor(lookup); + subst.setRecursiveEvaluationAllowed(true); + String infiniteSubstitution = "${${::-${::-$${::-j}}}}"; + assertEquals(infiniteSubstitution, subst.replace(infiniteSubstitution)); + } + + @Test + public void testInfiniteSubstitutionOnStringBuilder() { + final StrLookup lookup = new Interpolator(new MapLookup(new HashMap<String, String>())); + final StrSubstitutor subst = new StrSubstitutor(lookup); + subst.setRecursiveEvaluationAllowed(true); + String infiniteSubstitution = "${${::-${::-$${::-j}}}}"; + assertEquals(infiniteSubstitution, subst.replace(null, new StringBuilder(infiniteSubstitution))); + } + + @Test + public void testRecursiveSubstitution() { + final Map<String, String> map = new HashMap<String, String>(); + map.put("first", "${ctx:first}"); + map.put("second", "secondValue"); + final StrLookup lookup = new Interpolator(new MapLookup(map)); + final StrSubstitutor subst = new StrSubstitutor(lookup); + subst.setRecursiveEvaluationAllowed(true); + assertEquals("${ctx:first} and secondValue", subst.replace("${ctx:first} and ${ctx:second}")); + } + + @Test + public void testRecursiveWithDefault() { + final Map<String, String> map = new HashMap<String, String>(); + map.put("first", "${ctx:first:-default}"); + final StrLookup lookup = new Interpolator(new MapLookup(map)); + final StrSubstitutor subst = new StrSubstitutor(lookup); + subst.setRecursiveEvaluationAllowed(true); + assertEquals("default", subst.replace("${ctx:first}")); + } + + @Test + public void testRecursiveWithRecursiveDefault() { + final Map<String, String> map = new HashMap<String, String>(); + map.put("first", "${ctx:first:-${ctx:first}}"); + final StrLookup lookup = new Interpolator(new MapLookup(map)); + final StrSubstitutor subst = new StrSubstitutor(lookup); + subst.setRecursiveEvaluationAllowed(true); + assertEquals("${ctx:first}", subst.replace("${ctx:first}")); + } + + @Test + public void testNestedSelfReferenceWithRecursiveEvaluation() { + final Map<String, String> map = new HashMap<String, String>(); + map.put("first", "${${ctx:first}}"); + final StrLookup lookup = new Interpolator(new MapLookup(map)); + final StrSubstitutor subst = new StrSubstitutor(lookup); + subst.setRecursiveEvaluationAllowed(true); + assertEquals("${${ctx:first}}", subst.replace("${ctx:first}")); + } + + @Test + public void testRandomWithRecursiveDefault() { + final Map<String, String> map = new HashMap<String, String>(); + map.put("first", "${env:RANDOM:-${ctx:first}}"); + final StrLookup lookup = new Interpolator(new MapLookup(map)); + final StrSubstitutor subst = new StrSubstitutor(lookup); + subst.setRecursiveEvaluationAllowed(true); + assertEquals("${ctx:first}", subst.replace("${ctx:first}")); + } + + @Test + public void testNoRecursiveEvaluationWithDefault() { + final Map<String, String> map = new HashMap<String, String>(); + map.put("first", "${java:version}"); + map.put("second", "${java:runtime}"); + final StrLookup lookup = new Interpolator(new MapLookup(map)); + final StrSubstitutor subst = new StrSubstitutor(lookup); + subst.setRecursiveEvaluationAllowed(false); + assertEquals("${java:version}", subst.replace("${ctx:first:-${ctx:second}}")); + } + + @Test + public void testNoRecursiveEvaluationWithDepthOne() { + final Map<String, String> map = new HashMap<String, String>(); + map.put("first", "${java:version}"); + final StrLookup lookup = new Interpolator(new MapLookup(map)); + final StrSubstitutor subst = new StrSubstitutor(lookup); + subst.setRecursiveEvaluationAllowed(false); + assertEquals("${java:version}", subst.replace("${ctx:first}")); + } + + @Test + public void testLookupsNestedWithoutRecursiveEvaluation() { + final Map<String, String> map = new HashMap<String, String>(); + map.put("first", "${java:version}"); + final StrLookup lookup = new Interpolator(new MapLookup(map)); + final StrSubstitutor subst = new StrSubstitutor(lookup); + subst.setRecursiveEvaluationAllowed(false); + assertEquals("${java:version}", subst.replace("${${lower:C}t${lower:X}:first}")); + } + + @Test + public void testLookupThrows() { + final StrSubstitutor subst = new StrSubstitutor(new Interpolator(new StrLookup() { + + @Override + public String lookup(String key) { + if ("throw".equals(key)) { + throw new RuntimeException(); + } + return "success"; + } + + @Override + public String lookup(LogEvent event, String key) { + return lookup(key); + } + })); + subst.setRecursiveEvaluationAllowed(false); + assertEquals("success ${foo:throw} success", subst.replace("${foo:a} ${foo:throw} ${foo:c}")); + } } diff --git a/log4j-core/src/test/resources/log4j-routing.xml b/log4j-core/src/test/resources/log4j-routing-lookup.xml similarity index 51% copy from log4j-core/src/test/resources/log4j-routing.xml copy to log4j-core/src/test/resources/log4j-routing-lookup.xml index 4d83886..92f31c3 100644 --- a/log4j-core/src/test/resources/log4j-routing.xml +++ b/log4j-core/src/test/resources/log4j-routing-lookup.xml @@ -16,43 +16,23 @@ limitations under the License. --> -<Configuration status="OFF" name="RoutingTest"> - <Properties> - <Property name="filename">target/routing1/routingtest-$${sd:type}.log</Property> - </Properties> - <ThresholdFilter level="debug"/> - +<Configuration status="OFF" name="RoutingAppenderKeyLookupEvaluationTest"> <Appenders> - <Console name="STDOUT"> - <PatternLayout pattern="%m%n"/> - </Console> <List name="List"> - <ThresholdFilter level="debug"/> + <PatternLayout pattern="%p $${ctx:user} $${event:Message}"/> </List> <Routing name="Routing"> - <Routes pattern="$${sd:type}"> - <Route> - <RollingFile name="Routing-${sd:type}" fileName="${filename}" - filePattern="target/routing1/test1-${sd:type}.%i.log.gz"> - <PatternLayout> - <Pattern>%d %p %C{1.} [%t] %m%n</Pattern> - </PatternLayout> - <SizeBasedTriggeringPolicy size="500" /> - </RollingFile> - </Route> - <Route ref="STDOUT" key="Audit"/> - <Route ref="List" key="Service"/> + <Routes pattern="$${ctx:user:-none}"> + <Route ref="List" key="$${upper:name}"/> + <Route ref="List" key="none"/> + <Route ref="List" key="$${java:version}"/> </Routes> </Routing> </Appenders> <Loggers> - <Logger name="EventLogger" level="info" additivity="false"> + <Root level="debug"> <AppenderRef ref="Routing"/> - </Logger> - - <Root level="error"> - <AppenderRef ref="STDOUT"/> </Root> </Loggers> diff --git a/log4j-core/src/test/resources/log4j-routing-purge.xml b/log4j-core/src/test/resources/log4j-routing-purge.xml index d1de288..910ba80 100644 --- a/log4j-core/src/test/resources/log4j-routing-purge.xml +++ b/log4j-core/src/test/resources/log4j-routing-purge.xml @@ -17,10 +17,6 @@ --> <Configuration status="OFF" name="RoutingTest"> - <Properties> - <Property name="filename-idle">target/routing-purge-idle/routingtest-$${sd:id}.log</Property> - <Property name="filename-manual">target/routing-purge-manual/routingtest-$${sd:id}.log</Property> - </Properties> <ThresholdFilter level="debug"/> <Appenders> @@ -34,7 +30,7 @@ <Routing name="RoutingPurgeIdle"> <Routes pattern="$${sd:id}"> <Route> - <File name="Routing-${sd:id}" fileName="${filename-idle}"> + <File name="Routing-${sd:id}" fileName="target/routing-purge-idle/routingtest-${sd:id}.log"> <PatternLayout> <Pattern>%d %p %C{1.} [%t] %m%n</Pattern> </PatternLayout> @@ -58,7 +54,7 @@ <Routing name="RoutingPurgeManual"> <Routes pattern="$${sd:id}"> <Route> - <File name="Routing-${sd:id}" fileName="${filename-manual}"> + <File name="Routing-${sd:id}" fileName="target/routing-purge-manual/routingtest-${sd:id}.log"> <PatternLayout> <Pattern>%d %p %C{1.} [%t] %m%n</Pattern> </PatternLayout> diff --git a/log4j-core/src/test/resources/log4j-routing.json b/log4j-core/src/test/resources/log4j-routing.json index 809b3c2..4322d8d 100644 --- a/log4j-core/src/test/resources/log4j-routing.json +++ b/log4j-core/src/test/resources/log4j-routing.json @@ -15,9 +15,6 @@ * limitations under the license. */ { "configuration": { "status": "error", "name": "RoutingTest", - "properties": { - "property": { "name": "filename", "value" : "target/rolling1/rollingtest-$${sd:type}.log" } - }, "ThresholdFilter": { "level": "debug" }, "appenders": { "Console": { "name": "STDOUT", @@ -31,7 +28,7 @@ "Route": [ { "RollingFile": { - "name": "Rolling-${sd:type}", "fileName": "${filename}", + "name": "Rolling-${sd:type}", "fileName": "target/rolling1/rollingtest-${sd:type}.log", "filePattern": "target/rolling1/test1-${sd:type}.%i.log.gz", "PatternLayout": {"pattern": "%d %p %C{1.} [%t] %m%n"}, "SizeBasedTriggeringPolicy": { "size": "500" } diff --git a/log4j-core/src/test/resources/log4j-routing.properties b/log4j-core/src/test/resources/log4j-routing.properties index c365e35..0dc7547 100644 --- a/log4j-core/src/test/resources/log4j-routing.properties +++ b/log4j-core/src/test/resources/log4j-routing.properties @@ -16,8 +16,6 @@ status = error name = RoutingTest -property.filename = target/routing1/routingtestProps-$${sd:type}.log - filter.threshold.type = ThresholdFilter filter.threshold.level = debug @@ -33,7 +31,7 @@ appender.routing.routes.pattern = $${sd:type} appender.routing.routes.route1.type = Route appender.routing.routes.route1.rolling.type = RollingFile appender.routing.routes.route1.rolling.name = Routing-${sd:type} -appender.routing.routes.route1.rolling.fileName = ${filename} +appender.routing.routes.route1.rolling.fileName = target/routing1/routingtestProps-${sd:type}.log appender.routing.routes.route1.rolling.filePattern = target/routing1/test1-${sd:type}.%i.log.gz appender.routing.routes.route1.rolling.layout.type = PatternLayout appender.routing.routes.route1.rolling.layout.pattern = %d %p %C{1.} [%t] %m%n diff --git a/log4j-core/src/test/resources/log4j-routing.xml b/log4j-core/src/test/resources/log4j-routing.xml index 4d83886..003f7d7 100644 --- a/log4j-core/src/test/resources/log4j-routing.xml +++ b/log4j-core/src/test/resources/log4j-routing.xml @@ -17,9 +17,6 @@ --> <Configuration status="OFF" name="RoutingTest"> - <Properties> - <Property name="filename">target/routing1/routingtest-$${sd:type}.log</Property> - </Properties> <ThresholdFilter level="debug"/> <Appenders> @@ -32,7 +29,7 @@ <Routing name="Routing"> <Routes pattern="$${sd:type}"> <Route> - <RollingFile name="Routing-${sd:type}" fileName="${filename}" + <RollingFile name="Routing-${sd:type}" fileName="target/routing1/routingtest-${sd:type}.log" filePattern="target/routing1/test1-${sd:type}.%i.log.gz"> <PatternLayout> <Pattern>%d %p %C{1.} [%t] %m%n</Pattern> diff --git a/log4j-core/src/test/resources/log4j-routing2.json b/log4j-core/src/test/resources/log4j-routing2.json index fe50b37..af67454 100644 --- a/log4j-core/src/test/resources/log4j-routing2.json +++ b/log4j-core/src/test/resources/log4j-routing2.json @@ -15,9 +15,6 @@ * limitations under the license. */ { "configuration": { "status": "error", "name": "RoutingTest", - "properties": { - "property": { "name": "filename", "value" : "target/rolling1/rollingtest-$${sd:type}.log" } - }, "ThresholdFilter": { "level": "debug" }, "appenders": { "appender": [ @@ -28,7 +25,7 @@ "Route": [ { "RollingFile": { - "name": "Rolling-${sd:type}", "fileName": "${filename}", + "name": "Rolling-${sd:type}", "fileName": "target/rolling1/rollingtest-${sd:type}.log", "filePattern": "target/rolling1/test1-${sd:type}.%i.log.gz", "PatternLayout": {"pattern": "%d %p %C{1.} [%t] %m%n"}, "SizeBasedTriggeringPolicy": { "size": "500" } diff --git a/log4j-core/src/test/resources/log4j2-pattern-layout-with-context.xml b/log4j-core/src/test/resources/log4j2-pattern-layout-with-context.xml new file mode 100644 index 0000000..281e311 --- /dev/null +++ b/log4j-core/src/test/resources/log4j2-pattern-layout-with-context.xml @@ -0,0 +1,34 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- + 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. + +--> +<Configuration status="DEBUG" packages=""> + <Properties> + <Property name="pattern">%p %c $${ctx:user} $${event:Message}</Property> + </Properties> + <Appenders> + <List name="list"> + <PatternLayout pattern="${pattern}"/> + </List> + </Appenders> + + <Loggers> + <Root level="INFO"> + <AppenderRef ref="list"/> + </Root> + </Loggers> +</Configuration> diff --git a/log4j-samples/log4j-samples-loggerProperties/src/main/java/org/apache/logging/log4j/lookup/CustomLookup.java b/log4j-samples/log4j-samples-loggerProperties/src/main/java/org/apache/logging/log4j/lookup/CustomLookup.java index 8bd3415..2f9108e 100644 --- a/log4j-samples/log4j-samples-loggerProperties/src/main/java/org/apache/logging/log4j/lookup/CustomLookup.java +++ b/log4j-samples/log4j-samples-loggerProperties/src/main/java/org/apache/logging/log4j/lookup/CustomLookup.java @@ -50,6 +50,9 @@ public class CustomLookup extends AbstractLookup { */ @Override public String lookup(final LogEvent event, final String key) { + if (event == null) { + return null; + } try { final Map<String, String> properties = loggerProperties.get(event.getLoggerName()); if (properties == null) { diff --git a/log4j-samples/log4j-samples-loggerProperties/src/main/java/org/apache/logging/log4j/lookup/MapMessageLookup.java b/log4j-samples/log4j-samples-loggerProperties/src/main/java/org/apache/logging/log4j/lookup/MapMessageLookup.java index 5c444cb..59a01f0 100644 --- a/log4j-samples/log4j-samples-loggerProperties/src/main/java/org/apache/logging/log4j/lookup/MapMessageLookup.java +++ b/log4j-samples/log4j-samples-loggerProperties/src/main/java/org/apache/logging/log4j/lookup/MapMessageLookup.java @@ -52,6 +52,9 @@ public class MapMessageLookup extends AbstractLookup { */ @Override public String lookup(final LogEvent event, final String key) { + if (event == null) { + return null; + } final Message msg = event.getMessage(); if (msg instanceof MapMessage) { try { diff --git a/log4j-web/src/main/java/org/apache/logging/log4j/web/Log4jWebInitializerImpl.java b/log4j-web/src/main/java/org/apache/logging/log4j/web/Log4jWebInitializerImpl.java index ce36076..37bcfc3 100644 --- a/log4j-web/src/main/java/org/apache/logging/log4j/web/Log4jWebInitializerImpl.java +++ b/log4j-web/src/main/java/org/apache/logging/log4j/web/Log4jWebInitializerImpl.java @@ -36,6 +36,7 @@ import org.apache.logging.log4j.core.async.AsyncLoggerContext; import org.apache.logging.log4j.core.config.Configurator; import org.apache.logging.log4j.core.impl.ContextAnchor; import org.apache.logging.log4j.core.impl.Log4jContextFactory; +import org.apache.logging.log4j.core.lookup.ConfigurationStrSubstitutor; import org.apache.logging.log4j.core.lookup.Interpolator; import org.apache.logging.log4j.core.lookup.StrSubstitutor; import org.apache.logging.log4j.core.selector.ContextSelector; @@ -62,7 +63,7 @@ final class Log4jWebInitializerImpl extends AbstractLifeCycle implements Log4jWe } private final Map<String, String> map = new ConcurrentHashMap<>(); - private final StrSubstitutor substitutor = new StrSubstitutor(new Interpolator(map)); + private final StrSubstitutor substitutor = new ConfigurationStrSubstitutor(new Interpolator(map)); private final ServletContext servletContext; private String name; diff --git a/src/changes/changes.xml b/src/changes/changes.xml index a5fa32b..c060fee 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -30,6 +30,9 @@ - "remove" - Removed --> <release version="2.12.3" date="2021-12-dd" description="GA Release 2.12.3"> + <action issue="LOG4J2-3230" dev="ckozak" type="fix"> + Fix string substitution recursion. + </action> <action issue="LOG4J2-2819" dev="mattsicker" type="update"> Add support for specifying an SSL configuration for SmtpAppender. </action>