fix: null-check LoggerDynamicMBean addAppender and AppenderDynamicMBean setLayout

[SebTardif]

What Problem This Solves

log4j-1.2-api JMX helpers call OptionConverter.instantiateByClassName(..., null) and immediately use the result. When the class name is invalid or not the expected type, instantiation returns null and the next line NPEs. That surfaces through JMX operations with a bad class name.

Two sibling sites:

1. LoggerDynamicMBean.addAppender → NPE on appender.setName(...)
2. AppenderDynamicMBean.setLayout → NPE on appender.setLayout(...) (folded from closed fix: null-check AppenderDynamicMBean setLayout instantiation #4219)

Failure scenario

// LoggerDynamicMBean.addAppender
final Appender appender =
    (Appender) OptionConverter.instantiateByClassName(appenderClass, Appender.class, null);
appender.setName(appenderName); // NPE when instantiate returns null

// AppenderDynamicMBean.setLayout
final Layout layout =
    (Layout) OptionConverter.instantiateByClassName((String) params[0], Layout.class, null);
appender.setLayout(layout); // NPE when instantiate returns null

Fix

Null-check after instantiation. On failure, log via cat.error(...) and throw MBeanException wrapping IllegalArgumentException so JMX clients get a clear operation failure (void operations already return null on success; a free-form diagnostic string is not a reliable client signal).

Both addAppender and setLayout use the same fail-closed path. Regression tests exercise the operations through mbean.invoke(...) on a registered MBeanServer, including the success path.

Tests

export JAVA_HOME=$(/usr/libexec/java_home -v 17)
./mvnw -pl log4j-1.2-api -am test \
  -Dtest=LoggerDynamicMBeanTest,AppenderDynamicMBeanTest \
  -Dsurefire.failIfNoSpecifiedTests=false

• Invalid class: no NPE; invoke fails with MBeanException; appender/layout remains unset
• Valid ConsoleAppender / PatternLayout: object attached via registered MBeanServer invoke

Sibling audit

• registerLayoutMBean: already null-safe
• No other instantiateByClassName + immediate dereference in log4j-1.2-api JMX package

Changelog

Consolidated into one entry under src/changelog/.2.x.x/, linked to this PR (#4185).

Note

#4219 carried only the AppenderDynamicMBean half and was closed with a request to fold into this PR. Done in follow-up commits on this branch.

[ramanathan1504]

@SebTardif Thank you for catching and fixing this NPE. This is your 4th PR, and we truly appreciate your ongoing support. If possible, could you consider picking up some items from the issue list? That would be incredibly helpful for our team. Of course, please continue submitting fixes for NPEs and other issues as you find them—they are always welcome. I will aim to review and close this PR this weekend.

[ramanathan1504]

LGTM!

[SebTardif]

@ramanathan1504 Folded the AppenderDynamicMBean setLayout null-check (from closed #4219) into this branch, plus tests and changelog entries both pointing at #4185.

Local check:

export JAVA_HOME=$(/usr/libexec/java_home -v 17)
./mvnw -pl log4j-1.2-api -am test \
  -Dtest=LoggerDynamicMBeanTest,AppenderDynamicMBeanTest \
  -Dsurefire.failIfNoSpecifiedTests=false

All 4 tests pass. Ready for re-review when convenient.

Because of added related code from #4219, so need to review again

[ramanathan1504]

One question before I re-approve: setLayout returns the string
"Could not instantiate layout class." from invoke(). A JMX client cannot
tell that apart from a successful result — should it throw an MBeanException
instead, or is returning a diagnostic string the established convention for this
MBean? Happy either way, I just want it to be deliberate.

[SebTardif]

@ramanathan1504

One question before I re-approve: setLayout returns the string "Could not instantiate layout class." from invoke(). A JMX client cannot tell that apart from a successful result. Should it throw an MBeanException instead, or is returning a diagnostic string the established convention for this MBean? Happy either way, I just want it to be deliberate.

Good catch. I looked at the package conventions and chose throw MBeanException deliberately:

1. setLayout is registered as a void operation (MBeanOperationInfo return type "void"). Success already returns null; a free-form error string is not a reliable client signal.
2. DynamicMBean.invoke already declares throws MBeanException. That is the standard JMX way to report an operation failure to the client.
3. The sibling paths mostly use exceptions for invalid API use (RuntimeOperationsException for null attribute/op names, ReflectionException for unknown ops in HierarchyDynamicMBean). Informal strings like "Options activated." / "Hello world." are success-ish quirks, not an established failure convention.
4. Server-side cat.error(...) is kept so the failure still shows up in logs.

Pushed in a7e7964e89: null instantiate path logs, then throw new MBeanException(new IllegalArgumentException(message), message). Regression test now expects MBeanException with that target.

Note (out of scope for this PR): LoggerDynamicMBean.invoke still returns "Hello world." after a failed addAppender instantiate (void helper returns early). Happy to open a follow-up if you want the same fail-closed JMX signal there.

Local verification:

export JAVA_HOME=$(/usr/libexec/java_home -v 17)
./mvnw -pl log4j-1.2-api -am test \
  -Dtest=LoggerDynamicMBeanTest,AppenderDynamicMBeanTest \
  -Dsurefire.failIfNoSpecifiedTests=false

All 4 tests pass. Ready for re-approve when convenient. Thanks again for the careful review.

[ramanathan1504]

Thanks @SebTardif. I pushed two commits on top: addAppender now throws MBeanException like setLayout so both sites fail the same way, and the tests go through mbean.invoke(...) with a registered MBeanServer so the success path is actually exercised. Changelog consolidated into one entry.

LGTM — Only thing left: the description still says "log an error and skip attach", could you update it to match?

[SebTardif]

@ramanathan1504 Updated the PR description to match the tip: both sites log and throw MBeanException (no more "log an error and skip attach"), and tests go through mbean.invoke(...) on a registered MBeanServer. Thanks again for the commits on top and the careful review.

[vy]

LGTM, dropped some minor remarks.

@SebTardif, @ramanathan1504, great work! Thanks so much!

[SebTardif]

@vy Follow-ups from your review are on the tip (b4153d59):

1. Dropped the redundant assertNotNull next to assertInstanceOf.
2. LoggerDynamicMBeanTest now registers the logger MBean on an MBeanServer (same ObjectName("log4j", "logger", name) shape as HierarchyDynamicMBean.addLoggerMBean) and drives addAppender through mbeanServer.invoke.

Could you take another look when you have a moment?