CRYPT_ImportSystemRootCertsToReg calls sync_trusted_roots_from_known_locations which calls check_and_store_certs. check_and_store_certs creates a chain engine with `cached`. Here the problem is that the chain engine actually owns the store used to create it. And when later the chain engine is freed, the store is closed too.
This means on the success path `cached` is already closed when sync_trusted_roots_from_known_locations returns to CRYPT_ImportSystemRootCertsToReg, but CRYPT_ImportSystemRootCertsToReg tries to close `cached` again.
From: Yuxuan Shui yshui@codeweavers.com
CRYPT_ImportSystemRootCertsToReg calls sync_trusted_roots_from_known_locations which calls check_and_store_certs. check_and_store_certs creates a chain engine with `cached`. Here the problem is that the chain engine actually owns the store used to create it. And when later the chain engine is freed, the store is closed too.
This means on the success path `cached` is already closed when sync_trusted_roots_from_known_locations returns to CRYPT_ImportSystemRootCertsToReg, but CRYPT_ImportSystemRootCertsToReg tries to close `cached` again. --- dlls/crypt32/rootstore.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/dlls/crypt32/rootstore.c b/dlls/crypt32/rootstore.c index 3aca97aaab9..9112d93551a 100644 --- a/dlls/crypt32/rootstore.c +++ b/dlls/crypt32/rootstore.c @@ -118,6 +118,7 @@ static void mark_cert_imported( HKEY import_key, const CERT_CONTEXT *cert ) RegSetValueExW( import_key, hash_str, 0, REG_DWORD, (BYTE *)&value, sizeof(value) ); }
+/* Takes ownership of `cached`. */ static void check_and_store_certs( HCERTSTORE cached, HKEY key, HKEY import_key ) { DWORD root_count = 0; @@ -128,7 +129,12 @@ static void check_and_store_certs( HCERTSTORE cached, HKEY key, HKEY import_key TRACE("\n");
if (!(engine = CRYPT_CreateChainEngine( cached, CERT_SYSTEM_STORE_CURRENT_USER, &chainEngineConfig ))) + { + CertCloseStore( cached, 0 ); return; + } + + /* `engine` owns `cached`. */
while ((cert = CertEnumCertificatesInStore( cached, cert ))) { @@ -645,6 +651,8 @@ static void add_ms_root_certs(HKEY key, HCERTSTORE cached) * any location contains any certificates, to prevent spending unnecessary time * adding redundant certificates, e.g. when both a certificate bundle and * individual certificates exist in the same directory. + * + * This function takes ownership of `cached`. */ static void sync_trusted_roots_from_known_locations( HKEY key, HCERTSTORE cached ) { @@ -668,7 +676,10 @@ static void sync_trusted_roots_from_known_locations( HKEY key, HCERTSTORE cached { if (RegCreateKeyExW( HKEY_LOCAL_MACHINE, L"Software\Wine\HostImportedCertificates_tmp", 0, NULL, 0, KEY_ALL_ACCESS, NULL, &import_key, NULL )) + { + CertCloseStore( cached, 0 ); return; + }
/* If the key is absent existing certificates were added by an older Wine version, mark all cached certificates * as imported (as it was previously assumed) so they can be deleted when are deleted on host. */ @@ -680,6 +691,7 @@ static void sync_trusted_roots_from_known_locations( HKEY key, HCERTSTORE cached { ERR( "Error renaming key %#lx.\n", value ); RegCloseKey( import_key ); + CertCloseStore( cached, 0 ); return; } } @@ -750,6 +762,8 @@ static void sync_trusted_roots_from_known_locations( HKEY key, HCERTSTORE cached
if (new_count) check_and_store_certs( cached, key, import_key ); + else + CertCloseStore( cached, 0 );
RegCloseKey( import_key ); TRACE( "existing %u, deleted %u, new %u.\n", existing_count, deleted_count, new_count ); @@ -793,6 +807,7 @@ void CRYPT_ImportSystemRootCertsToReg(void) CRYPT_RegReadSerializedFromReg(key, CERT_STORE_CERTIFICATE_CONTEXT_FLAG, cached, CERT_STORE_ADD_ALWAYS); add_ms_root_certs(key, cached); sync_trusted_roots_from_known_locations(key, cached); + cached = NULL;
done: RegCloseKey(key);
Can’t instead ‘cached’ be closed just one at caller as it is now, and a special case when it gets closed earlier fixed instead? I am sure there should be a better way, tracking “ownership” through all those functions doesn’t look nice.
On Fri May 30 22:09:13 2025 +0000, Paul Gofman wrote:
Can’t instead ‘cached’ be closed just one at caller as it is now, and a special case when it gets closed earlier fixed instead? I am sure there should be a better way, tracking “ownership” through all those functions doesn’t look nice.
i think this makes reasoning easier, since `cached` passed to `sync_trusted_roots_from_known_locations` is freed no matter what.
the other option is returning a BOOL from `sync_trusted_roots_from_known_locations` and `check_and_store_certs` indicate whether they succeeded, and only free `cached` if not.
On Fri May 30 22:09:13 2025 +0000, Yuxuan Shui wrote:
i think this makes reasoning easier, since `cached` passed to `sync_trusted_roots_from_known_locations` is freed no matter what. the other option is returning a BOOL from `sync_trusted_roots_from_known_locations` and `check_and_store_certs` indicate whether they succeeded, and only free `cached` if not.
Either way introduces some mind breaking reference tracking which is best to avoid. I didn’t have a chance to look in details yet but there must be a better way, I don’t know, add a reference to cached when needed or change something in chain engine so it works in a more straightforward way.
On Fri May 30 23:13:10 2025 +0000, Paul Gofman wrote:
Either way introduces some mind breaking reference tracking which is best to avoid. I didn’t have a chance to look in details yet but there must be a better way, I don’t know, add a reference to cached when needed or change something in chain engine so it works in a more straightforward way.
The rule of thumb is if a function which doesn’t persist an object anywhere takes object on input it should keep its reference count the same on exit, whether it failed or succeeded.
On Fri May 30 23:17:05 2025 +0000, Paul Gofman wrote:
The rule of thumb is if a function which doesn’t persist an object anywhere takes object on input it should keep its reference count the same on exit, whether it failed or succeeded.
If it indeed turns out clumsy either way (not sure that it is actually the case) maybe it would be more interesting to get rid of ‘cached’ entirely. The need for that IIRC was initially stipulated by how registry storage worked at times, but there were some changes since and also we might want to change registry storage so it persists the changes immediately and not after release (that’s how that works on Windows). Then we maybe don’t need ‘cached’ at all and can work solely with registry storage to do the same.
On Fri May 30 23:35:59 2025 +0000, Paul Gofman wrote:
If it indeed turns out clumsy either way (not sure that it is actually the case) maybe it would be more interesting to get rid of ‘cached’ entirely. The need for that IIRC was initially stipulated by how registry storage worked at times, but there were some changes since and also we might want to change registry storage so it persists the changes immediately and not after release (that’s how that works on Windows). Then we maybe don’t need ‘cached’ at all and can work solely with registry storage to do the same.
Probably just calling CertDuplicateStore() for 'cached' when 'cached' is used with chain engine should do it? Note that CertDuplicateStore() just increases its refcount, it doesn't do an actual copy.
On Mon Jun 2 15:00:21 2025 +0000, Paul Gofman wrote:
Probably just calling CertDuplicateStore() for 'cached' when 'cached' is used with chain engine should do it? Note that CertDuplicateStore() just increases its refcount, it doesn't do an actual copy.
Actually, even better would be to just CertDuplicateStore in CRYPT_CreateChainEngine() if 'root' is not NULL, that would be normal idiomatic behaviour for this function. Also, it looks like CRYPT_CreateChainEngine() is only used with non-NULL root from this place in rootstore.c, other usages just pass NULL there.
On Mon Jun 2 15:13:56 2025 +0000, Paul Gofman wrote:
Actually, even better would be to just CertDuplicateStore in CRYPT_CreateChainEngine() if 'root' is not NULL, that would be normal idiomatic behaviour for this function. Also, it looks like CRYPT_CreateChainEngine() is only used with non-NULL root from this place in rootstore.c, other usages just pass NULL there.
Between the lines of this I sent https://gitlab.winehq.org/wine/wine/-/merge_requests/8199 .
On Tue Jun 3 23:52:52 2025 +0000, Paul Gofman wrote:
Between the lines of this I sent https://gitlab.winehq.org/wine/wine/-/merge_requests/8199 .
Looks good! Thank you so much.
I will close this.
Superseded by !8199
This merge request was closed by Yuxuan Shui.
-
Paul Gofman (＠gofman) -
Yuxuan Shui
-
Yuxuan Shui (＠yshui)