8210311: IllegalArgumentException in CookieManager - Comparison method violates its general contract

Reviewed-by: chegar, dfuchs
This commit is contained in:
Michael McMahon 2018-09-13 12:07:01 +01:00
parent b395d380e8
commit b5fb6b3566
6 changed files with 240 additions and 19 deletions

View File

@ -241,7 +241,7 @@ public class CookieManager extends CookieHandler
}
// 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 @@ public class CookieManager extends CookieHandler
/*
* 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 @@ public class CookieManager extends CookieHandler
}
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;
// those with more specific Path attributes precede those with less specific
if (c1.getPath().startsWith(c2.getPath()))
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;
else if (c2.getPath().startsWith(c1.getPath()))
if (len2 > len1)
return 1;
else
return 0;
// Check creation time. Sort older first
long creation1 = c1.getCreationTime();
long creation2 = c2.getCreationTime();
if (creation1 < creation2) {
return -1;
}
if (creation1 > creation2) {
return 1;
}
return 0;
}
}
}

View File

@ -142,6 +142,13 @@ public final class HttpCookie implements Cloneable {
}
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 @@ public final class HttpCookie implements Cloneable {
toDiscard = false;
secure = false;
whenCreated = System.currentTimeMillis();
whenCreated = creationTime;
portlist = null;
this.header = header;
}
@ -756,6 +763,11 @@ public final class HttpCookie implements Cloneable {
throw new RuntimeException(e.getMessage());
}
}
// ---------------- Package private operations --------------
long getCreationTime() {
return whenCreated;
}
// ---------------- Private operations --------------

View File

@ -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 @@ class CookieTransactionHandler implements HttpHandler {
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 {

View File

@ -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
*/

View File

@ -0,0 +1,3 @@
modules = \
java.base/java.net
bootclasspath.dirs=.

View File

@ -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;
}
}