diff --git a/log4j-1.2-api/src/main/java/org/apache/log4j/jmx/AppenderDynamicMBean.java b/log4j-1.2-api/src/main/java/org/apache/log4j/jmx/AppenderDynamicMBean.java index 2ee683240ad..053388fb18b 100644 --- a/log4j-1.2-api/src/main/java/org/apache/log4j/jmx/AppenderDynamicMBean.java +++ b/log4j-1.2-api/src/main/java/org/apache/log4j/jmx/AppenderDynamicMBean.java @@ -187,6 +187,14 @@ public Object invoke(final String operationName, final Object params[], final St } else if (operationName.equals("setLayout")) { final Layout layout = (Layout) OptionConverter.instantiateByClassName((String) params[0], Layout.class, null); + if (layout == null) { + final String message = "Could not instantiate layout class [" + params[0] + "] for appender [" + + getAppenderName(appender) + "]."; + cat.error(message); + // Fail via MBeanException so JMX clients can distinguish failure from + // success (setLayout is declared void; a return string is not reliable). + throw new MBeanException(new IllegalArgumentException(message), message); + } appender.setLayout(layout); registerLayoutMBean(layout); } diff --git a/log4j-1.2-api/src/main/java/org/apache/log4j/jmx/LoggerDynamicMBean.java b/log4j-1.2-api/src/main/java/org/apache/log4j/jmx/LoggerDynamicMBean.java index 7be0f070f5f..5d6825dd2d5 100644 --- a/log4j-1.2-api/src/main/java/org/apache/log4j/jmx/LoggerDynamicMBean.java +++ b/log4j-1.2-api/src/main/java/org/apache/log4j/jmx/LoggerDynamicMBean.java @@ -63,10 +63,16 @@ public LoggerDynamicMBean(final Logger logger) { buildDynamicMBeanInfo(); } - void addAppender(final String appenderClass, final String appenderName) { + void addAppender(final String appenderClass, final String appenderName) throws MBeanException { cat.debug("addAppender called with " + appenderClass + ", " + appenderName); final Appender appender = (Appender) OptionConverter.instantiateByClassName(appenderClass, org.apache.log4j.Appender.class, null); + if (appender == null) { + final String message = + "Could not instantiate appender class [" + appenderClass + "] for name [" + appenderName + "]."; + cat.error(message); + throw new MBeanException(new IllegalArgumentException(message), message); + } appender.setName(appenderName); logger.addAppender(appender); diff --git a/log4j-1.2-api/src/test/java/org/apache/log4j/jmx/AppenderDynamicMBeanTest.java b/log4j-1.2-api/src/test/java/org/apache/log4j/jmx/AppenderDynamicMBeanTest.java new file mode 100644 index 00000000000..597bd3822a9 --- /dev/null +++ b/log4j-1.2-api/src/test/java/org/apache/log4j/jmx/AppenderDynamicMBeanTest.java @@ -0,0 +1,83 @@ +/* + * 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.jmx; + +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import javax.management.MBeanException; +import javax.management.MBeanServer; +import javax.management.MBeanServerFactory; +import javax.management.ObjectName; +import org.apache.log4j.ConsoleAppender; +import org.apache.log4j.PatternLayout; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +/** + * Regression for JMX {@code setLayout}: instantiateByClassName may return null. + */ +class AppenderDynamicMBeanTest { + + private static final String[] SET_LAYOUT_SIGNATURE = {String.class.getName()}; + + private MBeanServer server; + + @BeforeEach + void createMBeanServer() { + server = MBeanServerFactory.newMBeanServer(); + } + + /** + * Registration is required: {@code preRegister} injects the server that + * {@code registerLayoutMBean} dereferences on the success path. + */ + private AppenderDynamicMBean registerAppenderMBean(final ConsoleAppender appender) throws Exception { + final AppenderDynamicMBean mbean = new AppenderDynamicMBean(appender); + server.registerMBean(mbean, new ObjectName("log4j:appender=" + appender.getName())); + return mbean; + } + + @Test + void setLayoutFailsWhenClassCannotBeInstantiated() throws Exception { + final ConsoleAppender appender = new ConsoleAppender(); + appender.setName("jmx-layout-test"); + final AppenderDynamicMBean mbean = registerAppenderMBean(appender); + + final MBeanException thrown = assertThrows( + MBeanException.class, + () -> mbean.invoke( + "setLayout", new Object[] {"this.class.does.not.exist.MissingLayout"}, SET_LAYOUT_SIGNATURE)); + assertTrue(thrown.getMessage().contains("Could not instantiate layout class")); + assertInstanceOf(IllegalArgumentException.class, thrown.getTargetException()); + assertNull(appender.getLayout()); + } + + @Test + void setLayoutStillAttachesValidLayout() throws Exception { + final ConsoleAppender appender = new ConsoleAppender(); + appender.setName("jmx-layout-valid"); + final AppenderDynamicMBean mbean = registerAppenderMBean(appender); + + mbean.invoke("setLayout", new Object[] {PatternLayout.class.getName()}, SET_LAYOUT_SIGNATURE); + assertInstanceOf(PatternLayout.class, appender.getLayout()); + assertTrue(server.isRegistered( + new ObjectName("log4j:appender=" + appender.getName() + ",layout=" + PatternLayout.class.getName()))); + } +} diff --git a/log4j-1.2-api/src/test/java/org/apache/log4j/jmx/LoggerDynamicMBeanTest.java b/log4j-1.2-api/src/test/java/org/apache/log4j/jmx/LoggerDynamicMBeanTest.java new file mode 100644 index 00000000000..8bd45969d55 --- /dev/null +++ b/log4j-1.2-api/src/test/java/org/apache/log4j/jmx/LoggerDynamicMBeanTest.java @@ -0,0 +1,102 @@ +/* + * 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.jmx; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.Enumeration; +import javax.management.MBeanException; +import javax.management.MBeanServer; +import javax.management.MBeanServerFactory; +import javax.management.ObjectName; +import org.apache.log4j.Appender; +import org.apache.log4j.ConsoleAppender; +import org.apache.log4j.Logger; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +/** + * Regression for invalid JMX {@code addAppender} class names: instantiation can + * return null, which must not NPE when calling {@code setName} and must not be + * reported to the client as a successful invocation. + */ +class LoggerDynamicMBeanTest { + + private static final String[] ADD_APPENDER_SIGNATURE = {String.class.getName(), String.class.getName()}; + + private MBeanServer server; + + @BeforeEach + void createMBeanServer() { + server = MBeanServerFactory.newMBeanServer(); + } + + /** + * Register through an {@link MBeanServer} so the bean goes through + * {@code preRegister}, matching real JMX use (and {@code AppenderDynamicMBeanTest}). + */ + private LoggerDynamicMBean registerLoggerMBean(final Logger logger) throws Exception { + final LoggerDynamicMBean mbean = new LoggerDynamicMBean(logger); + final String name = logger.getName().isEmpty() ? "root" : logger.getName(); + // Same ObjectName shape as HierarchyDynamicMBean.addLoggerMBean. + server.registerMBean(mbean, new ObjectName("log4j", "logger", name)); + return mbean; + } + + @Test + void addAppenderFailsWhenClassCannotBeInstantiated() throws Exception { + final Logger logger = Logger.getLogger("jmx.LoggerDynamicMBeanTest.invalid"); + final LoggerDynamicMBean mbean = registerLoggerMBean(logger); + + final MBeanException thrown = assertThrows( + MBeanException.class, + () -> mbean.invoke( + "addAppender", + new Object[] {"this.class.does.not.exist.MissingAppender", "should-not-attach"}, + ADD_APPENDER_SIGNATURE)); + assertTrue(thrown.getMessage().contains("Could not instantiate appender class")); + assertInstanceOf(IllegalArgumentException.class, thrown.getTargetException()); + assertFalse(hasAppenderNamed(logger, "should-not-attach")); + } + + @Test + void addAppenderStillAttachesValidAppender() throws Exception { + final Logger logger = Logger.getLogger("jmx.LoggerDynamicMBeanTest.valid"); + final LoggerDynamicMBean mbean = registerLoggerMBean(logger); + + final Object result = mbean.invoke( + "addAppender", new Object[] {ConsoleAppender.class.getName(), "console-jmx"}, ADD_APPENDER_SIGNATURE); + assertTrue(hasAppenderNamed(logger, "console-jmx")); + // Legacy success return value, pinned so the fix cannot silently change it. + assertEquals("Hello world.", result); + } + + private static boolean hasAppenderNamed(final Logger logger, final String name) { + final Enumeration enumeration = logger.getAllAppenders(); + while (enumeration.hasMoreElements()) { + final Appender appender = (Appender) enumeration.nextElement(); + if (name.equals(appender.getName())) { + return true; + } + } + return false; + } +} diff --git a/src/changelog/.2.x.x/4185_fix_jmx_mbean_npe.xml b/src/changelog/.2.x.x/4185_fix_jmx_mbean_npe.xml new file mode 100644 index 00000000000..65e2259a3fa --- /dev/null +++ b/src/changelog/.2.x.x/4185_fix_jmx_mbean_npe.xml @@ -0,0 +1,12 @@ + + + + + Fix exceptions in JMX integration + +