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