[9] RFR: 8130875 Out of memory when using TLS_RSA_WITH_AES_128_GCM_SHA256

[Valerie Peng]

Alright, I will update the synopsis. I am not exactly sure if it's GCM 
only as CipherFinal() call is used for all Ucrypto encryption/decryption.
The testcase is using AES/GCM, but I am not sure if the underlying 
native code which has the problem is GCM specific.

The using outLen's reference as the bufOut is from Solaris team. Since 
null output buffer is always used with 0 output length when java code 
calls the JNI code, it should be ok. However, let me see where I can add 
more checks.

Thanks,
Valerie

On 9/3/2015 7:20 AM, Anthony Scarpino wrote:
>> On Sep 2, 2015, at 10:50 PM, Anthony Scarpino<anthony.scarpino at oracle.com>  wrote:
>>
>>
>>> On Sep 2, 2015, at 3:45 PM, Valerie Peng<valerie.peng at oracle.com>  wrote:
>>>
>>>
>>> Can someone help review this java workaround for Solaris memory leak bug in Ucrypto library?
>>> Essentially, the memory leak occurs when a null output buffer is specified when doing encryption/decryption.
>>> So, the workaround in OracleUcrypto provider is to use non-null output buffers.
>>>
>>> Webrev: http://cr.openjdk.java.net/~valeriep/8130875/webrev.00/
>>>
>>> The fix is verified by running a program for a while and observe the memory usage.
>>> Valerie
>>
>> Not related to the code, I think the bug synopsis should be more specific to the issue.  It looks too eye-catching by saying java runs out of memory with that cipher suite, when it’s the OS library not cleaning up correctly for a particular provider when using AES GCM only.  I would not be surprised if a future issue got incorrectly linked because the synopsis was too generic.  Maybe something like “OracleUcrypto workaround for AES GCM with a null bufOut pointer during doFinal()"
>>
>> As for the code, I’m a bit unsure about using outLen’s reference as the bufOut pointer.  However, after seeing there are checks to make sure outLen is zero and it’s documented well that this is a workaround, I’m ok with this.
>>
>> Tony
> Thanks to Jamil for asking me about the webrev privately, I discounted a concern because I misread the webrev.
>
> So the comments are purely from the JNI code checks.  Maybe there are checks in the java code that prevent the below situations, but it’s still uneasy having no protection in the JNI code
>
> The change around 438, does not make sure outLen is zero, so if bufOut points to the reference of outLen, it could start overwriting data or even somewhere else with the offset also being unchecked.  The check should fail if outLen is not zero.  Before the change, having bufOut being null we could depend on the OS library to check, now giving it a false pointer, we should do more checking.
>
> Tony
>