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

[Nishit Jain]

Thanks Roger,

Updated.
http://cr.openjdk.java.net/~nishjain/8210583/webrev.05/

Regards,
Nishit Jain
On 18-01-2019 21:13, Roger Riggs wrote:
> Hi Nishit,
>
> Looks good, with a minor fix.
>
> ok, the rationale for MAX_VALUE-2 make sense.
>
> TestEncodingDecodingLength: Line 61 and 68,
>   The error message will be more readable with a ": " before the 
> methodname is appended.
>
> Thanks, Roger
>
>
> On 01/18/2019 06: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
>>>>
>>
>