[13] RFR: 8210583: Base64.Encoder incorrectly throws NegativeArraySizeException

[Nishit Jain]

Hi Joe,

Executing it on Mach5 platforms took around 3s to 5s. Below is the link

https://java.se.oracle.com:10065/mdash/jobs/nishjain-jdk_04_09-20190121-0805-19402/results?search=status%3A*

Also executed using JMH on local linux-64 VM (6GB RAM), it took ~2200 ms 
in SingleShotTime mode

Benchmark               Mode  Cnt     Score      Error  Units
MyBenchmark.testMethod    ss   50  2200.663 ± 1826.348  ms/op

Regards,
Nishit Jain
On 19-01-2019 06:09, Joe Darcy wrote:
> How long does the regression test take to run on machines that have 
> enough memory?
>
> Thanks,
>
> -Joe
>
> On 1/18/2019 3:38 PM, naoto.sato at oracle.com wrote:
>> Looks good, Nishit.
>>
>> Naoto
>>
>> On 1/18/19 3:03 AM, Nishit Jain wrote:
>>> Hi Roger, Naoto,
>>>
>>>  > The size of Integer.MAX_VALUE - 2 is implementation specific,
>>> I think more typically max -8 is used to get the biggest allocation.
>>>
>>> When Integer.MAX_VALUE - 8 is used, the decode methods do not throw 
>>> the added OOME, because the computed value does not overflow 
>>> Integer.MAX_VALUE, so to test the added OOME condition need to use 
>>> Integer.MAX_VALUE - 2.
>>>
>>> Updated the webrev with rest of the suggestions
>>>
>>> http://cr.openjdk.java.net/~nishjain/8210583/webrev.04/
>>>
>>> Regards,
>>> Nishit Jain
>>> On 18-01-2019 02:44, Naoto Sato wrote:
>>>> I agree with 'withOutputParam' comment. It does seem to require 
>>>> some explanation. Same for the newly introduced return value -1.
>>>>
>>>> The test:
>>>>   46         // A separate output array is not required, as it is 
>>>> not used and
>>>>   47         // the exception is thrown beforehand while 
>>>> calculating the size
>>>>   48         // of the encoded bytes using input array
>>>>
>>>> This seems to make an assumption on the implementation. Test should 
>>>> not depend on the internal impl.
>>>>
>>>> Naoto
>>>>
>>>> On 1/17/19 8:14 AM, Roger Riggs wrote:
>>>>> Hi Nishit,
>>>>>
>>>>> In the test, there are a couple of RuntimeExceptions with messages 
>>>>> that start with "As expected"...
>>>>>
>>>>> That's counter intuitive, that a failure of the test is reported 
>>>>> with a message saying, its normal!
>>>>> I would use a message like:  "XXException should have been thrown..."
>>>>>
>>>>> The size of Integer.MAX_VALUE - 2 is implementation specific,
>>>>> I think more typically max -8 is used to get the biggest allocation.
>>>>>
>>>>> java/util/Base64.java:
>>>>>
>>>>> Lines 335-342:  This optimization is not mentioned in the bug 
>>>>> report and
>>>>> should be a separate review.
>>>>>
>>>>>
>>>>> 245, 686:  in outLength(), the 2nd parameter would be easier to 
>>>>> understand
>>>>> as 'throwOOME', meaning to throw OOM if length is too large.
>>>>> The 'withOutputParam' only has any meaning in the context of the 
>>>>> caller.
>>>>> And even for the private method write the javadoc describing the 
>>>>> behavior.
>>>>>
>>>>> It also makes the call sites clearer, the argument will be true to 
>>>>> throw.
>>>>>
>>>>> Thanks, Roger
>>>>>
>>>>>
>>>>> On 01/17/2019 02:07 AM, Nishit Jain wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Please review the fix for 8210583
>>>>>>
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8210583
>>>>>> Webrev: http://cr.openjdk.java.net/~nishjain/8210583/webrev.03/
>>>>>> CSR: https://bugs.openjdk.java.net/browse/JDK-8215633
>>>>>>
>>>>>> Issue: Base64.Encoder.encode and Base64.Decoder.decode methods 
>>>>>> incorrectly throw unspecified exception e.g. 
>>>>>> NegativeArraySizeException, when the input byte array size is too 
>>>>>> large such that the calculated output byte size goes beyond the 
>>>>>> max-integer boundary and wraps around.
>>>>>>
>>>>>> Fix: Throw an OutOfMemoryError if the output byte array/buffer or 
>>>>>> memory can not be allocated. There is an unrelated change in 
>>>>>> encodeToString(byte[]) where a string instance is created using 
>>>>>> JavaLangAccess.newStringNoRepl(byte[], ISO_8859_1)instead of 
>>>>>> string constructor, to save memory.
>>>>>>
>>>>>> Regards,
>>>>>> Nishit Jain
>>>>>
>>>