RFR: 8357708: com.sun.jndi.ldap.Connection ignores queued LDAP replies if Connection is subsequently closed [v5]

[Aleksei Efimov]

On Fri, 12 Sep 2025 06:36:31 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Can I please get a review of this change which proposes to address the issue noted in https://bugs.openjdk.org/browse/JDK-8357708?
>> 
>> As noted in the issue, the current code in `com.sun.jndi.ldap.Connection.readReply()` is susceptible to throwing a `ServiceUnavailableException` even when the LDAP replies have already been received and queued for processing. The JBS issue has additional details about how that can happen.
>> 
>> The commit in this PR simplifies the code in `com.sun.jndi.ldap.LdapRequest` to make sure it always gives out the replies that have been queued when the `LdapRequest.getReplyBer()` gets invoked. One of those queued values could be markers for a cancelled or closed request. In that case, the `getReplyBer()`, like previously, continues to throw the right exception. With this change, the call to `replies.take()` or `replies.poll()` (with an infinite timeout) is no longer expected to hang forever, if the `Connection` is closed (or the request cancelled). This then allows us to remove the connection closure (`sock == null`) check in `Connection.readReply()`.
>> 
>> A new jtreg test has been introduced to reproduce this issue and verify the fix. The test reproduces this issue consistently when the source fix isn't present. With the fix present, even after several thousand runs of this test, the issue no longer reproduces.
>> 
>> tier1, tier2 and tier3 tests continue to pass with this change. I've marked the fix version of this issue for 26 and I don't plan to push this for 25.
>
> 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 eight additional commits since the last revision:
> 
>  - Daniel's review - make the test othervm
>  - merge latest from master branch
>  - remove format() call in exception message creation
>  - merge latest from master branch
>  - merge latest from master branch
>  - merge latest from master branch
>  - add test
>  - 8357708: com.sun.jndi.ldap.Connection ignores queued LDAP replies if Connection is subsequently closed

Hello Jaikiran,

The fix and its logic looks correct to me.

I have a couple of suggestions regarding the new test:

1. The test can be modified to remove the dependency on the internal `LdapCtx` class. Something like:

@@ -43,11 +43,14 @@
 import java.util.concurrent.Future;
 import java.util.concurrent.atomic.AtomicInteger;
 
-import com.sun.jndi.ldap.LdapCtx;
 import jdk.test.lib.net.URIBuilder;
 import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Test;
+
+import javax.naming.Context;
+import javax.naming.InitialContext;
+
 import static java.nio.charset.StandardCharsets.US_ASCII;
 import static org.junit.jupiter.api.Assertions.fail;
 
@@ -56,7 +59,6 @@
  * @bug 8357708
  * @summary verify that com.sun.jndi.ldap.Connection does not ignore the LDAP replies
  *          that were received before the Connection was closed.
- * @modules java.naming/com.sun.jndi.ldap
  * @library /test/lib
  * @build jdk.test.lib.net.URIBuilder
  * @run junit/othervm LdapClientConnTest
@@ -381,17 +383,22 @@
 
         @Override
         public Void call() throws Exception {
-            LdapCtx ldapCtx = null;
+            Context ldapCtx = null;
             try {
                 final InetSocketAddress serverAddr = server.getAddress();
                 final Hashtable<String, String> envProps = new Hashtable<>();
                 // explicitly set LDAP version to 3 to prevent LDAP BIND requests
                 // during LdapCtx instantiation
                 envProps.put("java.naming.ldap.version", "3");
-                ldapCtx = new LdapCtx("",
-                        serverAddr.getAddress().getHostAddress(),
-                        serverAddr.getPort(),
-                        envProps, false);
+                envProps.put(Context.INITIAL_CONTEXT_FACTORY,
+                        "com.sun.jndi.ldap.LdapCtxFactory");
+                String providerUrl = URIBuilder.newBuilder()
+                        .scheme("ldap")
+                        .host(serverAddr.getAddress())
+                        .port(serverAddr.getPort())
+                        .build().toString();
+                envProps.put(Context.PROVIDER_URL, providerUrl);
+                ldapCtx = new InitialContext(envProps);
                 final String name = SEARCH_REQ_DN_PREFIX + taskName + SEARCH_REQ_DN_SUFFIX;
                 // trigger the LDAP SEARCH requests through the lookup call. we are not
                 // interested in the returned value and are merely interested in a normal


2. Maybe move the new test to the `test/jdk/com/sun/jndi/ldap` folder where other tests for internal LDAP classes live.

3. I like the dynamic approach on generating and parsing the LDAP messages. For future tests in JNDI/LDAP area such approach could be beneficial. Therefore, a suggestion - move `private static final byte BER_TYPE_*` constants to the `test/jdk/com/sun/jndi/ldap/lib/LDAPTestUtils.java`.

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

PR Comment: https://git.openjdk.org/jdk/pull/25449#issuecomment-3285576752