From 16b25823ab2c116e6c076f30b3c81e546ef147f7 Mon Sep 17 00:00:00 2001 From: Sebastien Tardif Date: Mon, 20 Jul 2026 17:59:32 -0400 Subject: [PATCH 1/9] fix: null-check LoggerDynamicMBean addAppender instantiation OptionConverter.instantiateByClassName returns null when the class is missing or not an Appender. addAppender immediately called setName and NPE'd on invalid JMX addAppender class names. Skip attach and log an error instead. --- .../apache/log4j/jmx/LoggerDynamicMBean.java | 4 ++ .../log4j/jmx/LoggerDynamicMBeanTest.java | 63 +++++++++++++++++++ ...fix_logger_dynamic_mbean_null_appender.xml | 8 +++ 3 files changed, 75 insertions(+) create mode 100644 log4j-1.2-api/src/test/java/org/apache/log4j/jmx/LoggerDynamicMBeanTest.java create mode 100644 src/changelog/.2.x.x/fix_logger_dynamic_mbean_null_appender.xml 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..a2a86d78f41 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 @@ -67,6 +67,10 @@ void addAppender(final String appenderClass, final String appenderName) { cat.debug("addAppender called with " + appenderClass + ", " + appenderName); final Appender appender = (Appender) OptionConverter.instantiateByClassName(appenderClass, org.apache.log4j.Appender.class, null); + if (appender == null) { + cat.error("Could not instantiate appender class [" + appenderClass + "] for name [" + appenderName + "]."); + return; + } appender.setName(appenderName); logger.addAppender(appender); 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..735f6959bb6 --- /dev/null +++ b/log4j-1.2-api/src/test/java/org/apache/log4j/jmx/LoggerDynamicMBeanTest.java @@ -0,0 +1,63 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to you under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.log4j.jmx; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.Enumeration; +import org.apache.log4j.Appender; +import org.apache.log4j.ConsoleAppender; +import org.apache.log4j.Logger; +import org.junit.jupiter.api.Test; + +/** + * Regression for invalid JMX {@code addAppender} class names: instantiation can + * return null and must not NPE when calling {@code setName}. + */ +class LoggerDynamicMBeanTest { + + @Test + void addAppenderDoesNotNpeWhenClassCannotBeInstantiated() { + final Logger logger = Logger.getLogger("jmx.LoggerDynamicMBeanTest.invalid"); + final LoggerDynamicMBean mbean = new LoggerDynamicMBean(logger); + + assertDoesNotThrow(() -> mbean.addAppender("this.class.does.not.exist.MissingAppender", "should-not-attach")); + assertFalse(hasAppenderNamed(logger, "should-not-attach")); + } + + @Test + void addAppenderStillAttachesValidAppender() { + final Logger logger = Logger.getLogger("jmx.LoggerDynamicMBeanTest.valid"); + final LoggerDynamicMBean mbean = new LoggerDynamicMBean(logger); + + mbean.addAppender(ConsoleAppender.class.getName(), "console-jmx"); + assertTrue(hasAppenderNamed(logger, "console-jmx")); + } + + 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/fix_logger_dynamic_mbean_null_appender.xml b/src/changelog/.2.x.x/fix_logger_dynamic_mbean_null_appender.xml new file mode 100644 index 00000000000..b47e24558e0 --- /dev/null +++ b/src/changelog/.2.x.x/fix_logger_dynamic_mbean_null_appender.xml @@ -0,0 +1,8 @@ + + + + Fix NPE in log4j-1.2-api `LoggerDynamicMBean.addAppender` when the appender class cannot be instantiated + From e5e9108f44f5c80e36e72817ac3d386ebf5e349b Mon Sep 17 00:00:00 2001 From: Sebastien Tardif Date: Mon, 20 Jul 2026 18:00:06 -0400 Subject: [PATCH 2/9] changelog: set issue id to PR #4185 --- src/changelog/.2.x.x/fix_logger_dynamic_mbean_null_appender.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/changelog/.2.x.x/fix_logger_dynamic_mbean_null_appender.xml b/src/changelog/.2.x.x/fix_logger_dynamic_mbean_null_appender.xml index b47e24558e0..2e8967364c0 100644 --- a/src/changelog/.2.x.x/fix_logger_dynamic_mbean_null_appender.xml +++ b/src/changelog/.2.x.x/fix_logger_dynamic_mbean_null_appender.xml @@ -3,6 +3,6 @@ xmlns="https://logging.apache.org/xml/ns" xsi:schemaLocation="https://logging.apache.org/xml/ns https://logging.apache.org/xml/ns/log4j-changelog-0.xsd" type="fixed"> - + Fix NPE in log4j-1.2-api `LoggerDynamicMBean.addAppender` when the appender class cannot be instantiated From fd932f744ee0c1dc3271919ea49281e3f7e0b474 Mon Sep 17 00:00:00 2001 From: Sebastien Tardif Date: Sat, 25 Jul 2026 07:32:41 -0400 Subject: [PATCH 3/9] fix: null-check AppenderDynamicMBean setLayout instantiation OptionConverter.instantiateByClassName returns null when the class is missing or not a Layout. setLayout immediately called appender.setLayout and could NPE on invalid JMX setLayout class names. Log an error and skip attach instead. Sibling of the LoggerDynamicMBean.addAppender guard (#4185). Signed-off-by: Sebastien Tardif --- .../log4j/jmx/AppenderDynamicMBean.java | 5 ++ .../log4j/jmx/AppenderDynamicMBeanTest.java | 57 +++++++++++++++++++ ...fix_appender_dynamic_mbean_null_layout.xml | 8 +++ 3 files changed, 70 insertions(+) create mode 100644 log4j-1.2-api/src/test/java/org/apache/log4j/jmx/AppenderDynamicMBeanTest.java create mode 100644 src/changelog/.2.x.x/fix_appender_dynamic_mbean_null_layout.xml 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..ff774512d92 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,11 @@ 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) { + cat.error("Could not instantiate layout class [" + params[0] + "] for appender [" + + getAppenderName(appender) + "]."); + return "Could not instantiate layout class."; + } appender.setLayout(layout); registerLayoutMBean(layout); } 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..485b79af35c --- /dev/null +++ b/log4j-1.2-api/src/test/java/org/apache/log4j/jmx/AppenderDynamicMBeanTest.java @@ -0,0 +1,57 @@ +/* + * 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.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.apache.log4j.ConsoleAppender; +import org.apache.log4j.PatternLayout; +import org.junit.jupiter.api.Test; + +/** + * Regression for JMX {@code setLayout}: instantiateByClassName may return null. + */ +class AppenderDynamicMBeanTest { + + @Test + void setLayoutDoesNotNpeWhenClassCannotBeInstantiated() throws Exception { + final ConsoleAppender appender = new ConsoleAppender(); + appender.setName("jmx-layout-test"); + final AppenderDynamicMBean mbean = new AppenderDynamicMBean(appender); + + final Object result = assertDoesNotThrow( + () -> mbean.invoke("setLayout", new Object[] {"this.class.does.not.exist.MissingLayout"}, new String[] { + String.class.getName() + })); + assertTrue(result == null || result.toString().contains("Could not instantiate")); + assertNull(appender.getLayout()); + } + + @Test + void setLayoutStillAttachesValidLayout() throws Exception { + final ConsoleAppender appender = new ConsoleAppender(); + appender.setName("jmx-layout-valid"); + final AppenderDynamicMBean mbean = new AppenderDynamicMBean(appender); + + mbean.invoke("setLayout", new Object[] {PatternLayout.class.getName()}, new String[] {String.class.getName()}); + assertNotNull(appender.getLayout()); + assertTrue(appender.getLayout() instanceof PatternLayout); + } +} diff --git a/src/changelog/.2.x.x/fix_appender_dynamic_mbean_null_layout.xml b/src/changelog/.2.x.x/fix_appender_dynamic_mbean_null_layout.xml new file mode 100644 index 00000000000..9d096dc52be --- /dev/null +++ b/src/changelog/.2.x.x/fix_appender_dynamic_mbean_null_layout.xml @@ -0,0 +1,8 @@ + + + + Fix NPE in log4j-1.2-api `AppenderDynamicMBean.setLayout` when the layout class cannot be instantiated + From 6c5fb8627d56836bc303cf03e7871c598abd0769 Mon Sep 17 00:00:00 2001 From: Sebastien Tardif Date: Fri, 7 Aug 2026 09:21:53 -0400 Subject: [PATCH 4/9] changelog: point AppenderDynamicMBean fix at #4185 Folded from closed #4219 per review request; both JMX null-guards ship here. Signed-off-by: Sebastien Tardif --- src/changelog/.2.x.x/fix_appender_dynamic_mbean_null_layout.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/changelog/.2.x.x/fix_appender_dynamic_mbean_null_layout.xml b/src/changelog/.2.x.x/fix_appender_dynamic_mbean_null_layout.xml index 9d096dc52be..c40259dfee7 100644 --- a/src/changelog/.2.x.x/fix_appender_dynamic_mbean_null_layout.xml +++ b/src/changelog/.2.x.x/fix_appender_dynamic_mbean_null_layout.xml @@ -3,6 +3,6 @@ xmlns="https://logging.apache.org/xml/ns" xsi:schemaLocation="https://logging.apache.org/xml/ns https://logging.apache.org/xml/ns/log4j-changelog-0.xsd" type="fixed"> - + Fix NPE in log4j-1.2-api `AppenderDynamicMBean.setLayout` when the layout class cannot be instantiated From a7e7964e8983c4e20363f372bfed4740d94f4a63 Mon Sep 17 00:00:00 2001 From: Sebastien Tardif Date: Mon, 10 Aug 2026 05:42:52 -0400 Subject: [PATCH 5/9] fix: throw MBeanException when setLayout cannot instantiate Returning a diagnostic string from a void-declared JMX operation is easy for clients to treat as success. Throw MBeanException with an IllegalArgumentException target instead so failure is unambiguous, while still logging the error. Update the regression test to assert MBeanException. Signed-off-by: Sebastien Tardif --- .../org/apache/log4j/jmx/AppenderDynamicMBean.java | 9 ++++++--- .../org/apache/log4j/jmx/AppenderDynamicMBeanTest.java | 10 +++++++--- 2 files changed, 13 insertions(+), 6 deletions(-) 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 ff774512d92..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 @@ -188,9 +188,12 @@ public Object invoke(final String operationName, final Object params[], final St final Layout layout = (Layout) OptionConverter.instantiateByClassName((String) params[0], Layout.class, null); if (layout == null) { - cat.error("Could not instantiate layout class [" + params[0] + "] for appender [" - + getAppenderName(appender) + "]."); - return "Could not instantiate layout class."; + 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/test/java/org/apache/log4j/jmx/AppenderDynamicMBeanTest.java b/log4j-1.2-api/src/test/java/org/apache/log4j/jmx/AppenderDynamicMBeanTest.java index 485b79af35c..dea501e09d5 100644 --- 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 @@ -16,11 +16,13 @@ */ package org.apache.log4j.jmx; -import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertNotNull; 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 org.apache.log4j.ConsoleAppender; import org.apache.log4j.PatternLayout; import org.junit.jupiter.api.Test; @@ -36,11 +38,13 @@ void setLayoutDoesNotNpeWhenClassCannotBeInstantiated() throws Exception { appender.setName("jmx-layout-test"); final AppenderDynamicMBean mbean = new AppenderDynamicMBean(appender); - final Object result = assertDoesNotThrow( + final MBeanException thrown = assertThrows( + MBeanException.class, () -> mbean.invoke("setLayout", new Object[] {"this.class.does.not.exist.MissingLayout"}, new String[] { String.class.getName() })); - assertTrue(result == null || result.toString().contains("Could not instantiate")); + assertTrue(thrown.getMessage().contains("Could not instantiate layout class")); + assertInstanceOf(IllegalArgumentException.class, thrown.getTargetException()); assertNull(appender.getLayout()); } From 3d073b39ad8ea871dc2b49ce382c826dee04468c Mon Sep 17 00:00:00 2001 From: Ramanathan Date: Tue, 11 Aug 2026 13:14:10 +0530 Subject: [PATCH 6/9] changelog updated --- ...c_mbean_null_layout.xml => 4185_fix_jmx_mbean_npe.xml} | 4 ++-- .../.2.x.x/fix_logger_dynamic_mbean_null_appender.xml | 8 -------- 2 files changed, 2 insertions(+), 10 deletions(-) rename src/changelog/.2.x.x/{fix_appender_dynamic_mbean_null_layout.xml => 4185_fix_jmx_mbean_npe.xml} (67%) delete mode 100644 src/changelog/.2.x.x/fix_logger_dynamic_mbean_null_appender.xml diff --git a/src/changelog/.2.x.x/fix_appender_dynamic_mbean_null_layout.xml b/src/changelog/.2.x.x/4185_fix_jmx_mbean_npe.xml similarity index 67% rename from src/changelog/.2.x.x/fix_appender_dynamic_mbean_null_layout.xml rename to src/changelog/.2.x.x/4185_fix_jmx_mbean_npe.xml index c40259dfee7..5708c24d659 100644 --- a/src/changelog/.2.x.x/fix_appender_dynamic_mbean_null_layout.xml +++ b/src/changelog/.2.x.x/4185_fix_jmx_mbean_npe.xml @@ -4,5 +4,5 @@ xsi:schemaLocation="https://logging.apache.org/xml/ns https://logging.apache.org/xml/ns/log4j-changelog-0.xsd" type="fixed"> - Fix NPE in log4j-1.2-api `AppenderDynamicMBean.setLayout` when the layout class cannot be instantiated - + Fix `NPE` in `LoggerDynamicMBean.addAppender` and `AppenderDynamicMBean.setLayout` when classes cannot be instantiated. + \ No newline at end of file diff --git a/src/changelog/.2.x.x/fix_logger_dynamic_mbean_null_appender.xml b/src/changelog/.2.x.x/fix_logger_dynamic_mbean_null_appender.xml deleted file mode 100644 index 2e8967364c0..00000000000 --- a/src/changelog/.2.x.x/fix_logger_dynamic_mbean_null_appender.xml +++ /dev/null @@ -1,8 +0,0 @@ - - - - Fix NPE in log4j-1.2-api `LoggerDynamicMBean.addAppender` when the appender class cannot be instantiated - From d88820f5ef6435fad488c12db0ed44692742140a Mon Sep 17 00:00:00 2001 From: Ramanathan Date: Tue, 11 Aug 2026 13:51:45 +0530 Subject: [PATCH 7/9] handle NPE in LoggerDynamicMBean and AppenderDynamicMBean by throwing MBeanException for invalid class names --- .../apache/log4j/jmx/LoggerDynamicMBean.java | 8 ++-- .../log4j/jmx/AppenderDynamicMBeanTest.java | 40 +++++++++++++++---- .../log4j/jmx/LoggerDynamicMBeanTest.java | 28 ++++++++++--- .../.2.x.x/4185_fix_jmx_mbean_npe.xml | 16 +++++--- 4 files changed, 69 insertions(+), 23 deletions(-) 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 a2a86d78f41..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,13 +63,15 @@ 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) { - cat.error("Could not instantiate appender class [" + appenderClass + "] for name [" + appenderName + "]."); - return; + 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 index dea501e09d5..0c68df3f5b8 100644 --- 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 @@ -23,8 +23,12 @@ 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; /** @@ -32,17 +36,35 @@ */ 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 setLayoutDoesNotNpeWhenClassCannotBeInstantiated() throws Exception { + void setLayoutFailsWhenClassCannotBeInstantiated() throws Exception { final ConsoleAppender appender = new ConsoleAppender(); appender.setName("jmx-layout-test"); - final AppenderDynamicMBean mbean = new AppenderDynamicMBean(appender); + final AppenderDynamicMBean mbean = registerAppenderMBean(appender); final MBeanException thrown = assertThrows( MBeanException.class, - () -> mbean.invoke("setLayout", new Object[] {"this.class.does.not.exist.MissingLayout"}, new String[] { - String.class.getName() - })); + () -> 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()); @@ -52,10 +74,12 @@ void setLayoutDoesNotNpeWhenClassCannotBeInstantiated() throws Exception { void setLayoutStillAttachesValidLayout() throws Exception { final ConsoleAppender appender = new ConsoleAppender(); appender.setName("jmx-layout-valid"); - final AppenderDynamicMBean mbean = new AppenderDynamicMBean(appender); + final AppenderDynamicMBean mbean = registerAppenderMBean(appender); - mbean.invoke("setLayout", new Object[] {PatternLayout.class.getName()}, new String[] {String.class.getName()}); + mbean.invoke("setLayout", new Object[] {PatternLayout.class.getName()}, SET_LAYOUT_SIGNATURE); assertNotNull(appender.getLayout()); - assertTrue(appender.getLayout() instanceof PatternLayout); + 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 index 735f6959bb6..2f04b749d01 100644 --- 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 @@ -16,11 +16,14 @@ */ package org.apache.log4j.jmx; -import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +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 org.apache.log4j.Appender; import org.apache.log4j.ConsoleAppender; import org.apache.log4j.Logger; @@ -28,26 +31,39 @@ /** * Regression for invalid JMX {@code addAppender} class names: instantiation can - * return null and must not NPE when calling {@code setName}. + * 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()}; + @Test - void addAppenderDoesNotNpeWhenClassCannotBeInstantiated() { + void addAppenderFailsWhenClassCannotBeInstantiated() throws Exception { final Logger logger = Logger.getLogger("jmx.LoggerDynamicMBeanTest.invalid"); final LoggerDynamicMBean mbean = new LoggerDynamicMBean(logger); - assertDoesNotThrow(() -> mbean.addAppender("this.class.does.not.exist.MissingAppender", "should-not-attach")); + 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() { + void addAppenderStillAttachesValidAppender() throws Exception { final Logger logger = Logger.getLogger("jmx.LoggerDynamicMBeanTest.valid"); final LoggerDynamicMBean mbean = new LoggerDynamicMBean(logger); - mbean.addAppender(ConsoleAppender.class.getName(), "console-jmx"); + 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) { 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 index 5708c24d659..127bd98ad7c 100644 --- a/src/changelog/.2.x.x/4185_fix_jmx_mbean_npe.xml +++ b/src/changelog/.2.x.x/4185_fix_jmx_mbean_npe.xml @@ -1,8 +1,12 @@ - - - Fix `NPE` in `LoggerDynamicMBean.addAppender` and `AppenderDynamicMBean.setLayout` when classes cannot be instantiated. - \ No newline at end of file + + + Fix `NPE` in `LoggerDynamicMBean.addAppender()` and `AppenderDynamicMBean.setLayout()`; both now fail with an `MBeanException` when the class cannot be instantiated. + + From b4153d59631e2249882f1d7041b169ce11a4eda5 Mon Sep 17 00:00:00 2001 From: Sebastien Tardif Date: Thu, 13 Aug 2026 05:17:43 -0700 Subject: [PATCH 8/9] test: address vy review on JMX DynamicMBean tests - Drop redundant assertNotNull beside assertInstanceOf in AppenderDynamicMBeanTest (assertInstanceOf already fails on null). - Register LoggerDynamicMBean with MBeanServer before invoke, same pattern as AppenderDynamicMBeanTest / HierarchyDynamicMBean naming. --- .../log4j/jmx/AppenderDynamicMBeanTest.java | 2 -- .../log4j/jmx/LoggerDynamicMBeanTest.java | 27 +++++++++++++++++-- 2 files changed, 25 insertions(+), 4 deletions(-) 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 index 0c68df3f5b8..597bd3822a9 100644 --- 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 @@ -17,7 +17,6 @@ package org.apache.log4j.jmx; import static org.junit.jupiter.api.Assertions.assertInstanceOf; -import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -77,7 +76,6 @@ void setLayoutStillAttachesValidLayout() throws Exception { final AppenderDynamicMBean mbean = registerAppenderMBean(appender); mbean.invoke("setLayout", new Object[] {PatternLayout.class.getName()}, SET_LAYOUT_SIGNATURE); - assertNotNull(appender.getLayout()); 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 index 2f04b749d01..8bd45969d55 100644 --- 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 @@ -24,9 +24,13 @@ 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; /** @@ -38,10 +42,29 @@ 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 = new LoggerDynamicMBean(logger); + final LoggerDynamicMBean mbean = registerLoggerMBean(logger); final MBeanException thrown = assertThrows( MBeanException.class, @@ -57,7 +80,7 @@ void addAppenderFailsWhenClassCannotBeInstantiated() throws Exception { @Test void addAppenderStillAttachesValidAppender() throws Exception { final Logger logger = Logger.getLogger("jmx.LoggerDynamicMBeanTest.valid"); - final LoggerDynamicMBean mbean = new LoggerDynamicMBean(logger); + final LoggerDynamicMBean mbean = registerLoggerMBean(logger); final Object result = mbean.invoke( "addAppender", new Object[] {ConsoleAppender.class.getName(), "console-jmx"}, ADD_APPENDER_SIGNATURE); From e8238dab63b6edb4a5d7262bc4d817dee2872478 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Volkan=20Yaz=C4=B1c=C4=B1?= Date: Fri, 14 Aug 2026 09:50:52 +0200 Subject: [PATCH 9/9] Simplify changelog --- src/changelog/.2.x.x/4185_fix_jmx_mbean_npe.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index 127bd98ad7c..65e2259a3fa 100644 --- a/src/changelog/.2.x.x/4185_fix_jmx_mbean_npe.xml +++ b/src/changelog/.2.x.x/4185_fix_jmx_mbean_npe.xml @@ -7,6 +7,6 @@ type="fixed"> - Fix `NPE` in `LoggerDynamicMBean.addAppender()` and `AppenderDynamicMBean.setLayout()`; both now fail with an `MBeanException` when the class cannot be instantiated. + Fix exceptions in JMX integration