Thanks for the review~
Valerie
On 9/4/2015 8:20 AM, Anthony Scarpino wrote:
Ok. The changes looks good. Thanks for looking into it and changing the bug
synopsis.
Tony
On Sep 3, 2015, at 4:25 PM, Valerie Peng<valerie.p...@oracle.com> wrote:
For all the JavaCritical calls, it's impossible to have the combination of
(bufOut==NULL&& outLen!=0) as the outLen value is generated by VM based on
bufOut. Anyway, just to play it safe, I added a line to set outLen to 0 just to match
what is done for line 657.
http://cr.openjdk.java.net/~valeriep/8130875/webrev.01/
Thanks,
Valerie
On 9/3/2015 7:20 AM, Anthony Scarpino wrote:
On Sep 2, 2015, at 10:50 PM, Anthony Scarpino<anthony.scarp...@oracle.com>
wrote:
On Sep 2, 2015, at 3:45 PM, Valerie Peng<valerie.p...@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
