8210311: IllegalArgumentException in CookieManager - Comparison method violates its general contract
authormichaelm
Thu, 13 Sep 2018 12:07:01 +0100
changeset 51724 8abb0fa2c334
parent 51723 d3ada4479724
child 51725 ccea318862ae
8210311: IllegalArgumentException in CookieManager - Comparison method violates its general contract Reviewed-by: chegar, dfuchs
src/java.base/share/classes/java/net/CookieManager.java
src/java.base/share/classes/java/net/HttpCookie.java
test/jdk/java/net/CookieHandler/CookieManagerTest.java
test/jdk/java/net/whitebox/CookieTestDriver.java
test/jdk/java/net/whitebox/TEST.properties
test/jdk/java/net/whitebox/java.base/java/net/CookieManagerTest.java
--- a/src/java.base/share/classes/java/net/CookieManager.java	Thu Sep 13 12:41:42 2018 +0200
+++ b/src/java.base/share/classes/java/net/CookieManager.java	Thu Sep 13 12:07:01 2018 +0100
@@ -241,7 +241,7 @@
         }
 
         // apply sort rule (RFC 2965 sec. 3.3.4)
-        List<String> cookieHeader = sortByPath(cookies);
+        List<String> cookieHeader = sortByPathAndAge(cookies);
 
         return Map.of("Cookie", cookieHeader);
     }
@@ -402,11 +402,12 @@
 
 
     /*
-     * sort cookies with respect to their path: those with more specific Path attributes
-     * precede those with less specific, as defined in RFC 2965 sec. 3.3.4
+     * sort cookies with respect to their path and age: those with more longer Path attributes
+     * precede those with shorter, as defined in RFC 6265. Cookies with the same length
+     * path are distinguished by creation time (older first). Method made PP to enable testing.
      */
-    private List<String> sortByPath(List<HttpCookie> cookies) {
-        Collections.sort(cookies, new CookiePathComparator());
+    static List<String> sortByPathAndAge(List<HttpCookie> cookies) {
+        Collections.sort(cookies, new CookieComparator());
 
         List<String> cookieHeader = new java.util.ArrayList<>();
         for (HttpCookie cookie : cookies) {
@@ -424,22 +425,36 @@
     }
 
 
-    static class CookiePathComparator implements Comparator<HttpCookie> {
+    // Comparator compares the length of the path. Longer paths should precede shorter ones.
+    // As per rfc6265 cookies with equal path lengths sort on creation time.
+
+    static class CookieComparator implements Comparator<HttpCookie> {
         public int compare(HttpCookie c1, HttpCookie c2) {
             if (c1 == c2) return 0;
             if (c1 == null) return -1;
             if (c2 == null) return 1;
 
-            // path rule only applies to the cookies with same name
-            if (!c1.getName().equals(c2.getName())) return 0;
+            String p1 = c1.getPath();
+            String p2 = c2.getPath();
+            p1 = (p1 == null) ? "" : p1;
+            p2 = (p2 == null) ? "" : p2;
+            int len1 = p1.length();
+            int len2 = p2.length();
+            if (len1 > len2)
+                return -1;
+            if (len2 > len1)
+                return 1;
 
-            // those with more specific Path attributes precede those with less specific
-            if (c1.getPath().startsWith(c2.getPath()))
+            // Check creation time. Sort older first
+            long creation1 = c1.getCreationTime();
+            long creation2 = c2.getCreationTime();
+            if (creation1 < creation2) {
                 return -1;
-            else if (c2.getPath().startsWith(c1.getPath()))
+            }
+            if (creation1 > creation2) {
                 return 1;
-            else
-                return 0;
+            }
+            return 0;
         }
     }
 }
--- a/src/java.base/share/classes/java/net/HttpCookie.java	Thu Sep 13 12:41:42 2018 +0200
+++ b/src/java.base/share/classes/java/net/HttpCookie.java	Thu Sep 13 12:07:01 2018 +0100
@@ -142,6 +142,13 @@
     }
 
     private HttpCookie(String name, String value, String header) {
+        this(name, value, header, System.currentTimeMillis());
+    }
+
+    /**
+     * Package private for testing purposes.
+     */
+    HttpCookie(String name, String value, String header, long creationTime) {
         name = name.trim();
         if (name.length() == 0 || !isToken(name) || name.charAt(0) == '$') {
             throw new IllegalArgumentException("Illegal cookie name");
@@ -152,7 +159,7 @@
         toDiscard = false;
         secure = false;
 
-        whenCreated = System.currentTimeMillis();
+        whenCreated = creationTime;
         portlist = null;
         this.header = header;
     }
@@ -756,6 +763,11 @@
             throw new RuntimeException(e.getMessage());
         }
     }
