This is an automated email from the ASF dual-hosted git repository. ckozak pushed a commit to branch release-2.x in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git
commit 806023265f8c905b2dd1d81fd2458f64b2ea0b5e Author: Carter Kozak <[email protected]> AuthorDate: Thu Dec 16 17:11:56 2021 -0500 Fix string substitution recursion --- .../org/apache/log4j/builders/AbstractBuilder.java | 5 +- .../apache/log4j/config/Log4j1Configuration.java | 1 + .../log4j/config/Log4j1ConfigurationParser.java | 5 +- .../apache/logging/log4j/spi/AbstractLogger.java | 16 +-- .../log4j/core/config/AbstractConfiguration.java | 19 ++- .../logging/log4j/core/config/AppenderControl.java | 4 +- .../logging/log4j/core/config/Configuration.java | 9 ++ .../log4j/core/config/ConfigurationFactory.java | 3 +- .../config/composite/CompositeConfiguration.java | 2 +- .../log4j/core/config/json/JsonConfiguration.java | 2 +- .../core/config/plugins/util/PluginBuilder.java | 16 ++- .../log4j/core/config/xml/XmlConfiguration.java | 2 +- .../core/lookup/ConfigurationStrSubstitutor.java | 47 +++++++ .../log4j/core/lookup/ContextMapLookup.java | 2 +- .../logging/log4j/core/lookup/DateLookup.java | 2 +- .../logging/log4j/core/lookup/EventLookup.java | 3 + .../log4j/core/lookup/RuntimeStrSubstitutor.java | 45 +++++++ .../logging/log4j/core/lookup/StrSubstitutor.java | 143 +++++++++++++++++---- ...rnResolverDoesNotEvaluateThreadContextTest.java | 116 +++++++++++++++++ .../appender/rolling/RollingFileManagerTest.java | 2 +- .../RoutingAppenderKeyLookupEvaluationTest.java | 94 ++++++++++++++ .../log4j/core/lookup/StrSubstitutorTest.java | 133 +++++++++++++++++++ .../src/test/resources/log4j-routing-2767.xml | 6 +- ...j-routing-2767.xml => log4j-routing-lookup.xml} | 32 ++--- .../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 +++++ .../logging/log4j/web/Log4jWebInitializerImpl.java | 3 +- .../apache/logging/log4j/lookup/CustomLookup.java | 3 + .../logging/log4j/lookup/MapMessageLookup.java | 3 + .../logging/log4j/web/Log4jWebInitializerImpl.java | 3 +- src/changes/changes.xml | 7 + 35 files changed, 680 insertions(+), 109 deletions(-) diff --git a/log4j-1.2-api/src/main/java/org/apache/log4j/builders/AbstractBuilder.java b/log4j-1.2-api/src/main/java/org/apache/log4j/builders/AbstractBuilder.java index 35c44fb..9cd7851 100644 --- a/log4j-1.2-api/src/main/java/org/apache/log4j/builders/AbstractBuilder.java +++ b/log4j-1.2-api/src/main/java/org/apache/log4j/builders/AbstractBuilder.java @@ -24,6 +24,7 @@ import org.apache.logging.log4j.Level; import org.apache.logging.log4j.Logger; import org.apache.logging.log4j.core.filter.CompositeFilter; import org.apache.logging.log4j.core.filter.ThresholdFilter; +import org.apache.logging.log4j.core.lookup.ConfigurationStrSubstitutor; import org.apache.logging.log4j.core.lookup.StrSubstitutor; import org.apache.logging.log4j.status.StatusLogger; @@ -54,7 +55,7 @@ public abstract class AbstractBuilder { public AbstractBuilder() { this.prefix = null; this.props = new Properties(); - strSubstitutor = new StrSubstitutor(System.getProperties()); + strSubstitutor = new ConfigurationStrSubstitutor(System.getProperties()); } public AbstractBuilder(String prefix, Properties props) { @@ -63,7 +64,7 @@ public abstract class AbstractBuilder { Map<String, String> map = new HashMap<>(); System.getProperties().forEach((k, v) -> map.put(k.toString(), v.toString())); props.forEach((k, v) -> map.put(k.toString(), v.toString())); - strSubstitutor = new StrSubstitutor(map); + strSubstitutor = new ConfigurationStrSubstitutor(map); } public String getProperty(String key) { diff --git a/log4j-1.2-api/src/main/java/org/apache/log4j/config/Log4j1Configuration.java b/log4j-1.2-api/src/main/java/org/apache/log4j/config/Log4j1Configuration.java index 3603fbf..8be2ae8 100644 --- a/log4j-1.2-api/src/main/java/org/apache/log4j/config/Log4j1Configuration.java +++ b/log4j-1.2-api/src/main/java/org/apache/log4j/config/Log4j1Configuration.java @@ -55,6 +55,7 @@ public class Log4j1Configuration extends AbstractConfiguration implements Reconf @Override public void initialize() { getStrSubstitutor().setConfiguration(this); + getConfigurationStrSubstitutor().setConfiguration(this); super.getScheduler().start(); doConfigure(); setState(State.INITIALIZED); 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 2667f7c..8fa269a 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 b5114c8..1990a73 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 @@ -2104,7 +2104,7 @@ public abstract class AbstractLogger implements ExtendedLogger, LocationAwareLog try { incrementRecursionDepth(); log(level, marker, fqcn, location, message, throwable); - } catch (Exception ex) { + } catch (Throwable ex) { handleLogMessageException(ex, fqcn, message); } finally { decrementRecursionDepth(); @@ -2203,9 +2203,9 @@ public abstract class AbstractLogger implements ExtendedLogger, LocationAwareLog final Throwable throwable) { try { log(level, marker, fqcn, location, message, 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, message); + handleLogMessageException(t, fqcn, message); } } @@ -2218,16 +2218,16 @@ 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 message) { - if (exception instanceof LoggingException) { - throw (LoggingException) exception; + private void handleLogMessageException(final Throwable throwable, final String fqcn, final Message message) { + if (throwable instanceof LoggingException) { + throw (LoggingException) throwable; } StatusLogger.getLogger().warn("{} caught {} logging {}: {}", fqcn, - exception.getClass().getName(), + throwable.getClass().getName(), message.getClass().getSimpleName(), message.getFormat(), - exception); + throwable); } @Override 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 cb3ab40..d5f6b5e 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 @@ -56,8 +56,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; @@ -129,7 +131,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; @@ -217,6 +220,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) { @@ -623,12 +627,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; @@ -805,6 +813,11 @@ public abstract class AbstractConfiguration extends AbstractFilterable implement } @Override + 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 dc9d990..0c73a1f 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 @@ -161,8 +161,8 @@ public class AppenderControl extends AbstractFilterable { appender.append(event); } catch (final RuntimeException error) { handleAppenderError(event, error); - } catch (final Exception error) { - handleAppenderError(event, new AppenderLoggingException(error)); + } catch (final Throwable throwable) { + handleAppenderError(event, new AppenderLoggingException(throwable)); } } diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/Configuration.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/Configuration.java index b607571..eac94c5 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/Configuration.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/Configuration.java @@ -27,6 +27,7 @@ import org.apache.logging.log4j.core.Logger; import org.apache.logging.log4j.core.LoggerContext; import org.apache.logging.log4j.core.async.AsyncLoggerConfigDelegate; import org.apache.logging.log4j.core.filter.Filterable; +import org.apache.logging.log4j.core.lookup.ConfigurationStrSubstitutor; import org.apache.logging.log4j.core.lookup.StrSubstitutor; import org.apache.logging.log4j.core.net.Advertiser; import org.apache.logging.log4j.core.script.ScriptManager; @@ -116,6 +117,14 @@ public interface Configuration extends Filterable { StrSubstitutor getStrSubstitutor(); + default StrSubstitutor getConfigurationStrSubstitutor() { + StrSubstitutor defaultSubstitutor = getStrSubstitutor(); + if (defaultSubstitutor == null) { + return new ConfigurationStrSubstitutor(); + } + return new ConfigurationStrSubstitutor(defaultSubstitutor); + } + void createConfiguration(Node node, LogEvent event); <T> T getComponent(String name); 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 9a9c8a2..ea905dd 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 @@ -23,6 +23,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; @@ -141,7 +142,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 2be2a9f..c99d5dc 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..69623d3 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 @@ -124,20 +124,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 +180,9 @@ public class PluginBuilder implements Builder<Object> { final Object value = visitor.setAliases(aliases) .setAnnotation(a) .setConversionType(field.getType()) - .setStrSubstitutor(configuration.getStrSubstitutor()) + .setStrSubstitutor(event == null + ? configuration.getConfigurationStrSubstitutor() + : 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 +255,9 @@ public class PluginBuilder implements Builder<Object> { final Object value = visitor.setAliases(aliases) .setAnnotation(a) .setConversionType(types[i]) - .setStrSubstitutor(configuration.getStrSubstitutor()) + .setStrSubstitutor(event == null + ? configuration.getConfigurationStrSubstitutor() + : 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..eb191e7 --- /dev/null +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/ConfigurationStrSubstitutor.java @@ -0,0 +1,47 @@ +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..a8be6e2 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().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/EventLookup.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/EventLookup.java index e5128f1..b4587fc 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/EventLookup.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/EventLookup.java @@ -33,6 +33,9 @@ public class EventLookup extends AbstractLookup { */ @Override public String lookup(final LogEvent event, final String key) { + if (event == null) { + return null; + } switch (key) { case "Marker": { return event.getMarker() != null ? event.getMarker().getName() : null; 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..eb938ce --- /dev/null +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/RuntimeStrSubstitutor.java @@ -0,0 +1,45 @@ +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 8faf6f2..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; /** @@ -207,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 @@ -369,8 +373,8 @@ public class StrSubstitutor implements ConfigurationAware { * @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) { + final StrMatcher suffixMatcher, final char escape, final StrMatcher valueDelimiterMatcher, + final StrMatcher valueEscapeMatcher) { this.setVariableResolver(variableResolver); this.setVariablePrefixMatcher(prefixMatcher); this.setVariableSuffixMatcher(suffixMatcher); @@ -379,6 +383,20 @@ public class StrSubstitutor implements ConfigurationAware { 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 @@ -439,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 @@ -464,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(); } @@ -506,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(); } @@ -540,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(); } @@ -582,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(); } @@ -614,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(); } @@ -656,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(); } @@ -688,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(); } /** @@ -729,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(); } @@ -759,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(); } @@ -818,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()); @@ -974,8 +1039,12 @@ public class StrSubstitutor implements ConfigurationAware { 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<>(); + } final StringBuilder bufName = new StringBuilder(varNameExpr); - substitute(event, bufName, 0, bufName.length()); + substitute(event, bufName, 0, bufName.length(), priorVariables); varNameExpr = bufName.toString(); } pos += endMatchLen; @@ -1026,11 +1095,10 @@ public class StrSubstitutor implements ConfigurationAware { } // handle cyclic substitution - checkCyclicSubstitution(varName, priorVariables); - priorVariables.add(varName); + boolean isCyclic = isCyclicSubstitution(varName, priorVariables); // resolve the variable - String varValue = resolveVariable(event, varName, buf, startPos, endPos); + String varValue = isCyclic ? null : resolveVariable(event, varName, buf, startPos, endPos); if (varValue == null) { varValue = varDefaultValue; } @@ -1039,7 +1107,9 @@ public class StrSubstitutor implements ConfigurationAware { final int varLen = varValue.length(); buf.replace(startPos, endPos, varValue); altered = true; - int change = substitute(event, buf, startPos, varLen, priorVariables); + int change = isRecursiveEvaluationAllowed() + ? substitute(event, buf, startPos, varLen, priorVariables) + : 0; change = change + (varLen - (endPos - startPos)); pos += change; bufEnd += change; @@ -1048,7 +1118,9 @@ public class StrSubstitutor implements ConfigurationAware { } // remove variable from the cyclic stack - priorVariables.remove(priorVariables.size() - 1); + if (!isCyclic) { + priorVariables.remove(priorVariables.size() - 1); + } break; } nestedVarCount--; @@ -1064,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; } /** @@ -1107,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 @@ -1396,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..173101c --- /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.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.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(messages != null && messages.size() > 0, "No messages returned"); + 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(messages != null && messages.size() > 0, "No messages returned"); + 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(messages != null && messages.size() > 0, "No messages returned"); + 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(messages != null && messages.size() > 0, "No messages returned"); + 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(messages != null && messages.size() > 0, "No messages returned"); + 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/rolling/RollingFileManagerTest.java b/log4j-core/src/test/java/org/apache/logging/log4j/core/appender/rolling/RollingFileManagerTest.java index d134525..9c0159d 100644 --- a/log4j-core/src/test/java/org/apache/logging/log4j/core/appender/rolling/RollingFileManagerTest.java +++ b/log4j-core/src/test/java/org/apache/logging/log4j/core/appender/rolling/RollingFileManagerTest.java @@ -69,7 +69,7 @@ public class RollingFileManagerTest { .withFilePattern("FilePattern") .setName("RollingFileAppender") .setConfiguration(config) - .withStrategy(new CustomDirectFileRolloverStrategy(file, config.getStrSubstitutor())) + .withStrategy(new CustomDirectFileRolloverStrategy(file, config.getConfigurationStrSubstitutor())) .withPolicy(new SizeBasedTriggeringPolicy(100)) .build(); 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..8a31d18 --- /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.jupiter.api.Assertions.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 4edbd4e..375ff30 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,6 +20,7 @@ import java.util.HashMap; import java.util.Map; import org.apache.logging.log4j.ThreadContext; +import org.apache.logging.log4j.core.LogEvent; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; @@ -74,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<>(); + 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<>())); + 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<>())); + 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<>(); + 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<>(); + 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<>(); + 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<>(); + 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<>(); + 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<>(); + 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<>(); + 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<>(); + 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-2767.xml b/log4j-core/src/test/resources/log4j-routing-2767.xml index 34b9c30..4002659 100644 --- a/log4j-core/src/test/resources/log4j-routing-2767.xml +++ b/log4j-core/src/test/resources/log4j-routing-2767.xml @@ -17,10 +17,6 @@ --> <Configuration status="WARN" name="RoutingTest"> - <Properties> - <Property name="filename">target/routing1/routingtest-$${sd:type}.log</Property> - </Properties> - <Appenders> <Console name="STDOUT"> <PatternLayout pattern="%m%n"/> @@ -28,7 +24,7 @@ <Routing name="Routing"> <Routes> <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-routing-2767.xml b/log4j-core/src/test/resources/log4j-routing-lookup.xml similarity index 55% copy from log4j-core/src/test/resources/log4j-routing-2767.xml copy to log4j-core/src/test/resources/log4j-routing-lookup.xml index 34b9c30..92f31c3 100644 --- a/log4j-core/src/test/resources/log4j-routing-2767.xml +++ b/log4j-core/src/test/resources/log4j-routing-lookup.xml @@ -16,36 +16,22 @@ limitations under the License. --> -<Configuration status="WARN" name="RoutingTest"> - <Properties> - <Property name="filename">target/routing1/routingtest-$${sd:type}.log</Property> - </Properties> - +<Configuration status="OFF" name="RoutingAppenderKeyLookupEvaluationTest"> <Appenders> - <Console name="STDOUT"> - <PatternLayout pattern="%m%n"/> - </Console> + <List name="List"> + <PatternLayout pattern="%p $${ctx:user} $${event:Message}"/> + </List> <Routing name="Routing"> - <Routes> - <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> + <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"> - <AppenderRef ref="Routing"/> - </Logger> - - <Root level="info"> + <Root level="debug"> <AppenderRef ref="Routing"/> </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-jakarta-web/src/main/java/org/apache/logging/log4j/web/Log4jWebInitializerImpl.java b/log4j-jakarta-web/src/main/java/org/apache/logging/log4j/web/Log4jWebInitializerImpl.java index bb5d51d..b56270b 100644 --- a/log4j-jakarta-web/src/main/java/org/apache/logging/log4j/web/Log4jWebInitializerImpl.java +++ b/log4j-jakarta-web/src/main/java/org/apache/logging/log4j/web/Log4jWebInitializerImpl.java @@ -24,6 +24,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; @@ -57,7 +58,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/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 7cd7885..f41ce41 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 @@ -51,6 +51,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 323ccbb..32b08c4 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 @@ -23,6 +23,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; @@ -57,7 +58,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 08e3fd5..2f9473b 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -30,10 +30,17 @@ - "remove" - Removed --> <release version="2.17.0" date="2021-MM-dd" description="GA Release 2.17.0"> + <action issue="LOG4J2-3230" dev="ckozak" type="fix"> + Fix string substitution recursion. + </action> <action issue="LOG4J2-3242" dev="rgoers, ggregory" type="fix"> Limit JNDI to the java protocol only. JNDI will remain disabled by default. Rename JNDI enablement property from 'log4j2.enableJndi' to 'log4j2.enableJndiLookup', 'log4j2.enableJndiJms', and 'log4j2.enableJndiContextSelector'. </action> + <action issue="LOG4J2-3242" dev="rgoers" type="fix"> + Limit JNDI to the java protocol only. JNDI will remain disabled by default. The enablement + property has been renamed to 'log4j2.enableJndiJava' + </action> <action issue="LOG4J2-3241" dev="rgoers" type="fix"> Do not declare log4j-api-java9 and log4j-core-java9 as dependencies as it causes problems with the Maven enforcer plugin.
