bug 8014477 Race in String.contentEquals

[Peter Levart]

Hi,

Right, the test calls the correctly synchronized method in JDK7. The fix 
for 6914123 in JDK8 moves synchronization to the other method (the one 
taking CharSequence as parameter type), so in JDK8 it doesn't matter 
which method is called, but in JDK7 it makes a difference. The same test 
could be modified to call the unsynchronized method in JDK7 (by casting 
StringBuffer to CharSequence) in case a backport to JDK7 is attempted, 
but for JDK8 it is good as it is.

Regards, Peter

On 05/16/2013 06:08 AM, David Holmes wrote:
> Okay mea culpa - I was testing on the wrong JDK. I wrongly assumed 
> this would impact 7 as well but it doesn't as the bug was introduced 
> with the changes in 6914123.
>
> Sorry about that.
>
> Good to go. Has anyone offered to sponsor this yet?
>
> David
> -----
>
> On 15/05/2013 5:50 PM, Peter Levart wrote:
>> Mike, David,
>>
>> Could you try this variant of the test:
>>
>> http://cr.openjdk.java.net/~plevart/jdk8-tl/String.contentEquals/webrev.05/ 
>>
>>
>>
>> I tried it on 3 different machines (4-core i7 Linux, 8-core sparc T-2
>> Solaris, 4-core amd64 Solaris) x 2 different JDK8 JVMs (64bit and 32bit)
>> and it fails immediately on all 6 of them.
>>
>> Regards, Peter
>>
>> On 05/14/2013 05:39 PM, Mike Duigou wrote:
>>> 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.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
>>>>>>>     -----
>>>>>>>
>>