This is an automated email from the ASF dual-hosted git repository. pkarwasz pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git
commit dcd648e0b8afd579ed91627add456b2cfdeb278e Author: Piotr P. Karwasz <[email protected]> AuthorDate: Mon Feb 21 08:07:30 2022 +0100 Improves behavior of failing Log4j 1.x builders If a Builder fails due to a missing property, it return `null`. This activates the fallback _"instantiate by class name"_ mechanism that will fail with `NoClassDefFoundException` in the best scenario or will instantiate a native Log4j 1.x component in the worst scenario. A better approach, IMHO, is to an empty wrapper, which deactivates the fallback mechanism, but will be unwrapped to `null` soon after and removed from the configuration. This PR also increases the level of status logger messages to `ERROR` if the condition prevents the instantiation of the component. Conflicts: log4j-1.2-api/src/main/java/org/apache/log4j/builders/BuilderManager.java --- .../org/apache/log4j/builders/BuilderManager.java | 48 ++++++++++++++----- .../builders/appender/AsyncAppenderBuilder.java | 8 +++- .../appender/DailyRollingFileAppenderBuilder.java | 2 +- .../builders/appender/FileAppenderBuilder.java | 2 +- .../builders/appender/RewriteAppenderBuilder.java | 12 +++-- .../appender/RollingFileAppenderBuilder.java | 2 +- .../builders/filter/StringMatchFilterBuilder.java | 11 ++++- .../log4j/config/PropertiesConfiguration.java | 5 +- .../apache/log4j/builders/BuilderManagerTest.java | 54 ++++++++++++++++++++++ 9 files changed, 122 insertions(+), 22 deletions(-) diff --git a/log4j-1.2-api/src/main/java/org/apache/log4j/builders/BuilderManager.java b/log4j-1.2-api/src/main/java/org/apache/log4j/builders/BuilderManager.java index b524dfe..249ca94 100644 --- a/log4j-1.2-api/src/main/java/org/apache/log4j/builders/BuilderManager.java +++ b/log4j-1.2-api/src/main/java/org/apache/log4j/builders/BuilderManager.java @@ -18,6 +18,10 @@ package org.apache.log4j.builders; import org.apache.log4j.Appender; import org.apache.log4j.Layout; +import org.apache.log4j.bridge.AppenderWrapper; +import org.apache.log4j.bridge.FilterWrapper; +import org.apache.log4j.bridge.LayoutWrapper; +import org.apache.log4j.bridge.RewritePolicyWrapper; import org.apache.log4j.builders.appender.AppenderBuilder; import org.apache.log4j.builders.filter.FilterBuilder; import org.apache.log4j.builders.layout.LayoutBuilder; @@ -48,6 +52,10 @@ public class BuilderManager { /** Plugin category. */ public static final String CATEGORY = "Log4j Builder"; + public static final Appender INVALID_APPENDER = new AppenderWrapper(null); + public static final Filter INVALID_FILTER = new FilterWrapper(null); + public static final Layout INVALID_LAYOUT = new LayoutWrapper(null); + public static final RewritePolicy INVALID_REWRITE_POLICY = new RewritePolicyWrapper(null); public static final Key<PluginManager> PLUGIN_MANAGER_KEY = new @Named(CATEGORY) Key<>() {}; private static final Logger LOGGER = StatusLogger.getLogger(); private static final Class<?>[] CONSTRUCTOR_PARAMS = new Class[] { String.class, Properties.class }; @@ -96,41 +104,59 @@ public class BuilderManager { return (PluginType<T>) pluginType; } - private <T extends Builder<U>, U> U newInstance(final PluginType<T> plugin, final Function<T, U> consumer) { + private <T extends Builder<U>, U> U newInstance(final PluginType<T> plugin, final Function<T, U> consumer, + final U invalidValue) { if (plugin != null) { final T builder = injector.getInstance(plugin.getPluginClass()); if (builder != null) { - return consumer.apply(builder); + final U result = consumer.apply(builder); + // returning an empty wrapper is short for "we support this legacy class, but it has validation errors" + return result != null ? result : invalidValue; } } return null; } - public <P extends Parser<T>, T> T parse(final String className, final String prefix, final Properties props, final PropertiesConfiguration config) { + public <P extends Parser<T>, T> T parse(final String className, final String prefix, final Properties props, + final PropertiesConfiguration config, T invalidValue) { final P parser = createBuilder(getPlugin(className), prefix, props); - return parser != null ? parser.parse(config) : null; + if (parser != null) { + final T value = parser.parse(config); + return value != null ? value : invalidValue; + } + return null; } - public Appender parseAppender(final String className, final Element appenderElement, final XmlConfiguration config) { - return newInstance(this.<AppenderBuilder<Appender>>getPlugin(className), b -> b.parseAppender(appenderElement, config)); + public Appender parseAppender(final String className, final Element appenderElement, + final XmlConfiguration config) { + return newInstance(this.<AppenderBuilder<Appender>>getPlugin(className), + b -> b.parseAppender(appenderElement, config), INVALID_APPENDER); } public Appender parseAppender(final String name, final String className, final String prefix, final String layoutPrefix, final String filterPrefix, final Properties props, final PropertiesConfiguration config) { final AppenderBuilder<Appender> builder = createBuilder(getPlugin(className), prefix, props); - return builder != null ? builder.parseAppender(name, prefix, layoutPrefix, filterPrefix, props, config) : null; + if (builder != null) { + final Appender appender = builder.parseAppender(name, prefix, layoutPrefix, filterPrefix, props, config); + return appender != null ? appender : INVALID_APPENDER; + } + return null; } public Filter parseFilter(final String className, final Element filterElement, final XmlConfiguration config) { - return newInstance(this.<FilterBuilder>getPlugin(className), b -> b.parse(filterElement, config)); + return newInstance(this.<FilterBuilder>getPlugin(className), b -> b.parse(filterElement, config), + INVALID_FILTER); } public Layout parseLayout(final String className, final Element layoutElement, final XmlConfiguration config) { - return newInstance(this.<LayoutBuilder>getPlugin(className), b -> b.parse(layoutElement, config)); + return newInstance(this.<LayoutBuilder>getPlugin(className), b -> b.parse(layoutElement, config), + INVALID_LAYOUT); } - public RewritePolicy parseRewritePolicy(final String className, final Element rewriteElement, final XmlConfiguration config) { - return newInstance(this.<RewritePolicyBuilder>getPlugin(className), b -> b.parse(rewriteElement, config)); + public RewritePolicy parseRewritePolicy(final String className, final Element rewriteElement, + final XmlConfiguration config) { + return newInstance(this.<RewritePolicyBuilder>getPlugin(className), b -> b.parse(rewriteElement, config), + INVALID_REWRITE_POLICY); } } diff --git a/log4j-1.2-api/src/main/java/org/apache/log4j/builders/appender/AsyncAppenderBuilder.java b/log4j-1.2-api/src/main/java/org/apache/log4j/builders/appender/AsyncAppenderBuilder.java index 1801dfa..5aece66 100644 --- a/log4j-1.2-api/src/main/java/org/apache/log4j/builders/appender/AsyncAppenderBuilder.java +++ b/log4j-1.2-api/src/main/java/org/apache/log4j/builders/appender/AsyncAppenderBuilder.java @@ -118,12 +118,12 @@ public class AsyncAppenderBuilder extends AbstractBuilder implements AppenderBui final String level = getProperty(THRESHOLD_PARAM); final int bufferSize = getIntegerProperty(BUFFER_SIZE_PARAM, 1024); if (appenderRef == null) { - LOGGER.warn("No appender references configured for AsyncAppender {}", name); + LOGGER.error("No appender references configured for AsyncAppender {}", name); return null; } final Appender appender = configuration.parseAppender(props, appenderRef); if (appender == null) { - LOGGER.warn("Cannot locate Appender {}", appenderRef); + LOGGER.error("Cannot locate Appender {}", appenderRef); return null; } return createAppender(name, level, new String[]{appenderRef}, blocking, bufferSize, includeLocation, filter, @@ -133,6 +133,10 @@ public class AsyncAppenderBuilder extends AbstractBuilder implements AppenderBui private <T extends Log4j1Configuration> Appender createAppender(final String name, final String level, final String[] appenderRefs, final boolean blocking, final int bufferSize, final boolean includeLocation, final Filter filter, final T configuration) { + if (appenderRefs.length == 0) { + LOGGER.error("No appender references configured for AsyncAppender {}", name); + return null; + } final org.apache.logging.log4j.Level logLevel = OptionConverter.convertLevel(level, org.apache.logging.log4j.Level.TRACE); final AppenderRef[] refs = new AppenderRef[appenderRefs.length]; diff --git a/log4j-1.2-api/src/main/java/org/apache/log4j/builders/appender/DailyRollingFileAppenderBuilder.java b/log4j-1.2-api/src/main/java/org/apache/log4j/builders/appender/DailyRollingFileAppenderBuilder.java index a0dacac..c0704a1 100644 --- a/log4j-1.2-api/src/main/java/org/apache/log4j/builders/appender/DailyRollingFileAppenderBuilder.java +++ b/log4j-1.2-api/src/main/java/org/apache/log4j/builders/appender/DailyRollingFileAppenderBuilder.java @@ -152,7 +152,7 @@ public class DailyRollingFileAppenderBuilder extends AbstractBuilder implements } final org.apache.logging.log4j.core.Filter fileFilter = buildFilters(level, filter); if (fileName == null) { - LOGGER.warn("Unable to create File Appender, no file name provided"); + LOGGER.error("Unable to create DailyRollingFileAppender, no file name provided"); return null; } final String filePattern = fileName + "%d{" + datePattern + "}"; diff --git a/log4j-1.2-api/src/main/java/org/apache/log4j/builders/appender/FileAppenderBuilder.java b/log4j-1.2-api/src/main/java/org/apache/log4j/builders/appender/FileAppenderBuilder.java index 0327218..65bd489 100644 --- a/log4j-1.2-api/src/main/java/org/apache/log4j/builders/appender/FileAppenderBuilder.java +++ b/log4j-1.2-api/src/main/java/org/apache/log4j/builders/appender/FileAppenderBuilder.java @@ -136,7 +136,7 @@ public class FileAppenderBuilder extends AbstractBuilder implements AppenderBuil } final org.apache.logging.log4j.core.Filter fileFilter = buildFilters(level, filter); if (fileName == null) { - LOGGER.warn("Unable to create File Appender, no file name provided"); + LOGGER.error("Unable to create FileAppender, no file name provided"); return null; } return new AppenderWrapper(FileAppender.newBuilder() diff --git a/log4j-1.2-api/src/main/java/org/apache/log4j/builders/appender/RewriteAppenderBuilder.java b/log4j-1.2-api/src/main/java/org/apache/log4j/builders/appender/RewriteAppenderBuilder.java index d8b61ba..9e1b3b5 100644 --- a/log4j-1.2-api/src/main/java/org/apache/log4j/builders/appender/RewriteAppenderBuilder.java +++ b/log4j-1.2-api/src/main/java/org/apache/log4j/builders/appender/RewriteAppenderBuilder.java @@ -33,6 +33,7 @@ import org.apache.log4j.bridge.AppenderWrapper; import org.apache.log4j.bridge.RewritePolicyAdapter; import org.apache.log4j.bridge.RewritePolicyWrapper; import org.apache.log4j.builders.AbstractBuilder; +import org.apache.log4j.builders.BuilderManager; import org.apache.log4j.config.Log4j1Configuration; import org.apache.log4j.config.PropertiesConfiguration; import org.apache.log4j.helpers.OptionConverter; @@ -105,15 +106,16 @@ public class RewriteAppenderBuilder extends AbstractBuilder implements AppenderB final Filter filter = configuration.parseAppenderFilters(props, filterPrefix, name); final String policyPrefix = appenderPrefix + ".rewritePolicy"; final String className = getProperty(policyPrefix); - final RewritePolicy policy = configuration.getBuilderManager().parse(className, policyPrefix, props, configuration); + final RewritePolicy policy = configuration.getBuilderManager() + .parse(className, policyPrefix, props, configuration, BuilderManager.INVALID_REWRITE_POLICY); final String level = getProperty(THRESHOLD_PARAM); if (appenderRef == null) { - LOGGER.warn("No appender references configured for AsyncAppender {}", name); + LOGGER.error("No appender references configured for RewriteAppender {}", name); return null; } final Appender appender = configuration.parseAppender(props, appenderRef); if (appender == null) { - LOGGER.warn("Cannot locate Appender {}", appenderRef); + LOGGER.error("Cannot locate Appender {}", appenderRef); return null; } return createAppender(name, level, new String[] {appenderRef}, policy, filter, configuration); @@ -121,6 +123,10 @@ public class RewriteAppenderBuilder extends AbstractBuilder implements AppenderB private <T extends Log4j1Configuration> Appender createAppender(final String name, final String level, final String[] appenderRefs, final RewritePolicy policy, final Filter filter, final T configuration) { + if (appenderRefs.length == 0) { + LOGGER.error("No appender references configured for RewriteAppender {}", name); + return null; + } final org.apache.logging.log4j.Level logLevel = OptionConverter.convertLevel(level, org.apache.logging.log4j.Level.TRACE); final AppenderRef[] refs = new AppenderRef[appenderRefs.length]; diff --git a/log4j-1.2-api/src/main/java/org/apache/log4j/builders/appender/RollingFileAppenderBuilder.java b/log4j-1.2-api/src/main/java/org/apache/log4j/builders/appender/RollingFileAppenderBuilder.java index 9664756..4cd8bb4 100644 --- a/log4j-1.2-api/src/main/java/org/apache/log4j/builders/appender/RollingFileAppenderBuilder.java +++ b/log4j-1.2-api/src/main/java/org/apache/log4j/builders/appender/RollingFileAppenderBuilder.java @@ -157,7 +157,7 @@ public class RollingFileAppenderBuilder extends AbstractBuilder implements Appen } final org.apache.logging.log4j.core.Filter fileFilter = buildFilters(level, filter); if (fileName == null) { - LOGGER.warn("Unable to create File Appender, no file name provided"); + LOGGER.error("Unable to create RollingFileAppender, no file name provided"); return null; } final String filePattern = fileName + ".%i"; diff --git a/log4j-1.2-api/src/main/java/org/apache/log4j/builders/filter/StringMatchFilterBuilder.java b/log4j-1.2-api/src/main/java/org/apache/log4j/builders/filter/StringMatchFilterBuilder.java index 616e9f8..cce3344 100644 --- a/log4j-1.2-api/src/main/java/org/apache/log4j/builders/filter/StringMatchFilterBuilder.java +++ b/log4j-1.2-api/src/main/java/org/apache/log4j/builders/filter/StringMatchFilterBuilder.java @@ -19,6 +19,7 @@ package org.apache.log4j.builders.filter; import static org.apache.log4j.builders.BuilderManager.CATEGORY; import static org.apache.log4j.xml.XmlConfiguration.forEachElement; +import java.util.Properties; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; @@ -43,6 +44,14 @@ public class StringMatchFilterBuilder extends AbstractBuilder<Filter> implements private static final String STRING_TO_MATCH = "StringToMatch"; private static final String ACCEPT_ON_MATCH = "AcceptOnMatch"; + public StringMatchFilterBuilder() { + super(); + } + + public StringMatchFilterBuilder(String prefix, Properties props) { + super(prefix, props); + } + @Override public Filter parse(Element filterElement, XmlConfiguration config) { final AtomicBoolean acceptOnMatch = new AtomicBoolean(); @@ -72,7 +81,7 @@ public class StringMatchFilterBuilder extends AbstractBuilder<Filter> implements private Filter createFilter(String text, boolean acceptOnMatch) { if (text == null) { - LOGGER.warn("No text provided for StringMatchFilter"); + LOGGER.error("No text provided for StringMatchFilter"); return null; } org.apache.logging.log4j.core.Filter.Result onMatch = acceptOnMatch diff --git a/log4j-1.2-api/src/main/java/org/apache/log4j/config/PropertiesConfiguration.java b/log4j-1.2-api/src/main/java/org/apache/log4j/config/PropertiesConfiguration.java index 5eb9686..f5774f1 100644 --- a/log4j-1.2-api/src/main/java/org/apache/log4j/config/PropertiesConfiguration.java +++ b/log4j-1.2-api/src/main/java/org/apache/log4j/config/PropertiesConfiguration.java @@ -36,6 +36,7 @@ import org.apache.log4j.PatternLayout; import org.apache.log4j.bridge.AppenderAdapter; import org.apache.log4j.bridge.AppenderWrapper; import org.apache.log4j.bridge.FilterAdapter; +import org.apache.log4j.builders.BuilderManager; import org.apache.log4j.helpers.OptionConverter; import org.apache.log4j.spi.ErrorHandler; import org.apache.log4j.spi.Filter; @@ -498,7 +499,7 @@ public class PropertiesConfiguration extends Log4j1Configuration { if (layoutClass == null) { return null; } - Layout layout = manager.parse(layoutClass, layoutPrefix, props, this); + Layout layout = manager.parse(layoutClass, layoutPrefix, props, this, BuilderManager.INVALID_LAYOUT); if (layout == null) { layout = buildLayout(layoutPrefix, layoutClass, appenderName, props); } @@ -574,7 +575,7 @@ public class PropertiesConfiguration extends Log4j1Configuration { final String clazz = props.getProperty(entry.getKey()); Filter filter = null; if (clazz != null) { - filter = manager.parse(clazz, entry.getKey(), props, this); + filter = manager.parse(clazz, entry.getKey(), props, this, BuilderManager.INVALID_FILTER); if (filter == null) { LOGGER.debug("Filter key: [{}] class: [{}] props: {}", entry.getKey(), clazz, entry.getValue()); filter = buildFilter(clazz, appenderName, entry.getValue()); diff --git a/log4j-1.2-api/src/test/java/org/apache/log4j/builders/BuilderManagerTest.java b/log4j-1.2-api/src/test/java/org/apache/log4j/builders/BuilderManagerTest.java new file mode 100644 index 0000000..7203b19 --- /dev/null +++ b/log4j-1.2-api/src/test/java/org/apache/log4j/builders/BuilderManagerTest.java @@ -0,0 +1,54 @@ +/* + * 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.log4j.builders; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import java.util.Properties; + +import org.apache.log4j.Appender; +import org.apache.log4j.FileAppender; +import org.apache.log4j.config.PropertiesConfiguration; +import org.apache.log4j.spi.Filter; +import org.apache.log4j.varia.StringMatchFilter; +import org.apache.logging.log4j.plugins.di.DI; +import org.junit.jupiter.api.Test; + +public class BuilderManagerTest { + + /** + * This test ensures that instantiation failures due to missing parameters + * always return an empty wrapper instead of null, hence disabling the + * <i>"instantiate by classname"</i> fallback mechanism for supported components. + */ + @Test + public void testReturnInvalidValueOnError() { + final PropertiesConfiguration config = new PropertiesConfiguration(null, null); + final BuilderManager manager = new BuilderManager(DI.createInjector()); + final Properties props = new Properties(); + props.setProperty("FILE", FileAppender.class.getName()); + props.setProperty("FILE.filter.1", StringMatchFilter.class.getName()); + // Parse an invalid StringMatchFilter + final Filter filter = manager.parse(StringMatchFilter.class.getName(), "FILE.filter", props, config, + BuilderManager.INVALID_FILTER); + assertEquals(BuilderManager.INVALID_FILTER, filter); + // Parse an invalid FileAppender + final Appender appender = manager.parseAppender("FILE", FileAppender.class.getName(), "FILE", "FILE.layout", + "FILE.filter.", props, config); + assertEquals(BuilderManager.INVALID_APPENDER, appender); + } +}
