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

[Roger Riggs]

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