RFR 6913047: SunPKCS11 memory leak

Martin Balao mbalao at redhat.com
Thu Oct 18 16:26:24 UTC 2018


Hi Valerie,

Thanks for your feedback.

I suggest to keep only one hierarchy of webrevs, so I'll continue in
Webrev.12 using your Webrev.02 as a base.

In general, I'm happy with your P11Key refactorings and with keeping the
previous reference inc/dec scheme on P11key clients side.

I'll go file-by-file reviewing changes between your Webrev.02 and my
Webrev.11:

 * P11Cipher.java
  * Ok
   * I used to set "initialized" variable to false in all "initialize"
function error flows but it's not necessary as it shouldn't be true when
entering.

 * P11DHKeyFactory.java
  * Copyright updated

 * P11DSAKeyFactory.java
  * Ok

 * P11Digest.java
  * Ok

 * P11ECDHKeyAgreement.java
  * Copyright updated

 * P11ECKeyFactory.java
  * Copyright updated

 * P11Key.java
  * Why is P11Key now public?
  * I have some concerns regarding how you calculate "extractKeyInfo" value:
   * Why is !sensitive a requirement?
    * We can extract them wrapped, and the code already handles that.
   * !tokenObject should be a requirement
    * I know it's checked in NativeKeyHolder constructor but it could be
part of extractKeyInfo too for clarity
   * Why is !type.equals(SECRET) a requirement?
  * It's correct that we can not destroy the key after creation (under the
assumption that will be used soon), and that this should be a performance
benefit for most cases. My approach here was more memory conservative, but
it should be fine anyways. I'm okay with this change.
  * In case of createNativeKey failure, I think we should decrement the
previously-incremented reference counter
  * Why do we need refCount to be of AtomicInteger type? All the
modifications to this instance variable are done inside instance
synchronized methods (getKeyID and releaseKeyID)
  * The benefit of the previous incNativeKeyRef/decNativeKeyRef methods is
that they don't pay a sync cost for all cases in which the feature is
disabled. I believe we can keep that performance improvement here too.
   * nativeKeyInfo variable is written at creation time and all other
usages are read-only, so we can have multiple readers at the same time and
synchronize on it if not null (instead of synchronizing the whole method).
What do you think?

 * P11KeyAgreement.java
  * Ok

 * P11KeyGenerator.java
  * Ok

 * P11KeyPairGenerator.java
  * Ok

 * P11KeyStore.java
  * Copyright updated
  * The reason why a call to "makeNativeKeyPersistent" was made in
P11KeyStore.updateP11Pkey method was because "P11Key key" could have been
extracted and the change not persisted in the keystore. This call was
disabling the feature for such keys. However, we now decided -for a good
reason- that keys from keystores (token objects) will not be extracted.
Thus, this call is no longer needed. I'm okay with this change.
  * "long newKeyID" variable not used. Fixed on some other files too.

 * P11Mac.java
  * Copyright updated
  * Split ensureInitialized and initialize functions again for the reasons
we've previously discussed.

 * P11RSACipher.java
  * Ok

 * P11RSAKeyFactory.java
  * Ok

 * P11SecretKeyFactory.java
  * Copyright updated
  * The inc/dec pattern was broken in case of "token.p11.C_CopyObject"
failure. Fixed.

 * P11Signature.java
  * Ok

 * P11TlsKeyMaterialGenerator.java
  * Removed dead & debug code

 * P11TlsMasterSecretGenerator.java
  * Ok

 * P11TlsPrfGenerator.java
  * Removed dead & debug code

 * P11TlsRsaPremasterSecretGenerator.java
  * Ok

 * PKCS11.java
  * Copyright updated

 * p11_keymgmt.c
  * Removed dead code
  * I've seen that you replaced some variable assignments with memcpy and
memset calls because of compiler errors. I wonder what these errors are, as
we should be able to cast and do assignments.

 * pkcs11t.h
  * Ok

 * pkcs11wrapper.h
  * Ok

In addition, I removed trimming white spaces in several files.

I agree with having a property to completely disable this feature. I've not
seen, so far, unfixed cases in which extraction succeeds but re-creation
fails.

Webrev.12 is based on your Webrev.02, with modifications previously
described:

 * http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.12/
 * http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.12.zip

NOTE: Webrev.12 does not address neither the P11Key.java issues above nor
the property to completely disable this feature. Those 2 things are the
only pending on my side for now.

Kind regards,
Martin.-
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20181018/49419871/attachment.htm>


More information about the security-dev mailing list