Don't keep the store open in by_store_ctrl_ex #28198
Conversation
Previously openssl#27529 made a change to `by_store_ctrl_ex` in order to open the OSSL_STORE early. The reason given in that PR is: "This way, we can call OSSL_STORE_open_ex() in by_store_ctrl_ex(), and get to see possible errors when the URI is loaded" That PR then kept the store open until cache_objects is called and then reused it. Unfortunately by the time cache_objects() is called we could be in a multi-threaded scenario where the X509_STORE is being shared by multiple threads. We then get a race condition where multiple threads are all using (and ultimately closing) the same `OSSL_STORE_CTX`. The purpose of keeping the `OSSL_STORE` object between by_store_ctrl_ex() and `cache_objects` is presumably an optimisation to avoid having to open the store twice. But this does not work because of the above issue. We just take the hit and open it again. Fixes openssl#28171
|
I guess it also fixes #28065. |
|
I assume this will revert the performance improvement reported in #27529 (comment) though. Unfortunately I am afraid there is no way around that. Will you take it out of draft @mattcaswell ? |
|
Unfortunately, there's the risk that there are errors when reopening the URI in Mind you, I do understand the reasons for this PR, but it seems we're weighing one bad outcome against another. |
|
Can you clarify why cache_objects can't report errors in any sensible way? |
Sure. It goes back to #27461, which was about STORE URIs not being reported on when giving them (when So |
|
And there is no cache per location? |
|
Not sure I understand the question... |
That risk existed even in #27529. In #27529 it only reused the currently open store once. After that it reopened it again anyway (because it closed the store at the end of |
I think there is more work to be done. I'd like to confirm the performance impact (which I hope is negligible) - and I'd like to write a test. Unfortunately that won't happen until next week at the earliest. |
Oh, good point |
Previously #27529 made a change to
by_store_ctrl_exin order to open the OSSL_STORE early. The reason given in that PR is:"This way, we can call OSSL_STORE_open_ex() in by_store_ctrl_ex(), and get to see possible errors when the URI is loaded"
That PR then kept the store open until cache_objects is called and then reused it. Unfortunately by the time cache_objects() is called we could be in a multi-threaded scenario where the X509_STORE is being shared by multiple threads. We then get a race condition where multiple threads are all using (and ultimately closing) the same
OSSL_STORE_CTX.The purpose of keeping the
OSSL_STOREobject between by_store_ctrl_ex() andcache_objectsis presumably an optimisation to avoid having to open the store twice. But this does not work because of the above issue.We just take the hit and open it again.
Fixes #28171