On Tue, 6 Sep 2022 22:16:57 GMT, zzambers <d...@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
