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

[Joe Wang]

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