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

[Nishit Jain]

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
>>