RFR (JDK11) 8137326: Methods for comparing CharSequence, StringBuilder, and StringBuffer

[Xueming Shen]

+1

On 2/27/18, 11:37 AM, Joe Wang wrote:
> Hi Sherman and all,
>
> Thanks for the further reviews!
>
> Here's the latest webrev with boundary checks in 
> StringLatin1/StringUTF16:
>
> JBS: https://bugs.openjdk.java.net/browse/JDK-8137326
> webrev: http://cr.openjdk.java.net/~joehw/jdk11/8137326/webrev/
>
> Best,
> Joe
>
> On 2/22/2018 6:07 PM, Joe Wang wrote:
>>
>>
>> On 2/22/18, 12:51 PM, Xueming Shen wrote:
>>> On 2/22/18, 12:04 PM, Joe Wang wrote:
>>>> Hi Sherman,
>>>>
>>>> Thanks for reviewing the change.
>>>>
>>>> Taking a local copy of the count field, but the boundary check 
>>>> would be almost immediately done against the field itself. Are you 
>>>> worrying about the count field may be out of sync with the byte 
>>>> array? I would think that's unlikely to happen. Whether it's 
>>>> StringBuilder or StringBuffer, it's not advisable/practical to use 
>>>> in multiple threads. In a valid usage, the count is always 
>>>> consistent with the byte array.
>>>>
>>>
>>> Hi Joe,
>>>
>>> It might not be a "valid usage" but it did happen and when it 
>>> happens it might just crash the
>>> vm without those boundary checks. It's especially true for those 
>>> intrinsics methods with explicit
>>> comments "intrinsic performs no bounds checks". In this case, the 
>>> StringUTF16.getChar() is being
>>> called in new public method StringUTF16.compareTo(byte[], byte[], 
>>> int, int) without appropriate
>>> boundary check. In the "old" code the "index" is guaranteed to be 
>>> within [0, len) in
>>> StringUTF16.compareTo(byte[], byte[]), so it's safe. A real case for 
>>> such scenario can be found in
>>> JDK-8158168 [1], for example.
>>
>> Thanks for the pointer! The email thread helps a lot. I've updated 
>> the webrev with a boundary check in ASB (AbstractStringBuilder line 
>> 106, 107), and then a note to StringUTF16.compareTo (StringUTF16 line 
>> 280). Hopefully this is sufficient. Didn't want to add any check in 
>> StringUTF16 since that may affect the original two-arg method.
>>
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8137326
>> webrev: http://cr.openjdk.java.net/~joehw/jdk11/8137326/webrev/
>>
>> -Joe
>>
>>>
>>> -Sherman
>>>
>>>
>>> [1] https://bugs.openjdk.java.net/browse/JDK-8158168
>