ppkarwasz opened a new issue, #4339: URL: https://github.com/apache/logging-log4j2/issues/4339
`JndiManager#createManagerName` returns `JndiManager.class.getName() + '@' + JndiManager.class.hashCode()`, a constant, so every call to `getJndiManager(Properties)` or to the six-argument `getJndiManager(...)` resolves to the same entry of the `AbstractManager` cache. The first JNDI environment created in the JVM is reused by every later caller, even one that supplies a different `InitialContextFactory`, provider URL, principal or credentials, and the later environments are never instantiated. `JmsManager` is the only caller that passes properties; `JndiLookup`, `JndiContextSelector` and `DataSourceConnectionSource` use the default manager. The same code is present in `log4j-jndi` on `main`. With two `JmsAppender` instances (in one or several logger contexts) that point at different JNDI providers, the second appender looks up its connection factory and destination through the first appender's `InitialContext`, so its events either fail or are delivered through the wrong provider. This is a misrouting bug with no trust boundary involved, since everything in one JVM sharing a copy of Log4j Core is the same trusted user. It originates from a private security report classified as a bug. ## Why the shared `InitialContext` only hurts the JMS appender `InitialContext` is caller-sensitive at construction time: the JDK merges the supplied environment with the system properties and with every `jndi.properties` visible to the **thread context class loader**, loads the `InitialContextFactory` through that class loader, and keeps the resulting default context for the lifetime of the object. Names with a URL scheme such as `java:` bypass that cached context, because `getURLOrDefaultInitCtx` obtains a fresh URL context on every call, and the containers resolve those from the calling thread: Tomcat's `javaURLContextFactory` hands out a `SelectorContext` that re-resolves the bound web application on every operation, and Jetty's `javaRootURLContext` and JBoss's `ENCFactory` pick the component context from the thread context class loader at lookup time. `JndiLookup` (which prefixes `java:comp/env/`), `JndiContextSelector` (`java:comp/env/log4j/context-name`) and `DataSourceConnectionSource` (which requires a `java:` path) therefore work correctly with a shared `InitialContext`: whichever thread created it, each lookup lands in the calling application's namespace. `JmsManager` is the exception. It resolves the connection factory and destination names verbatim, typically plain names served by an external provider such as ActiveMQ through the environment's `InitialContextFactory` and provider URL, and those names go to the cached default context of the first environment created in the JVM. ## The registry buys nothing for `JndiManager` `AbstractManager` exists to keep an expensive or exclusive resource alive across a reconfiguration: a file handle, a socket, a JMS connection. The new appender picks up the old appender's manager by name, the reference count keeps the resource open, and no event is lost. An `InitialContext` is neither expensive nor exclusive: constructing one is a few hashtable merges and a factory instantiation, the JDK caches the `jndi.properties` scan and the factory class per class loader, and the container factories return a lightweight selector without any I/O. The expensive JNDI-related resource is the JMS connection, which `JmsManager` already owns and reference-counts under the appender's name. The registry actually makes the common path slower. `JndiLookup` and `JndiContextSelector` acquire the default manager in a try-with-resources block around every lookup, and nothing else holds it, so each lookup takes the global manager lock, creates a `JndiManager`, takes the lock again, drops the count to zero, removes the entry and closes the context. That is a fresh `InitialContext` per lookup anyway, plus two acquisitions of a lock shared with every appender manager in the JVM, two debug status messages and a map insert and remove. A plain `InitialContext` created and closed in the same try-with-resources block would do the same work with none of the overhead. ## Proposal Looking at the usage, the only service `JndiManager` renders is `lookup`, and it is already a thin wrapper around an `InitialContext`; the `name`, `count` and `loggerContext` fields inherited from `AbstractManager` serve no purpose. So the fix is to make every static factory (`getDefaultManager()`, `getDefaultManager(String)` and both `getJndiManager(...)` overloads) return a **new instance on every call**, constructed directly with `new InitialContext(properties)` on the calling thread and never registered in the `AbstractManager` map. This is what happens in practice already for the three `java:` callers, minus the accidental sharing between concurrent calls and the synchronization on the global manager lock, and it gives `JmsManager` the private context it needs. `stop(long, TimeUnit)` is overridden to close the context directly, so an instance never touches the registry or its lock; `JndiManagerFactory` disappears. The class keeps extending `AbstractManager` on `2.x` for binar y compatibility. On `main` it can drop the superclass and become a plain `AutoCloseable` wrapper. Deriving the manager name from the JNDI parameters instead would fix the reported collision but keep sharing one `InitialContext` between JMS appenders that supplied the same parameters from different class loaders, whose `jndi.properties` and factory class may differ; it is not worth the complexity. Objections and alternatives are welcome here. A regression test should call `getJndiManager(Properties)` twice with different provider URLs and assert that the two managers are distinct instances backed by distinct `InitialContext` objects, and that `getDefaultManager()` returns a new instance on each call. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
