RFR: 8230743 - StringJoiner does not handle too large strings correctly

[Paul Sandoz]

+1

Paul.

> On Jun 2, 2020, at 6:21 AM, Jim Laskey <james.laskey at oracle.com> wrote:
> 
> Revised with requested changes.
> 
> 
> http://cr.openjdk.java.net/~jlaskey/8230743/webrev-01 <http://cr.openjdk.java.net/~jlaskey/8230743/webrev-01>
> 
> 
>> On Jun 1, 2020, at 5:32 PM, Paul Sandoz <paul.sandoz at oracle.com> wrote:
>> 
>> 
>> 
>>> On Jun 1, 2020, at 12:44 PM, Jim Laskey <james.laskey at oracle.com <mailto:james.laskey at oracle.com>> wrote:
>>> 
>>> 
>>> 
>>>> On Jun 1, 2020, at 4:28 PM, Paul Sandoz <paul.sandoz at oracle.com <mailto:paul.sandoz at oracle.com>> wrote:
>>>> 
>>>> Can we consolidate the use of Integer.MAX_VALUE - 8 in tests by referring to the constant jdk.internal.util.ArraysSupport.MAX_ARRAY_LENGTH?
>>> 
>>> I suppose  it's possible to add module for java.base/jdk.internal.util.ArraysSupport to the tests.
>>> 
>>> The mission here was to provide useful error messages. If we're going to clean up growing arrays throughout the JDK, I feel that is a separate mission, probably not something that we should be doing just before RD1.
>>> 
>> 
>> Ok.
>> 
>> 
>>> We should probably have gone one step further with jdk.internal.util.ArraysSupport.newLength and just defined  jdk.internal.util.ArraysSupport.growArray(array, newSize, () -> { // exception code });
>> 
>>> 
>>>> 
>>>> 
>>>> StringJoiner.java
>>>> —
>>>> 
>>>> 164     @Override
>>>> 165     public String toString() {
>>>> 166         final String[] elts = this.elts;
>>>> 167         if (elts == null && emptyValue != null) {
>>>> 168             return emptyValue;
>>>> 169         }
>>>> 170         final int size = this.size;
>>>> 171         final int addLen = prefix.length() + suffix.length();
>>>> 172         if (len < 0 || len + addLen < 0) {
>>>> 
>>>> It bothers me that len might be < 0, suggesting a larger issue. Perhaps look at the add method where len is modified?
>>> 
>>> I thought about it but that would change the behaviour of StringJoiner. An error would be thrown on the add. Existing code that added but then tested length before trying a toString() would fail too soon.
>> 
>> Or fail at the right point? Lazy String construction by the joiner can be considered an implementation detail.
>> 
>> A negative len could result in some odd behavior:
>> 
>> - an incorrect value returned from length(), such as a negative length value.
>> 
>> - a number of adds could be performed, which one would have resulted in an OOME if the implementation was not lazy?
>> 
>> - (len + other.len < 0) could be positive and result in array construction exception for a negative length on other.compactElts.
>> 
>> Paul.
>> 
>>> 
>>>> 
>>>> 
>>>> 173             throw new
>>>> 174             OutOfMemoryError("Resulting string is exceeds maximum size”);
>>>> 
>>>> "Resulting string is exceeds… ” -> "Resulting string exceeds… "
>>> 
>>> Oops, thanks.
>>> 
>>> 
>>>> 
>>>> Paul.
>>>> 
>>>> 
>>>>> On Jun 1, 2020, at 8:55 AM, Jim Laskey <james.laskey at oracle.com> wrote:
>>>>> 
>>>>> Change NegativeArraySizeException to OutOfMemoryError. Tests added.
>>>>> 
>>>>> Cheers,
>>>>> 
>>>>> -- Jim
>>>>> 
>>>>> webrev: http://cr.openjdk.java.net/~jlaskey/8230743/webrev-00/index.html <http://cr.openjdk.java.net/~jlaskey/8230743/webrev-00/index.html>
>>>>> jbs: https://bugs.openjdk.java.net/browse/JDK-8230743 <https://bugs.openjdk.java.net/browse/JDK-8230743>
>