RFR: 8351443: Improve robustness of StringBuilder [v5]

[Roger Riggs]

On Tue, 6 May 2025 01:38:21 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Roger Riggs has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Refactor to consistently use `isLatin1(coder)` within AbstractStringBuilder.
>
> src/java.base/share/classes/java/lang/AbstractStringBuilder.java line 277:
> 
>> 275:                 // copy all bytes to new larger buffer
>> 276:                 value = Arrays.copyOf(value,
>> 277:                         newCapacity(value, newCoder, minimumCapacity) << newCoder);
> 
> Would this benefit with an `assert` to verify that the new buffer length being proposed is not lesser than the current buffer's length (thus truncating content). Something like:
> 
> 
> // copy all bytes to new larger buffer
> int newLen = newCapacity(value, newCoder, minimumCapacity) << newCoder;
> assert newLen >= value.length : "bad new length " + newLen + " for buffer's length " + value.length; 
> value = Arrays.copyOf(value, newLen);

I'd be concerned about bloating the code size for a check that will not be triggered.
`growth` is non-negative and `newCapacity()` uses the same comparison to determine its growth value.
If ArraySupport.newLength can't satisify the request, it will throw.

> src/java.base/share/classes/java/lang/AbstractStringBuilder.java line 309:
> 
>> 307:         if (minimumCapacity - oldCapacity > 0) {
>> 308:             value = Arrays.copyOf(value,
>> 309:                     newCapacity(value, coder, minimumCapacity) << coder);
> 
> Same question about whether we should assert that the new length we determine for the buffer is not lesser than the current buffer length.

ditto, will not return a shorter array, it will throw instead.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/24967#discussion_r2075667207
PR Review Comment: https://git.openjdk.org/jdk/pull/24967#discussion_r2075668360