> 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
