> 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 `Bu...

Jaikiran Pai has updated the pull request with a new target base due to a merge 
or a rebase. The incremental webrev excludes the unrelated changes brought in 
by the merge/rebase. The pull request contains 11 additional commits since the 
last revision:

 - merge latest from master branch
 - Daniel's review - recheck while holding the lock
 - merge latest from master branch
 - merge latest from master branch
 - merge latest from master branch
 - add comment
 - copyright years
 - alternate fix
 - rename test
 - 8362268: NPE thrown from SASL GSSAPI impl when  TLS is  used with QOP 
auth-int against Active Directory
 - ... and 1 more: https://git.openjdk.org/jdk/compare/e63a50da...fa582f62

-------------

Changes:
  - all: https://git.openjdk.org/jdk/pull/29638/files
  - new: https://git.openjdk.org/jdk/pull/29638/files/52517a9d..fa582f62

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=29638&range=04
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=29638&range=03-04

  Stats: 85637 lines in 374 files changed: 35843 ins; 35691 del; 14103 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

Reply via email to