Fix several race conditions #2735
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
Author
|
@dengert , could you check whether ignoring the result of card_reader_lock_obtained in sc_lock is OK for you? |
frankmorgner
force-pushed
the
hardware-error
branch
from
c727d6e to
adf8927
Compare
card->mutex is used to protect card->lock_count, so let's limit the scope of the mutex to where the lock_count is used. First and foremost, this fixes OpenSC#2707, which is caused by PIV's failing card_reader_lock_obtained, which would have caused sc_lock to change lock_count while *not* locking the mutex, which creates a race condition with other threads using and modifying lock_count. Secondly, this fixes a possible dead lock when sc_lock is used inside the card's card->sm_ctx.ops.open, which previously would be called in sc_lock when the mutex was locked.
frankmorgner
force-pushed
the
hardware-error
branch
from
adf8927 to
6d4e1bc
Compare
Member
Author
|
I've extended this PR to allow the |
Member
Author
|
with the last couple of changes, no more race conditions were found by helgrind 🥳 |
Jakuje
reviewed
Mar 28, 2023
Threads can now run the same tests as if pkcs11-tool was started with `--test`, e.g. ``` pkcs11-tool --use-locking --pin=123456 --test-threads LT --test-threads LT ```
frankmorgner
force-pushed
the
hardware-error
branch
from
c587adf to
da5e721
Compare
Jakuje
approved these changes
Mar 29, 2023
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#2707 was haunting me for months now and I was finally able to track the problem down to a race condition in sc_lock().
On my way, I learned about valgrind's Helgrind, which may be used like this:
This still shows some problem in pkcs11-tool, which I am ignoring for now.
Unfortunately, running Firefox with helgrind is not possible, at least on my machine, which caused me a lot of headache while finally fixing the real problem. I think it would be very useful if we could extend pkcs11-tool's thread tests with the tests we already have in place by using
--test --pin=123456. Currently--test-threadsonly calls C_Initialize (IL), C_GetInfo (GI) or C_GetSlotList (SL).Checklist