Re: RFR [9] 8011645: CopyOnWriteArrayList.COWSubList.subList does not validate range properly
On Jan 31, 2014, at 5:23 PM, Chris Hegarty chris.hega...@oracle.com wrote:
Trivial change to CopyOnWriteArrayList.COWSubList.subList to catch the case
where the fromIndex is greater that the toIndex.
http://cr.openjdk.java.net/~chegar/8011645/webrev.00/webrev/
Looks ok to me.
--
Shame it's awkward to share this code from ArrayList:
static void subListRangeCheck(int fromIndex, int toIndex, int size) {
if (fromIndex 0)
throw new IndexOutOfBoundsException(fromIndex = + fromIndex);
if (toIndex size)
throw new IndexOutOfBoundsException(toIndex = + toIndex);
if (fromIndex toIndex)
throw new IllegalArgumentException(fromIndex( + fromIndex +
) toIndex( + toIndex + ));
}
Paul.
Re: RFR [9] 8011645: CopyOnWriteArrayList.COWSubList.subList does not validate range properly
Thanks Martin and Chris!
-Doug
On 01/31/2014 01:01 PM, Martin Buchholz wrote:
jsr166 CVS is now updated with this fix, and also adds some missing tck tests.
Index: src/jdk7/java/util/concurrent/CopyOnWriteArrayList.java
===
RCS file:
/export/home/jsr166/jsr166/jsr166/src/jdk7/java/util/concurrent/CopyOnWriteArrayList.java,v
retrieving revision 1.6
diff -u -r1.6 CopyOnWriteArrayList.java
--- src/jdk7/java/util/concurrent/CopyOnWriteArrayList.java4 Nov 2013 00:00:39
-1.6
+++ src/jdk7/java/util/concurrent/CopyOnWriteArrayList.java31 Jan 2014 17:50:14
-
@@ -1232,7 +1232,7 @@
lock.lock();
try {
checkForComodification();
-if (fromIndex 0 || toIndex size)
+if (fromIndex 0 || toIndex size || fromIndex toIndex)
throw new IndexOutOfBoundsException();
return new COWSubListE(l, fromIndex + offset,
toIndex + offset);
Index: src/main/java/util/concurrent/CopyOnWriteArrayList.java
===
RCS file:
/export/home/jsr166/jsr166/jsr166/src/main/java/util/concurrent/CopyOnWriteArrayList.java,v
retrieving revision 1.114
diff -u -r1.114 CopyOnWriteArrayList.java
--- src/main/java/util/concurrent/CopyOnWriteArrayList.java4 Nov 2013 00:00:39
-1.114
+++ src/main/java/util/concurrent/CopyOnWriteArrayList.java31 Jan 2014 17:50:15
-
@@ -1371,7 +1371,7 @@
lock.lock();
try {
checkForComodification();
-if (fromIndex 0 || toIndex size)
+if (fromIndex 0 || toIndex size || fromIndex toIndex)
throw new IndexOutOfBoundsException();
return new COWSubListE(l, fromIndex + offset,
toIndex + offset);
Index: src/test/tck/CopyOnWriteArrayListTest.java
===
RCS file:
/export/home/jsr166/jsr166/jsr166/src/test/tck/CopyOnWriteArrayListTest.java,v
retrieving revision 1.29
diff -u -r1.29 CopyOnWriteArrayListTest.java
--- src/test/tck/CopyOnWriteArrayListTest.java30 May 2013 03:28:55 -1.29
+++ src/test/tck/CopyOnWriteArrayListTest.java31 Jan 2014 17:50:15 -
@@ -500,10 +500,10 @@
* can not store the objects inside the list
*/
public void testToArray_ArrayStoreException() {
+CopyOnWriteArrayList c = new CopyOnWriteArrayList();
+c.add(zfasdfsdf);
+c.add(asdadasd);
try {
-CopyOnWriteArrayList c = new CopyOnWriteArrayList();
-c.add(zfasdfsdf);
-c.add(asdadasd);
c.toArray(new Long[5]);
shouldThrow();
} catch (ArrayStoreException success) {}
@@ -513,167 +513,196 @@
* get throws an IndexOutOfBoundsException on a negative index
*/
public void testGet1_IndexOutOfBoundsException() {
-try {
-CopyOnWriteArrayList c = new CopyOnWriteArrayList();
-c.get(-1);
-shouldThrow();
-} catch (IndexOutOfBoundsException success) {}
+CopyOnWriteArrayList c = populatedArray(5);
+List[] lists = { c, c.subList(1, c.size() - 1) };
+for (List list : lists) {
+try {
+list.get(-1);
+shouldThrow();
+} catch (IndexOutOfBoundsException success) {}
+}
}
/**
* get throws an IndexOutOfBoundsException on a too high index
*/
public void testGet2_IndexOutOfBoundsException() {
-try {
-CopyOnWriteArrayList c = new CopyOnWriteArrayList();
-c.add(asdasd);
-c.add(asdad);
-c.get(100);
-shouldThrow();
-} catch (IndexOutOfBoundsException success) {}
+CopyOnWriteArrayList c = populatedArray(5);
+List[] lists = { c, c.subList(1, c.size() - 1) };
+for (List list : lists) {
+try {
+list.get(list.size());
+shouldThrow();
+} catch (IndexOutOfBoundsException success) {}
+}
}
/**
* set throws an IndexOutOfBoundsException on a negative index
*/
public void testSet1_IndexOutOfBoundsException() {
-try {
-CopyOnWriteArrayList c = new CopyOnWriteArrayList();
-c.set(-1,qwerty);
-shouldThrow();
-} catch (IndexOutOfBoundsException success) {}
+CopyOnWriteArrayList c = populatedArray(5);
+List[] lists = { c, c.subList(1, c.size() - 1) };
+for (List list : lists) {
+try {
+list.set(-1, qwerty);
+shouldThrow();
+} catch (IndexOutOfBoundsException success) {}
+}
}
/**
* set throws an IndexOutOfBoundsException on a too
Re: RFR [9] 8011645: CopyOnWriteArrayList.COWSubList.subList does not validate range properly
Thanks I’ll update the test before pushing. I know you have tests in the TCK for this now, but I’ll add the trivial jtreg test to OpenJDK to ensure this doesn’t creep back. -Chris. On 31 Jan 2014, at 18:07, Martin Buchholz marti...@google.com wrote: The jtreg test is fine, but: s/IOBE/IOOBE/ When I created MOAT.java many years ago, I intended tests such as this to get added to that, so that all of the List implementations could share the same test code. jsr166 does not have the same concern, since it only has one List implementation at the moment. Today, there are other choices, like sharing test infrastructure with Guava e.g. ListTestSuiteBuilder. More generally, openjdk core libraries can benefit from all the great testing work that guava folk have done. On Fri, Jan 31, 2014 at 8:23 AM, Chris Hegarty chris.hega...@oracle.com wrote: Trivial change to CopyOnWriteArrayList.COWSubList.subList to catch the case where the fromIndex is greater that the toIndex. http://cr.openjdk.java.net/~chegar/8011645/webrev.00/webrev/ -Chris.
RE: RFR [9] 8011645: CopyOnWriteArrayList.COWSubList.subList does not validate range properly
Martin, Unifying the List testing code might be kind of tricky with https://bugs.openjdk.java.net/browse/JDK-4506427 as unresolved. http://docs.oracle.com/javase/7/docs/api/java/util/List.html http://docs.oracle.com/javase/7/docs/api/java/util/AbstractList.html The patch looks good though. Cheers, Jason Date: Fri, 31 Jan 2014 10:07:31 -0800 Subject: Re: RFR [9] 8011645: CopyOnWriteArrayList.COWSubList.subList does notvalidate range properly From: marti...@google.com To: chris.hega...@oracle.com CC: core-libs-dev@openjdk.java.net The jtreg test is fine, but: s/IOBE/IOOBE/ When I created MOAT.java many years ago, I intended tests such as this to get added to that, so that all of the List implementations could share the same test code. jsr166 does not have the same concern, since it only has one List implementation at the moment. Today, there are other choices, like sharing test infrastructure with Guava e.g. ListTestSuiteBuilder. More generally, openjdk core libraries can benefit from all the great testing work that guava folk have done. On Fri, Jan 31, 2014 at 8:23 AM, Chris Hegarty chris.hega...@oracle.comwrote: Trivial change to CopyOnWriteArrayList.COWSubList.subList to catch the case where the fromIndex is greater that the toIndex. http://cr.openjdk.java.net/~chegar/8011645/webrev.00/webrev/ -Chris.
Re: RFR [9] 8011645: CopyOnWriteArrayList.COWSubList.subList does not validate range properly
On Jan 31 2014, at 11:50 , Martin Buchholz marti...@google.com wrote: Jason, Thanks for pointing that out. I'm sure I have seen those bugs before (when I owned them!) but had suppressed the memory. I'm currently the assignee for this bug. I probably didn't try fixing them because there is no clean way out, and I was afraid of getting bogged down in compatibility hell for what is a non-issue for real-world users. Indeed. That's exactly why they still haven't been addressed. Suggestions are, of course, always welcome. Mike On Fri, Jan 31, 2014 at 11:43 AM, Jason Mehrens jason_mehr...@hotmail.comwrote: Martin, Unifying the List testing code might be kind of tricky with https://bugs.openjdk.java.net/browse/JDK-4506427 as unresolved. http://docs.oracle.com/javase/7/docs/api/java/util/List.html http://docs.oracle.com/javase/7/docs/api/java/util/AbstractList.html The patch looks good though. Cheers, Jason Date: Fri, 31 Jan 2014 10:07:31 -0800 Subject: Re: RFR [9] 8011645: CopyOnWriteArrayList.COWSubList.subList does not validate range properly From: marti...@google.com To: chris.hega...@oracle.com CC: core-libs-dev@openjdk.java.net The jtreg test is fine, but: s/IOBE/IOOBE/ When I created MOAT.java many years ago, I intended tests such as this to get added to that, so that all of the List implementations could share the same test code. jsr166 does not have the same concern, since it only has one List implementation at the moment. Today, there are other choices, like sharing test infrastructure with Guava e.g. ListTestSuiteBuilder. More generally, openjdk core libraries can benefit from all the great testing work that guava folk have done. On Fri, Jan 31, 2014 at 8:23 AM, Chris Hegarty chris.hega...@oracle.com wrote: Trivial change to CopyOnWriteArrayList.COWSubList.subList to catch the case where the fromIndex is greater that the toIndex. http://cr.openjdk.java.net/~chegar/8011645/webrev.00/webrev/ -Chris.
Re: RFR [9] 8011645: CopyOnWriteArrayList.COWSubList.subList does not validate range properly
On Jan 31 2014, at 15:09 , Jason Mehrens jason_mehr...@hotmail.com wrote: Martin, Mike, Totally agree with everything that has been said on this. Leaving it 'unresolved' or 'closed as will not fix' won't bother me. Mike has it listed as a 'doc clarification only' so my suggestion toward a resolution would be to modify AbstractList.subList documentation with a spec implementation note. Might be worth adding to the bug report that AbstractList.removeRange doesn't detect or specify that exceptions are thrown when 'to' is less than 'from' but, ArrayList.removeRange overrides AbstactList.removeRange with an implementation that detects and throws IOOBE. Might want to add an optional IOOBE exception to AbstractList.removeRange documentation when this patch is attempted. Added to the bug so that it doesn't get forgotten. Mike Jason Subject: Re: RFR [9] 8011645: CopyOnWriteArrayList.COWSubList.subList does not validate range properly From: mike.dui...@oracle.com Date: Fri, 31 Jan 2014 12:06:16 -0800 CC: jason_mehr...@hotmail.com; core-libs-dev@openjdk.java.net To: marti...@google.com On Jan 31 2014, at 11:50 , Martin Buchholz marti...@google.com wrote: Jason, Thanks for pointing that out. I'm sure I have seen those bugs before (when I owned them!) but had suppressed the memory. I'm currently the assignee for this bug. I probably didn't try fixing them because there is no clean way out, and I was afraid of getting bogged down in compatibility hell for what is a non-issue for real-world users. Indeed. That's exactly why they still haven't been addressed. Suggestions are, of course, always welcome. Mike
