Hi Valerie,
Thanks for having a look at this proposal.
On 3/25/19 9:03 PM, Valerie Peng wrote:
Thanks for trying to address this "free-wrapper-key" issue. However,
changes in the key clean up/free area is difficult to verify and check
for issues and I am a little concerned that this new model may be hard
to trace when the key id is mutable and potentially fragile/error prone
for future changes/fixes.
I agree in that there is some complexity involved, but it is not much
different than the one we have dealt with before in this whole "memory
leak" fix.
My questions/feedbacks on current changes are:
1) line 1283: ref may be null (see line 1220) and this will lead to NPE?
For line 1283 to be executed, nativeKeyInfo has to be non-null (see line
1264). When nativeKeyInfo is non-null, a SessionKeyRef instance is
always assigned to a "ref" variable, and no execution path can set "ref"
back to null -we removed the previous "this.ref = this.ref.dispose()" line-.
This is not by-chance, this is because we now keep SessionKeyRef
instances always alive when there is native key info extracted. Previous
to this patch, a SessionKeyRef instance was alive only while there was a
native key created. The reason for this change, as I said, is that
SessionKeyRef instances now protect not only a created native key but a
wrapper key that may have been used to encrypt the extracted native key
information.
2) It looks like you don't really need to store the "wrapperKeyUsed"
value in both NativeKeyHolder and SessionKeyRef classes. It can be a
local variable for NativeKeyHolder class and the increment of
nativeKeyWrapperRefCount can be moved to SessionKeyRef constructor. The
counter nativeKeyWrapperRefCount is incremented inside NativeKeyHolder
constructor
Yes, I thought about that when coming to the proposed patch but was not
convinced for clarity and safety reasons. When a wrapper key does not
exist and has to be created, we must exit the creation block with the
reference counter incremented to protect the key. Otherwise, a race
condition is possible: while one thread creates the native key but still
does not increment the reference counter, a short-living object may
destroy the wrapper key. Moving the whole creation block to the
SessionKeyRef constructor is less clear to me -see below-.
3) I have not spent as much time as you on tracing this. However, I
think we should be able to keep the "final keyID" approach and handle
the NativeKeyWrapperRefCount inc/dec in SessionKeyRef
constructor/dispose methods. Have you tried this?
Keeping the "final keyID" approach is creating different SessionKeyRef
instances every time a native key is created. However, this approach
does not have a SessionKeyRef instance when there is extracted native
key information but there isn't a native key created. Now suppose that
this extracted native key information is protected by a wrapper key and
that the P11Key gets garbage-collected. There is no one to decrement the
reference counter and the wrapper key will be always alive.
4) It may not be enough to just run tests under sun/security/pkcs11, as
SunPKCS11 provider is used by many other JDK security components such as
TLS. To be safe, you should submit the test against jdk-tier1,jdk-tier2
to check for possible regressions. If you can't, please let me know.
I'm not exactly sure how can I do that but sounds good. If I cannot find
public information, I'll ask you for help.
Thanks,
Martin.-
