[PATCH 0/1] MR6138: crypt32: When importing certificates to Root store set 'done' flag before entering semaphore.
CSP is loaded for each imported certificate, and if CSP accesses Root store it deadlocks. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6138
From: Dmitry Timoshkov <dmitry(a)baikal.ru> CSP is loaded for each imported certificate, and if CSP accesses Root store it deadlocks. Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/crypt32/rootstore.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/dlls/crypt32/rootstore.c b/dlls/crypt32/rootstore.c index cfddcc143ac..5c055193d4a 100644 --- a/dlls/crypt32/rootstore.c +++ b/dlls/crypt32/rootstore.c @@ -748,6 +748,8 @@ void CRYPT_ImportSystemRootCertsToReg(void) if (root_certs_imported) return; + root_certs_imported = TRUE; + hsem = CreateSemaphoreW( NULL, 0, 1, L"crypt32_root_semaphore"); if (!hsem) { @@ -787,7 +789,6 @@ done: RegCloseKey(key); CertCloseStore(store, 0); CertCloseStore(reg, 0); - root_certs_imported = TRUE; ReleaseSemaphore(hsem, 1, NULL); CloseHandle(hsem); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6138
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=147271 Your paranoid android. === debian11b (64 bit WoW report) === ddraw: ddraw4.c:3969: Test failed: Expected message 0x5, but didn't receive it.
Can I get a review for this patch, please? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6138#note_77299
Tbh it doesn't look right: if something needs to access root store it is supposed to wait for the import to finish. So if loading CSP load accesses root store during importing root certs this should probably be understood first how that is supposed to work and have some other way to solve it. Is it Wine's CSP or some custom one? As if it really needs to access root store it is not a matter of just a deadlock properly, it needs to be initialized before. If you could possibly elaborate on details a bit on how exactly that fails... -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6138#note_77301
It's a custom CSP, and yes, it deadlocks the way you're describing it. Is there anything else I should clarify? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6138#note_77304
Well, the fix looks wrong and a proper one is probably more complicated than that. If CSP needs rootstore it probably can't work in a general case when called during the same rootstore initialization even if that doesn't deadlock (maybe this CSP doesn't do anything with the rootstore but it probably makes more sense to fix the issue in general). Probably the correct way would be to avoid loading (unbeeded?) CSPs during root store init somehow, although I now didn't look in details. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6138#note_77305
In Windows this CSP doesn't deadlock, so I'd guess that it shouldn't deadlock in Wine either. It doesn't matter that the root store isn't initialized at that point, that's fine even if it would be empty. What is important is a deadlock, and this patch does fix it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6138#note_77306
I think the fact that a patch fixes one use case was never considered enough justification for the patch correctness. To clarify a bit, I think in the current form the patch will break things for apps without any custom CSP present. Imagine an app accessing root store from the two threads, one is in process of initializing that root store. The other thread should wait for it to finish before going forward and get correctly populated store. The latter could be technically solved by, e. g., detecting the recursion here and exiting the function only in this specific case. This at least would not obviously break existing functionality, but I'd think it is a bit ugly and doesn't solve the core problem. I guess it needs some work to evaluate why do we have to load those CSP providers at all during cert store population and whether we can sanely load only those needed (unless anyone knows that offhand). I am not bringing up the question about tests here even because it looks rather obvious that this CRYPT_ImportSystemRootCertsToReg() is a Wine-specific thing and on Windows the root store is not populated on the first use. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6138#note_77308
I'd guess that the CSP is probably loaded to verify a certificate's signature. 'done' flag is also set to TRUE on every failure case in that code path, so an argument that something may go wrong is the root store isn't initialized doesn't sound too convincing IMHO. Perhaps using a critical section instead of a semaphore would solve the scenario you've described, and for the deadlock we should check the recursion count of a critical section. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6138#note_77309
Technically for loading host root store we don't need any custom PE CSPs. It might happen we'd need even custom CSPs for the CRYPT_RegReadSerializedFromReg() part but not immediately sure if we do. In case we don't it is interesting where exactly we end loading those custom CSPs and maybe we can stop doing that. If we do need those custom CSPs for some certs serialized in registry this is getting complicated, can't be theorizing on this without some deeper look. Regarding the solution solving exactly the deadlock avoiding recursion. Named semaphore is there for reasons, it syncs things cross process. Adding a critical section on top is weird and also I'd advise against using sync object internals. This could probably be done by using CreateMutex instead of semaphore (those mutices are re-entrant), and then adding a var with thread id which took the lock, to check that after taking mutex so if this is the same thread just release the mutex and exit the function. With this ad-hoc solution I won't at least be whining that it is going to break everything, but I still don't see why wouldn't we explore a proper way at least. Perhaps we might listen to another opinion, if @hans could comment if a simplified solution for this specific usecase looks reasonable to him. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6138#note_77310
participants (4)
-
Dmitry Timoshkov -
Dmitry Timoshkov (＠dmitry)
-
Marvin -
Paul Gofman (＠gofman)