RFR: 8293232: Fix race condition in pkcs11 SessionManager

[Valerie Peng]

On Tue, 6 Sep 2022 22:16:57 GMT, zzambers <duke at openjdk.org> wrote:

>> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SessionManager.java line 210:
>> 
>>> 208:             return;
>>> 209:         }
>>> 210:         releaseSession(session);
>> 
>> With the described race condition, have you tried fixing it by adding a if-condition check before doing line 204-210, i.e. if (!session.hasObjects()) { .... }?
>
> I am afraid putting check before line 204 would not solve the issue (just lowered it's likelihood). Problem is, that operation consisting of check for objects on a session and then removing it from objSessions pool is not atomic. Session still could be obtained from objSessions pool by other thread after session.hasObjects() was called, object added to it and released back to objSessions pool before objSessions.remove(session) is called. I think this check for objects can only be trusted after session was successfully removed from objSessions (that is, session was in objSessions pool (no tread "holds" it) and was removed).
> 
> Actually whole call of demoteObjSession method is already behind one check for zero objects (but that check cannot be trusted), and needs to be redone after objSessions.remove(session),  because of problem described higher . See:
> https://github.com/openjdk/jdk/blob/9444a081cc9873caa7b5c6a78df0d1aecda6e4f1/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/Session.java#L100

I am aware of the zero objects check in the reference above. 
I am fine with the proposed fix then. Perhaps add a comment to this releaseSession() call to warn about this race condition.

-------------

PR: https://git.openjdk.org/jdk/pull/10125