[I] AbstractConfiguration - Potential NPE on AsyncLoggerConfigDisruptor (logging-log4j2)

["JWT007 (via GitHub)" <gi...@apache.org>]

JWT007 opened a new issue, #3156:
URL: https://github.com/apache/logging-log4j2/issues/3156

   AbstractConfiguration (Log4j 2.24.1)
   
   In `AbstractConfiguration` the field `asyncLoggerConfigDisruptor`is lazily initialized.
   
   ```
   private AsyncLoggerConfigDisruptor asyncLoggerConfigDisruptor;
   
   public AsyncLoggerConfigDelegate getAsyncLoggerConfigDelegate() {
       // lazily instantiate only when requested by AsyncLoggers:
       // loading AsyncLoggerConfigDisruptor requires LMAX Disruptor jar on classpath
       if (asyncLoggerConfigDisruptor == null) {
           asyncLoggerConfigDisruptor = new AsyncLoggerConfigDisruptor(asyncWaitStrategyFactory);
       }
       return asyncLoggerConfigDisruptor;
   }
   ```
   
   The method `getAsyncLoggerConfigDelegate()` would normally be called in the constructor of `AsyncLoggerConfig(...)`:
   
   ```
   protected AsyncLoggerConfig(
               final String name,
               final List<AppenderRef> appenders,
               final Filter filter,
               final Level level,
               final boolean additive,
               final Property[] properties,
               final Configuration config,
               final boolean includeLocation) {
           super(name, appenders, filter, level, additive, properties, config, includeLocation);
           delegate = config.getAsyncLoggerConfigDelegate();
           delegate.setLogEventFactory(getLogEventFactory());
       }
   ```
   
   Here an assumption is being made that `Configuration#getAsyncLoggerConfigDelegate()` will not return `null` and properly initialiize the field, but since `Configuration` is an interface that can be implemented by anyone, anything is possible. :)
   
   The potential NPE comes up in the `AbstractConfiguration#start()` and `AbstractConfiguration#stop()` methods where it is assumed that the `asyncLoggerConfigDisruptor` has already been lazily initialized to a non-`null` value.
   
   `start()`
   ```
   if (hasAsyncLoggers()) {
       asyncLoggerConfigDisruptor.start();
   }
   ```
   `stop()`
   ```
   if (hasAsyncLoggers()) {
       LOGGER.trace("{} stopping AsyncLoggerConfigDisruptor.", cls);
       asyncLoggerConfigDisruptor.stop(timeout, timeUnit);
   }
   ```
   
   A potential fix is *either* doing a simple null-check or maybe adding a protected API (since you can't call `start()` on the `AsyncLoggerConfigDelegate`:
   
   ```
   @Override
   public AsyncLoggerConfigDelegate getAsyncLoggerConfigDelegate() {
      return getAsyncLoggerConfigDisruptor();
   }
   
   protected AsyncLoggerConfigDisruptor getAsyncLoggerConfigDisruptor() {
       // lazily instantiate only when requested by AsyncLoggers:
       // loading AsyncLoggerConfigDisruptor requires LMAX Disruptor jar on classpath
       if (asyncLoggerConfigDisruptor == null) {
           asyncLoggerConfigDisruptor = new AsyncLoggerConfigDisruptor(asyncWaitStrategyFactory);
       }
       return asyncLoggerConfigDisruptor;
   }
   
   public void start() {
       ...
       if (hasAsyncLoggers()) {
           getAsyncLoggerConfigDisruptor().start();
       }
       ...
   }
   
   public void stop() {
       ...
       if (hasAsyncLoggers()) {
           getAsyncLoggerConfigDisruptor().stop(timeout, timeUnit);
       }
       ...
   }
   ```


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] AbstractConfiguration - Potential NPE on AsyncLoggerConfigDisruptor (logging-log4j2)

["JWT007 (via GitHub)" <gi...@apache.org>]

JWT007 commented on issue #3156:
URL: https://github.com/apache/logging-log4j2/issues/3156#issuecomment-2454856232

   NOTE: additionally I *think* the constructor will throw an exception if the LMX Jar is not on the classpath (at the very latest in start/stop) but I am not sure how exactly it expresses itselff :)


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org