8191169: java/net/Authenticator/B4769350.java failed intermittently

Fixed a race condition in AuthenticationInfo when serializeAuth=true

Reviewed-by: chegar, michaelm

src/java.base/share/classes/sun/net/www/protocol/http/AuthenticationInfo.java

 /*
- * Copyright (c) 1995, 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1995, 2019, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -31,6 +31,7 @@ import java.net.PasswordAuthentication;
 import java.net.URL;
 import java.util.HashMap;
 import java.util.Objects;
+import java.util.function.Function;
 
 import sun.net.www.HeaderParser;
 
@@ -125,25 +126,42 @@ public abstract class AuthenticationInfo extends AuthCacheValue implements Clone
      */
     private static HashMap<String,Thread> requests = new HashMap<>();
 
-    /* check if a request for this destination is in progress
-     * return false immediately if not. Otherwise block until
-     * request is finished and return true
+    /*
+     * check if AuthenticationInfo is available in the cache.
+     * If not, check if a request for this destination is in progress
+     * and if so block until the other request is finished authenticating
+     * and returns the cached authentication value.
+     * Otherwise, returns the cached authentication value, which may be null.
      */
-    private static boolean requestIsInProgress (String key) {
-        if (!serializeAuth) {
-            /* behavior is disabled. Revert to concurrent requests */
-            return false;
+    private static AuthenticationInfo requestAuthentication(String key, Function<String, AuthenticationInfo> cache) {
+        AuthenticationInfo cached = cache.apply(key);
+        if (cached != null || !serializeAuth) {
+            // either we already have a value in the cache, and we can
+            // use that immediately, or the serializeAuth behavior is disabled,
+            // and we can revert to concurrent requests
+            return cached;
         }
         synchronized (requests) {
+            // check again after synchronizing, and if available
+            // just return the cached value.
+            cached = cache.apply(key);
+            if (cached != null) return cached;
+
+            // Otherwise, if no request is in progress, record this
+            // thread as performing authentication and returns null.
             Thread t, c;
             c = Thread.currentThread();
             if ((t = requests.get(key)) == null) {
                 requests.put (key, c);
-                return false;
+                assert cached == null;
+                return cached;
             }
             if (t == c) {
-                return false;
+                assert cached == null;
+                return cached;
             }
+            // Otherwise, an other thread is currently performing authentication:
+            // wait until it finishes.
             while (requests.containsKey(key)) {
                 try {
                     requests.wait ();
@@ -151,7 +169,7 @@ public abstract class AuthenticationInfo extends AuthCacheValue implements Clone
             }
         }
         /* entry may be in cache now. */
-        return true;
+        return cache.apply(key);
     }
 
     /* signal completion of an authentication (whether it succeeded or not)
@@ -318,13 +336,13 @@ public abstract class AuthenticationInfo extends AuthCacheValue implements Clone
         return key;
     }
 
-    static AuthenticationInfo getServerAuth(String key) {
-        AuthenticationInfo cached = getAuth(key, null);
-        if ((cached == null) && requestIsInProgress (key)) {
-            /* check the cache again, it might contain an entry */
-            cached = getAuth(key, null);
+    private static AuthenticationInfo getCachedServerAuth(String key) {
+        return getAuth(key, null);
     }
-        return cached;
+
+    static AuthenticationInfo getServerAuth(String key) {
+        if (!serializeAuth) return getCachedServerAuth(key);
+        return requestAuthentication(key, AuthenticationInfo::getCachedServerAuth);
     }
 
 
@@ -367,13 +385,13 @@ public abstract class AuthenticationInfo extends AuthCacheValue implements Clone
         return key;
     }
 
-    static AuthenticationInfo getProxyAuth(String key) {
-        AuthenticationInfo cached = (AuthenticationInfo) cache.get(key, null);
-        if ((cached == null) && requestIsInProgress (key)) {
-            /* check the cache again, it might contain an entry */
-            cached = (AuthenticationInfo) cache.get(key, null);
+    private static AuthenticationInfo getCachedProxyAuth(String key) {
+        return (AuthenticationInfo) cache.get(key, null);
     }
-        return cached;
+
+    static AuthenticationInfo getProxyAuth(String key) {
+        if (!serializeAuth) return getCachedProxyAuth(key);
+        return requestAuthentication(key, AuthenticationInfo::getCachedProxyAuth);
     }
 
 

test/jdk/java/net/Authenticator/B4769350.java

@@ -23,7 +23,7 @@
 
 /**
  * @test
- * @bug 4769350 8017779
+ * @bug 4769350 8017779 8191169
  * @modules jdk.httpserver
  * @run main/othervm B4769350 server
  * @run main/othervm B4769350 proxy

test/jdk/java/net/HttpURLConnection/SetAuthenticator/HTTPSetAuthenticatorTest.java

 /*
- * Copyright (c) 2016, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 2019, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -115,8 +115,8 @@ public class HTTPSetAuthenticatorTest extends HTTPTest {
                    HttpAuthType mode)
             throws IOException
     {
-        HttpTestAuthenticator authOne = new HttpTestAuthenticator("dublin", "foox");
-        HttpTestAuthenticator authTwo = new HttpTestAuthenticator("dublin", "foox");
+        HttpTestAuthenticator authOne = new HttpTestAuthenticator("authOne", "dublin", "foox");
+        HttpTestAuthenticator authTwo = new HttpTestAuthenticator("authTwo", "dublin", "foox");
         int expectedIncrement = scheme == HttpSchemeType.NONE
                                 ? 0 : EXPECTED_AUTH_CALLS_PER_TEST;
         int count;

test/jdk/java/net/HttpURLConnection/SetAuthenticator/HTTPTest.java

@@ -89,8 +89,10 @@ public class HTTPTest {
         // count will be incremented every time getPasswordAuthentication()
         // is called from the client side.
         final AtomicInteger count = new AtomicInteger();
+        private final String name;
 
-        public HttpTestAuthenticator(String realm, String username) {
+        public HttpTestAuthenticator(String name, String realm, String username) {
+            this.name = name;
             this.realm = realm;
             this.username = username;
         }
@@ -98,7 +100,7 @@ public class HTTPTest {
         @Override
         protected PasswordAuthentication getPasswordAuthentication() {
             if (skipCount.get() == null || skipCount.get().booleanValue() == false) {
-                System.out.println("Authenticator called: " + count.incrementAndGet());
+                System.out.println("Authenticator " + name + " called: " + count.incrementAndGet());
             }
             return new PasswordAuthentication(getUserName(),
                     new char[] {'b','a','r'});
@@ -118,6 +120,11 @@ public class HTTPTest {
             throw new SecurityException("User unknown: " + user);
         }
 
+        @Override
+        public String toString() {
+            return super.toString() + "[name=\"" + name + "\"]";
+        }
+
         public final String getUserName() {
             return username;
         }
@@ -128,7 +135,7 @@ public class HTTPTest {
     }
     public static final HttpTestAuthenticator AUTHENTICATOR;
     static {
-        AUTHENTICATOR = new HttpTestAuthenticator("dublin", "foox");
+        AUTHENTICATOR = new HttpTestAuthenticator("AUTHENTICATOR","dublin", "foox");
         Authenticator.setDefault(AUTHENTICATOR);
     }
 

test/jdk/java/net/HttpURLConnection/SetAuthenticator/HTTPTestServer.java

@@ -165,7 +165,7 @@ public class HTTPTestServer extends HTTPTest {
                 for (int i = 1; i <= max; i++) {
                     B bindable = createBindable();
                     SocketAddress address = getAddress(bindable);
-                    String key = address.toString();
+                    String key = toString(address);
                     if (addresses.addIfAbsent(key)) {
                        System.out.println("Socket bound to: " + key
                                           + " after " + i + " attempt(s)");
@@ -188,6 +188,16 @@ public class HTTPTestServer extends HTTPTest {
                                   + "addresses used before: " + addresses);
         }
 
+        private static String toString(SocketAddress address) {
+            // We don't rely on address.toString(): sometimes it can be
+            // "/127.0.0.1:port", sometimes it can be "localhost/127.0.0.1:port"
+            // Instead we compose our own string representation:
+            InetSocketAddress candidate = (InetSocketAddress) address;
+            String hostAddr = candidate.getAddress().getHostAddress();
+            if (hostAddr.contains(":")) hostAddr = "[" + hostAddr + "]";
+            return hostAddr + ":" + candidate.getPort();
+        }
+
         protected abstract B createBindable() throws IOException;
 
         protected abstract SocketAddress getAddress(B bindable);