Good to see the test added. I was also unable to reproduce the failure on my Core 2 Duo Mac laptop but the test and fix match up.
Mike On May 14 2013, at 04:34 , Peter Levart wrote: > On 05/14/2013 01:17 PM, David Holmes wrote: >> Thanks Peter this looks good to me. >> >> One minor thing please set the correct copyright year in the test, it should >> just be >> >> Copyright (c) 2013, Oracle ... > > Ok, fixed: > > http://cr.openjdk.java.net/~plevart/jdk8-tl/String.contentEquals/webrev.04/ > > >> >> I couldn't get the test to actually fail, but I can see that it could. > > Hm, do you have a multi-core chip? On my computer (i7 CPU) it fails > immediately and always. You could try varying the number of concurrent > threads and or the time to wait for exception to be thrown... > >> >> David >> >> On 14/05/2013 9:04 PM, Peter Levart wrote: >>> Ok, here's the corrected fix: >>> >>> http://cr.openjdk.java.net/~plevart/jdk8-tl/String.contentEquals/webrev.03/ >>> >>> I also added a reproducer for the bug. >>> >>> Regards, peter >>> >>> On 05/14/2013 07:53 AM, Peter Levart wrote: >>>> >>>> Right, sb.length() should be used. I will correct that as soon as I >>>> get to the keyboard. >>>> >>>> Regards, Peter >>>> >>>> On May 14, 2013 7:22 AM, "David Holmes" <david.hol...@oracle.com >>>> <mailto:david.hol...@oracle.com>> wrote: >>>> >>>> Thanks Peter! I've filed >>>> >>>> 8014477 >>>> Race condition in String.contentEquals when comparing with >>>> StringBuffer >>>> >>>> On 14/05/2013 8:34 AM, Peter Levart wrote: >>>> >>>> On 05/14/2013 12:09 AM, Peter Levart wrote: >>>> >>>> I noticed a synchronization bug in String.contentEquals >>>> method. If >>>> called with a StringBuffer argument while concurrent thread is >>>> modifying the StringBuffer, the method can either throw >>>> ArrayIndexOutOfBoundsException or return true even though >>>> the content >>>> of the StringBuffer has never been the same as the String's. >>>> >>>> Here's a proposed patch: >>>> >>>> http://cr.openjdk.java.net/~plevart/jdk8-tl/String.contentEquals/webrev.01/ >>>> <http://cr.openjdk.java.net/%7Eplevart/jdk8-tl/String.contentEquals/webrev.01/> >>>> >>>> Regards, Peter >>>> >>>> >>>> Or even better (with some code clean-up): >>>> >>>> http://cr.openjdk.java.net/~plevart/jdk8-tl/String.contentEquals/webrev.02/ >>>> <http://cr.openjdk.java.net/%7Eplevart/jdk8-tl/String.contentEquals/webrev.02/> >>>> >>>> >>>> This part is not correct I believe: >>>> >>>> 1010 private boolean >>>> nonSyncContentEquals(AbstractStringBuilder sb) { >>>> 1011 char v1[] = value; >>>> 1012 char v2[] = sb.getValue(); >>>> 1013 int n = v1.length; >>>> 1014 if (n != v2.length) { >>>> 1015 return false; >>>> 1016 } >>>> >>>> v2 can be larger than v1 if v2 is not being fully used (ie count < >>>> length). >>>> >>>> David >>>> ----- >>>> >>> >
