8210311: IllegalArgumentException in CookieManager - Comparison method violates its general contract
Reviewed-by: chegar, dfuchs
--- 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;
+ }
+}