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