+    // ---------------- Package private operations --------------
+
+    long getCreationTime() {
+        return whenCreated;
+    }
 
     // ---------------- Private operations --------------
 
--- a/test/jdk/java/net/CookieHandler/CookieManagerTest.java	Thu Sep 13 12:41:42 2018 +0200
+++ b/test/jdk/java/net/CookieHandler/CookieManagerTest.java	Thu Sep 13 12:07:01 2018 +0100
@@ -31,6 +31,9 @@
  */
 
 import com.sun.net.httpserver.*;
+import java.util.Collections;
+import java.util.LinkedList;
+import java.util.List;
 import java.io.IOException;
 import java.net.*;
 import static java.net.Proxy.NO_PROXY;
@@ -160,14 +163,40 @@
         exchange.close();
     }
 
+    private static String trim(String s) {
+        StringBuilder sb = new StringBuilder();
+        for (int i=0; i<s.length(); i++) {
+            char c = s.charAt(i);
+            if (!Character.isWhitespace(c))
+                sb.append(c);
+        }
+        return sb.toString();
+    }
+
+    private static boolean cookieEquals(String s1, String s2) {
+        s1 = trim(s1);
+        s2 = trim(s2);
+        String[] s1a = s1.split(";");
+        String[] s2a = s2.split(";");
+        List<String> l1 = new LinkedList(List.of(s1a));
+        List<String> l2 = new LinkedList(List.of(s2a));
+        Collections.sort(l1);
+        Collections.sort(l2);
+        int i = 0;
+        for (String s : l1) {
+            if (!s.equals(l2.get(i++))) {
+                return false;
+            }
+        }
+        return true;
+    }
+
     private void checkRequest(Headers hdrs) {
 
         assert testDone > 0;
         String cookieHeader = hdrs.getFirst("Cookie");
-        if (cookieHeader != null &&
-            cookieHeader
-                .equalsIgnoreCase(testCases[testcaseDone][testDone-1]
-                                  .cookieToRecv))
+        if (cookieHeader != null && cookieEquals(
+                cookieHeader, testCases[testcaseDone][testDone-1].cookieToRecv))
         {
             System.out.printf("%15s %s\n", "PASSED:", cookieHeader);
         } else {
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/test/jdk/java/net/whitebox/CookieTestDriver.java	Thu Sep 13 12:07:01 2018 +0100
@@ -0,0 +1,29 @@
+/*
+ * Copyright (c) 2018, 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
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test
+ * @modules java.base/java.net
+ * @run main/othervm java.base/java.net.CookieManagerTest
+ */
+
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/test/jdk/java/net/whitebox/TEST.properties	Thu Sep 13 12:07:01 2018 +0100
@@ -0,0 +1,3 @@
+modules = \
+    java.base/java.net
+bootclasspath.dirs=.
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/test/jdk/java/net/whitebox/java.base/java/net/CookieManagerTest.java	Thu Sep 13 12:07:01 2018 +0100
@@ -0,0 +1,133 @@
+/*
+ * Copyright (c) 2018, 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
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+package java.net;
+
+import java.net.CookieManager;
+import java.net.HttpCookie;
+import java.util.*;
+import java.util.stream.Collectors;
+
+// Whitebox Test of CookieManager.sortByPathAndAge
+
+public class CookieManagerTest
+{
+    Random r1; // random is reseeded so it is reproducible
+    long seed;
+
+    public static void main(String[] args) {
+        CookieManagerTest test = new CookieManagerTest();
+        test.run();
+    }
+
+    CookieManagerTest() {
+        r1 = new Random();
+        seed = r1.nextLong();
+        System.out.println("Random seed is: " + seed);
+        r1.setSeed(seed);
+    }
+
+    static class TestCookie {
+        String path;
+        long creationTime;
+
+        static TestCookie of(String path, long creationTime) {
+            TestCookie t = new TestCookie();
+            t.path = path;
+            t.creationTime = creationTime;
+            return t;
+        }
+    }
+
+    // The order shown below is what we expect the sort to produce
+    // longest paths first then for same length path by age (oldest first)
+    // This array is copied to a list and shuffled before being passed to
+    // the sort function several times.
+
+    TestCookie[] cookies = new TestCookie[] {
+        TestCookie.of("alpha/bravo/charlie/delta", 50),
+        TestCookie.of("alphA/Bravo/charlie/delta", 100),
+        TestCookie.of("bravo/charlie/delta", 1),
+        TestCookie.of("bravo/chArlie/delta", 2),
+        TestCookie.of("bravo/charlie/delta", 5),
+        TestCookie.of("bravo/charliE/dElta", 10),
+        TestCookie.of("charlie/delta", 1),
+        TestCookie.of("charlie/delta", 1),
+        TestCookie.of("charlie/delta", 1),
+        TestCookie.of("charlie/delta", 2),
+        TestCookie.of("charlie/delta", 2),
+        TestCookie.of("charlie/delta", 2),
+        TestCookie.of("ChaRlie/delta", 3),
+        TestCookie.of("charliE/deLta", 4),
+        TestCookie.of("cHarlie/delta", 7),
+        TestCookie.of("chaRRie/delta", 9),
+        TestCookie.of("delta", 999),
+        TestCookie.of("Delta", 1000),
+        TestCookie.of("Delta", 1000),
+        TestCookie.of("DeLta", 1001),
+        TestCookie.of("DeLta", 1002),
+        TestCookie.of("/", 2),
+        TestCookie.of("/", 3),
+        TestCookie.of("/", 300)
+    };
+
+    public void run()
+    {
+        for (int i=0; i<100; i++) {
+            List<TestCookie> l1 = new LinkedList(Arrays.asList(cookies));
+            Collections.shuffle(l1, r1);
+            List<HttpCookie> l2 = l1
+                    .stream()
+                    .map(this::createCookie)
+                    .collect(Collectors.toList());
+
+
+            // call PP method of CookieManager
+            List<String> result = CookieManager.sortByPathAndAge(l2);
+            int index = 0;
+            for (String r : result) {
+                if (!r.contains("name=\"value\""))
+                    continue;
+                // extract Path value
+                r = r.substring(r.indexOf("Path=")+6);
+                // remove trailing "
+
+                // should go from name="value";$Path="bravo/charlie/delta" ->
+                //                                    bravo/charlie/delta
+                r = r.substring(0, r.indexOf('"'));
+                if (!r.equals(cookies[index].path)) {
+                    System.err.printf("ERROR: got %s index: %d", r, index);
+                    System.err.printf(" expected: %s\n", cookies[index].path);
+                    throw new RuntimeException("Test failed");
+                }
+                index++;
+            }
+        }
+    }
+
+    private HttpCookie createCookie(TestCookie tc) {
+        HttpCookie ck = new HttpCookie("name", "value", null, tc.creationTime);
+        ck.setPath(tc.path);
+        return ck;
+    }
+}