<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p>Hi Martin,</p>
<p>I am ok with your conservation choice of only applying this when
using NSS. If we are only applying this for NSS, we should really
refactor the code to minimize the impact on callers and P11Key
class. My prototype code may be on the extreme end of minimizing
changes. But the current webrev can use some refactoring also.
With your explanation, I now understand your model better. How
about the refactoring in P11Key class? Is there a reason for not
doing this? I did test my prototype code against existing
regression tests (except the KeyStore ones as more API changes are
needed for persistent keys which I have not covered in prototype)
but I ran into some strange errors in some native p11 calls <span
class="new">which I did not touch so I commented them out and
just checked the part of reference count, etc.<br>
</span></p>
I will take a closer look at the KeyStore case and let you know.<br>
Thanks,<br>
Valerie<br>
<br>
<div class="moz-cite-prefix">On 9/18/2018 7:29 AM, Martin Balao
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAKZz+gd8ARwjKd0Pu6tpXXCNStuTY8nc5ObmXEYKqawcezkwDQ@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=utf-8">
<div dir="ltr">
<div dir="ltr">
<div>Hi Valerie,</div>
<div><br>
</div>
<div>Thanks for your comments.</div>
<div><br>
</div>
<div>Here it is Webrev.11:</div>
<div><br>
</div>
<div> * <a
href="http://cr.openjdk.java.net/%7Embalao/webrevs/6913047/6913047.webrev.11/"
moz-do-not-send="true">http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.11/</a></div>
<div> * <a
href="http://cr.openjdk.java.net/%7Embalao/webrevs/6913047/6913047.webrev.11.zip"
moz-do-not-send="true">http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.11.zip</a></div>
<div><br>
</div>
<div><src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java></div>
<div>L397: That's right. I was trying to simplify the code but
missed this. Thanks.</div>
<div>L471: The key reference counter has to be decremented
under any exception (P11Key.decNativeKeyRef method call).
But, yes, no exception different than PKCS11Exception should
be thrown. Reverted this change.</div>
<div><br>
</div>
<div><src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Key.java></div>
<div>L99: Comment changed. It should be better now.</div>
<div>L148-L149: In fact, I'd enforce this and disable the
feature for all token keys. Token keys are permanent and
extracting them is risky. This criteria was already applied
when dealing with key stores (P11Keystore class).</div>
<div><br>
</div>
<div>Yes, this feature is enabled for NSS only because it's
the only backend we currently know that is affected by this
memory "leak" issue. If there were any other software-token
backend affected, we can try this feature there too. HSMs
shouldn't have any problem. I prefer to take a more
conservative approach and enable the feature only in those
cases in which it's really necessary. All other cases,
default to the previous mechanism for freeing memory.</div>
<div><br>
</div>
<div>This does not replace the PhantomReference approach; both
work together and are complementary. In cases where
temporary keys feature is disabled or when a temporary key
client is not behaving correctly (i.e.: leaking stateful
operations like "cipher" or "signature" in an intermediate
state with the native key initialized), PhantomReference
approach will be the last chance to free memory. The native
key object can be destroyed (C_DestroyObject call) either
from the PhantomReference mechanism or from the temporary
keys mechanism. There shouldn't be any conflict between
them. If it's destroyed through temporary keys mechanism,
then we know that the P11Key object is alive (refereced) and
thus PhantomReference destruction won't be taking place at
the same time. Once the key is deleted, keyID is set to 0
and session to null. Thus, PhantomReference destruction
won't have any effect when executed later. If we think of
the other case (when the key is freed by PhantomReference),
we have a P11Key object with a native key initialized but
with no references to it. Thus, destroyNativeKey method
won't be called and SessionKeyRef.disposeNative is the only
method that will delete the key.</div>
<div><br>
</div>
<div>L157: that's right, synchronization has to be at class
level. Fixed.</div>
<div><br>
</div>
<div>L1343: It's not the same session: this.session was
assigned a new value (this.session = session;) before
calling addObject.</div>
<div><br>
</div>
<div>L1363: removeObject is called for the session, inside
setKeyIDAndSession: "this.session.removeObject();". Null is
set to this.session instance variable after this call.</div>
<div><br>
</div>
<div>In regards to the refactorings you proposed, the problem
I see with moving key reference incrementing/decrementing to
PKCS11.java is that some operations are stateful. I.e.:
encryption. When we initialize the operation with
C_EncryptInit, the key id is the 3rd parameter. Destroying
the key id and then doing C_EncryptUpdate sounds incorrect
to me. Have you tried the regression testing suite after
this refactoring? (I see some parts commented). In regards
to removing the tmpNativeKey parameter (used to explicitly
disable the feature for new P11Key objects), how do you
handle the P11KeyStore case? We don't want temporary keys
there.</div>
<div><br>
</div>
<div>Kind regards,</div>
<div>Martin.-</div>
<div><br>
</div>
</div>
</div>
</blockquote>
<br>
</body>
</html>