bug 8014477 Race in String.contentEquals

[David Holmes]

On 14/05/2013 9:34 PM, 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/

Thanks.

>> 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...

24-way x86 system. I tried changing threads and timeout but still 
couldn't trigger a failure. Same for my 4-way win box.

David

>>
>> 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.holmes at oracle.com
>>>> <mailto:david.holmes at 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
>>>>     -----
>>>>
>>>
>