RFR 8218228 : The constructor StringBuffer(CharSequence) violates spec for negatively sized argument

Ivan Gerasimov ivan.gerasimov at oracle.com
Wed Feb 13 01:21:16 UTC 2019


Uh, lots of paperwork, comparing to the actual change in the source code :-)

Anyways, I updated the CSR as you suggested, and created a release note 
subtask:
https://bugs.openjdk.java.net/browse/JDK-8218884

Could you please review it?

With kind regards,
Ivan

On 2/12/19 8:51 AM, Roger Riggs wrote:
> Hi Ivan,
>
> Sorry, one more.  The compatibility risk description should be 
> specific about
> the behavior that is changing.  Something like:
>
> Since JDK5, the exception thrown for negative lengths have been 
> unspecified
> and inconsistent; with this change, NegativeArraySizeException will be 
> thrown.
>
> Thanks, Roger
>
> p.s.  A release note capturing the essence of the change is probably 
> needed also.
> Add label release-note=yes to the issue and create a subtask.
>
>
> On 02/12/2019 11:39 AM, Ivan Gerasimov wrote:
>>
>>
>> On 2/12/19 7:33 AM, Roger Riggs wrote:
>>> Hi Ivan,
>>>
>>> The change to consistently throw NegativeSizeArrayException applies 
>>> to StringBuilder also.
>>> Please mention that with the solution.
>>>
>> Right, thanks.  I updated CSR accordingly.
>>
>> With kind regards,
>> Ivan
>>
>>> Thanks, Roger
>>>
>>>
>>> On 02/11/2019 06:12 PM, Ivan Gerasimov wrote:
>>>> Hi Roger!
>>>>
>>>>
>>>> On 2/11/19 7:30 AM, Roger Riggs wrote:
>>>>> Hi Ivan,
>>>>>
>>>>> I called it out because the CSR does not mention that the behavior
>>>>> of some of the cases (-1..-16) is changing and some of the emails 
>>>>> asserted
>>>>> there was no change in behavior.
>>>>>
>>>>> I'm fine with one changeset as long as both changes are explicit.
>>>>> The bug and the CSR should both be updated.
>>>>>
>>>> Yes, this makes sense.
>>>> I've updated CSR accordingly, and added a comment to the bug to 
>>>> make the intention of the fix clear.
>>>>
>>>> Could you please review the updated CSR?
>>>>
>>>> With kind regards,
>>>> Ivan
>>>>
>>>>
>>>>
>>>>> Thanks, Roger
>>>>>
>>>>>
>>>>> On 02/08/2019 05:12 PM, Ivan Gerasimov wrote:
>>>>>> Hi Roger!
>>>>>>
>>>>>> That's because two tiny changes are combined in the patch:
>>>>>>
>>>>>> 1) remove a problematic statement from the javadoc, as it doesn't 
>>>>>> hold anyway.  This part does not change the behavior.
>>>>>>
>>>>>> 2) unify type of the exception thrown for all negative values of 
>>>>>> CharSequence.length().  The regression test is to verify this, 
>>>>>> second change, so it fails prior the fix for length in [-16, -1].
>>>>>>
>>>>>> I can separate them into different bugs, if you think it will 
>>>>>> make it clearer.  Though I thought they can be fixed together.
>>>>>>
>>>>>> With kind regards,
>>>>>>
>>>>>> Ivan
>>>>>>
>>>>>>
>>>>>> On 2/8/19 1:49 PM, Roger Riggs wrote:
>>>>>>> Hi Ivan,
>>>>>>>
>>>>>>> In the direction of not changing the behavior; the webrev does 
>>>>>>> change the behavior.
>>>>>>>
>>>>>>> In the case of CharSeqence with length -1..-16, the new behavior 
>>>>>>> throws NegativeArrayIndexException
>>>>>>> instead of java.lang.IndexOutOfBoundsException.
>>>>>>>
>>>>>>>
>>>>>>> AbstractStringBuilder:101-102 throw an exception for length < 0.
>>>>>>> However, the current behavior is to create a StringBuffer with 
>>>>>>> length + 16 and
>>>>>>> then append the contents.  For -1..-16, it will succeed in 
>>>>>>> creating a StringBuffer
>>>>>>> but fail with IndexOutOfBoundsException during the append of the 
>>>>>>> contents.
>>>>>>>
>>>>>>> The new Test should pass both before and after the change to the 
>>>>>>> code.
>>>>>>>
>>>>>>> Roger
>>>>>>>
>>>>>>>
>>>>>>> On 02/07/2019 10:19 PM, Ivan Gerasimov wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2/7/19 6:33 PM, David Holmes wrote:
>>>>>>>>> On 8/02/2019 11:59 am, Ivan Gerasimov wrote:
>>>>>>>>>> Hi David!
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 2/7/19 5:16 PM, David Holmes wrote:
>>>>>>>>>>> Hi Ivan,
>>>>>>>>>>>
>>>>>>>>>>> On 8/02/2019 11:02 am, Ivan Gerasimov wrote:
>>>>>>>>>>>> Hello!
>>>>>>>>>>>>
>>>>>>>>>>>> The specification states:
>>>>>>>>>>>> """
>>>>>>>>>>>> If the length of the specified CharSequence is less than or 
>>>>>>>>>>>> equal to zero, then an empty buffer of capacity 16 is 
>>>>>>>>>>>> returned.
>>>>>>>>>>>> """
>>>>>>>>>>>>
>>>>>>>>>>>> However, the current implementation throws either 
>>>>>>>>>>>> NegativeArraySizeException or IndexOutOfBounds instead (the 
>>>>>>>>>>>> actual exception type depends on the value of reported 
>>>>>>>>>>>> negative length).
>>>>>>>>>>>
>>>>>>>>>>> How can you have a negative length CharSequence ??
>>>>>>>>>>>
>>>>>>>>>> A custom CharSequence returning negative length() can be 
>>>>>>>>>> created.
>>>>>>>>>> Not sure how useful/popular this may be, though.
>>>>>>>>>
>>>>>>>>> Seems pretty meaningless so just treating that as an error 
>>>>>>>>> seems fine. Though somewhat debatable whether you need to add 
>>>>>>>>> an appropriate @throws.
>>>>>>>>>
>>>>>>>> Right.  There were two reasons not to add @throws here:
>>>>>>>> - by removing the problematic paragraph we just make the 
>>>>>>>> javadoc almost the same as for StringBuilder(CharSequence),
>>>>>>>> - we don't seem to have any other places (at least I couldn't 
>>>>>>>> find any) specifying exceptions due to negatively sized 
>>>>>>>> CharSequence.
>>>>>>>>
>>>>>>>> (To be precise, there is one candidate, but I'll file a 
>>>>>>>> separate bug to fix it.)
>>>>>>>>
>>>>>>>>>> That's why I propose just removing the mentioning negative 
>>>>>>>>>> length, and not changing the behavior.
>>>>>>>>>>
>>>>>>>>>> The proposed code change is to only unify the behavior for 
>>>>>>>>>> any negative value of length.
>>>>>>>>>
>>>>>>>>> Ok.
>>>>>>>>>
>>>>>>>>>>> If its an empty CharSequence then it should return the empty 
>>>>>>>>>>> buffer as indicated.
>>>>>>>>>>>
>>>>>>>>>> Empty CharSequence is processed correctly already.
>>>>>>>>>
>>>>>>>>> Okay so by removing this part:
>>>>>>>>>
>>>>>>>>> -      * <p>
>>>>>>>>> -      * If the length of the specified {@code CharSequence} is
>>>>>>>>> -      * less than or equal to zero, then an empty buffer of 
>>>>>>>>> capacity
>>>>>>>>> -      * {@code 16} is returned.
>>>>>>>>>
>>>>>>>>> you're relying on the main specification to implicitly handle 
>>>>>>>>> the empty case.
>>>>>>>>>
>>>>>>>> Yes.
>>>>>>>>
>>>>>>>> With kind regards,
>>>>>>>> Ivan
>>>>>>>>
>>>>>>>>
>>>>>>>>> David
>>>>>>>>> -----
>>>>>>>>>
>>>>>>>>>> With kind regards,
>>>>>>>>>> Ivan
>>>>>>>>>>
>>>>>>>>>>> Cheers,
>>>>>>>>>>> David
>>>>>>>>>>>
>>>>>>>>>>>> It is proposed to do two things:
>>>>>>>>>>>> 1) remove the problematic sentence from the javadoc (CSR is 
>>>>>>>>>>>> filed).
>>>>>>>>>>>> 2) unify the exception type thrown, if the argument reports 
>>>>>>>>>>>> negative length.
>>>>>>>>>>>> NegativeArraySizeException will be consistent with, for 
>>>>>>>>>>>> example, StringBuffer(negativeCapacity).
>>>>>>>>>>>>
>>>>>>>>>>>> Would you please help review the fix?
>>>>>>>>>>>>
>>>>>>>>>>>> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8218228
>>>>>>>>>>>> WEBREV: 
>>>>>>>>>>>> http://cr.openjdk.java.net/~igerasim/8218228/00/webrev/
>>>>>>>>>>>> CRS: https://bugs.openjdk.java.net/browse/JDK-8218649
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

-- 
With kind regards,
Ivan Gerasimov



More information about the core-libs-dev mailing list