Can I please get a review of this change which proposes to fix the issue reported in https://bugs.openjdk.org/browse/JDK-8362268?
The underlying issue here is simple - A `javax.naming.Context` for LDAP is backed by a JDK internal `com.sun.jndi.ldap.LdapCtx` instance. Each `LdapCtx` uses a `com.sun.jndi.ldap.LdapClient` instance to do the LDAP operations. Each `LdapClient` further uses a `com.sun.jndi.ldap.Connection` instance. Each `Connection` instance uses a `Socket` and the socket's `InputStream` and `OutputStream` to read/write LDAP messages from/to a LDAP server. Each `Connection` instance spawns a `Thread` to read (over the InputStream) and queue incoming messages from the LDAP server. When a LDAP backed `javax.naming.Context` initiates an operation, for example a `Context.lookup()`, then internally the LdapCtx initiates a LDAP request over the Connection's `OutputStream` and then waits for a LDAP response to arrive. In the issue reported here, it so happens that while reading over the `Connection`'s `InputStream`, the `InputStream.read()` raises an `IOException` (for whatever reason). That `IOException` rightly initiates the close of the `Connection` instance. Closing a `Connection` instance involves queuing a marker response for all waiting thread(s) to notice and raise an IOException, which they can ulimately propagate as a `NamingException` to the application. Additionally, the closing of the `Connection` also closes the `InputStream` and `OutputStream` of that `Connection`. When a thread that was waiting for a LDAP response, in LdapCtx, wakes up due to an IOException, it attempts to send a "abandon request" LDAP message over the `Connection`, so that the server knows that the client has abandoned the request. Since the Connection and its Input/OutputStream(s) are already closed, trying to write a message over the OutputStream can/will lead to an exception. The implementation of `Connection.abandonRequest(LdapRequest ldr, Control[] reqCtls)` which is where this code resides, guards against such exceptions by catching and ignoring an `IOException` from an `OutputStream.write(...)/flush()` call. Although `OutputStream.write(...)` is specified to throw an IOException if that stream is already closed, not all implementations adhere to that specification. For example, `java.io.BufferedOutputStream` does not throw any exception when `write(...)` is invoked on a closed `OutputStream`. Incidentally, the `Connection` instance's `OutputStream` is a `BufferedOutputStream`. So the fact that `Connection` attempts to write out on a closed `OutputStream` after the `Connection` has been closed has gone unnoticed in this part of the code. The reason why this shows up when SASL is invoked is because, in the presence of SASL, the LdapCtx code sets up a different (wrapping) `InputStream` and `OutputStream` for the `Connection` instance. Specifically, the `OutputStream` is now a `com.sun.jndi.ldap.sasl.SaslOutputStream`. If (for whatever reason) the `Connection`'s InputStream raises an IOException when reading from the `Connection`'s stream, then like before we close the `Connection` and attempt to write a "abandon request" message over a closed `OutputStream`. Turns out `SaslOutputStream.write(...)` doesn't have checks for closed state. When a `SaslOutputStream` is closed, it `null`s out the reference to a member field. That member field then gets used in `SaslOutputStream.write(...)` and results in a `NullPointerException` which propagates out of the `OutputStream.write(...)` call. Since `Connection.abandonRequest(...)` only guards and ignores the specified `IOException` from `OutputStream.write(...)`, this unexpected e xception gets propagated all the way to the application code. The fix is trivial - the internal implementation in `Connection.abandonRequest(...)` must not attempt to write out on the OutputStream, if the connection is already closed. That's the fix this PR proposes. A new jtreg test has been introduced to reproduce the issue and verify the fix. All tests under `test/jdk/com/sun/jndi/ldap/` directory continue to pass after this change and tier testing is currently in progress. Additionally, `com.sun.jndi.ldap.sasl.SaslOutputStream` could also be enhanced to throw `IOException` from `write(...)` and other related methods if the stream is closed. But that can be done as a separate task. ------------- Commit messages: - copyright years - alternate fix - rename test - 8362268: NPE thrown from SASL GSSAPI impl when TLS is used with QOP auth-int against Active Directory - introduce test Changes: https://git.openjdk.org/jdk/pull/29638/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=29638&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8362268 Stats: 293 lines in 3 files changed: 281 ins; 0 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/29638.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/29638/head:pull/29638 PR: https://git.openjdk.org/jdk/pull/29638
