[PATCH 0/5] MR7193: crypt32: A refcount fix and cert enumeraton improvement.
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/crypt32/collectionstore.c | 2 +- dlls/crypt32/crypt32_private.h | 2 ++ dlls/crypt32/rootstore.c | 1 - dlls/crypt32/store.c | 14 ++++++++++++-- dlls/crypt32/tests/cert.c | 6 ++---- dlls/crypt32/tests/store.c | 20 +++++++++----------- dlls/cryptui/main.c | 4 +++- 7 files changed, 29 insertions(+), 20 deletions(-) diff --git a/dlls/crypt32/collectionstore.c b/dlls/crypt32/collectionstore.c index 0193be86222..d11ec89ecdf 100644 --- a/dlls/crypt32/collectionstore.c +++ b/dlls/crypt32/collectionstore.c @@ -265,7 +265,7 @@ static BOOL Collection_deleteCert(WINECRYPT_CERTSTORE *store, context_t *context TRACE("(%p, %p)\n", store, cert); linked = (cert_t*)context->linked; - return CertDeleteCertificateFromStore(&linked->ctx); + return CRYPT_DeleteCertificateFromStore(&linked->ctx); } static BOOL Collection_addCRL(WINECRYPT_CERTSTORE *store, context_t *crl, diff --git a/dlls/crypt32/crypt32_private.h b/dlls/crypt32/crypt32_private.h index a5aeae63168..9ac5b4a3067 100644 --- a/dlls/crypt32/crypt32_private.h +++ b/dlls/crypt32/crypt32_private.h @@ -417,6 +417,8 @@ context_t *Context_CreateDataContext(size_t contextSize, const context_vtbl_t *v */ context_t *Context_CreateLinkContext(unsigned contextSize, context_t *linked, struct WINE_CRYPTCERTSTORE*); +BOOL CRYPT_DeleteCertificateFromStore(PCCERT_CONTEXT pCertContext); + /* Copies properties from fromContext to toContext. */ void Context_CopyProperties(const void *to, const void *from); diff --git a/dlls/crypt32/rootstore.c b/dlls/crypt32/rootstore.c index 4c215d44943..3aca97aaab9 100644 --- a/dlls/crypt32/rootstore.c +++ b/dlls/crypt32/rootstore.c @@ -745,7 +745,6 @@ static void sync_trusted_roots_from_known_locations( HKEY key, HCERTSTORE cached /* Delete from cached so deleted certs do not participate in chain verification. */ CertDeleteCertificateFromStore( cert ); /* Restart enumeration as it is broken by deleting cert from store. */ - CertFreeCertificateContext( cert ); cert = NULL; } diff --git a/dlls/crypt32/store.c b/dlls/crypt32/store.c index de2d760d2b0..f41aad2c3ff 100644 --- a/dlls/crypt32/store.c +++ b/dlls/crypt32/store.c @@ -934,10 +934,9 @@ PCCERT_CONTEXT WINAPI CertEnumCertificatesInStore(HCERTSTORE hCertStore, PCCERT_ return ret ? &ret->ctx : NULL; } -BOOL WINAPI CertDeleteCertificateFromStore(PCCERT_CONTEXT pCertContext) +BOOL CRYPT_DeleteCertificateFromStore(PCCERT_CONTEXT pCertContext) { WINECRYPT_CERTSTORE *hcs; - TRACE("(%p)\n", pCertContext); if (!pCertContext) @@ -951,6 +950,17 @@ BOOL WINAPI CertDeleteCertificateFromStore(PCCERT_CONTEXT pCertContext) return hcs->vtbl->certs.delete(hcs, &cert_from_ptr(pCertContext)->base); } +BOOL WINAPI CertDeleteCertificateFromStore(PCCERT_CONTEXT pCertContext) +{ + BOOL ret; + + TRACE("(%p)\n", pCertContext); + + ret = CRYPT_DeleteCertificateFromStore(pCertContext); + CertFreeCertificateContext(pCertContext); + return ret; +} + BOOL WINAPI CertAddCRLContextToStore(HCERTSTORE hCertStore, PCCRL_CONTEXT pCrlContext, DWORD dwAddDisposition, PCCRL_CONTEXT* ppStoreContext) diff --git a/dlls/crypt32/tests/cert.c b/dlls/crypt32/tests/cert.c index c56578f1e21..396f6a2e390 100644 --- a/dlls/crypt32/tests/cert.c +++ b/dlls/crypt32/tests/cert.c @@ -4416,9 +4416,8 @@ static void testKeyProvInfo(void) ret = CertDeleteCertificateFromStore(cert); ok(ret, "CertDeleteCertificateFromStore error %#lx\n", GetLastError()); - - CertFreeCertificateContext(cert); - CertCloseStore(store, 0); + ret = CertCloseStore(store, CERT_CLOSE_STORE_CHECK_FLAG); + ok(ret, "got error %#lx.\n", GetLastError()); } static void test_VerifySignature(void) @@ -4518,7 +4517,6 @@ START_TEST(cert) testGetIssuerCert(); testLinkCert(); testKeyProvInfo(); - testCryptHashCert(); testCryptHashCert2(); testCertSigs(); diff --git a/dlls/crypt32/tests/store.c b/dlls/crypt32/tests/store.c index 8809c5048b4..48f06c54f7c 100644 --- a/dlls/crypt32/tests/store.c +++ b/dlls/crypt32/tests/store.c @@ -269,10 +269,10 @@ static void testMemStore(void) /* close an empty store */ ret = CertCloseStore(NULL, 0); ok(ret, "CertCloseStore failed: %ld\n", GetLastError()); - ret = CertCloseStore(store1, 0); - ok(ret, "CertCloseStore failed: %ld\n", GetLastError()); - ret = CertCloseStore(store2, 0); - ok(ret, "CertCloseStore failed: %ld\n", GetLastError()); + ret = CertCloseStore(store1, CERT_CLOSE_STORE_CHECK_FLAG); + ok(ret, "got error %#lx.\n", GetLastError()); + ret = CertCloseStore(store2, CERT_CLOSE_STORE_CHECK_FLAG); + ok(ret, "got error %#lx.\n", GetLastError()); /* This seems nonsensical, but you can open a read-only mem store, only * it isn't read-only @@ -447,8 +447,8 @@ static void testRegStoreSavedCerts(void) ok (ret, "Failed to delete certificate from store at %ld, %lx\n", i, GetLastError()); CertFreeCertificateContext(cert1); - CertFreeCertificateContext(cert2); - CertCloseStore(store, 0); + ret = CertCloseStore(store, CERT_CLOSE_STORE_CHECK_FLAG); + ok(ret, "got error %#lx.\n", GetLastError()); res = RegOpenKeyExW(reg_store_saved_certs[i].key, key_name, 0, KEY_ALL_ACCESS, &key); ok (res, "The cert's registry entry should be absent at %li, %lx\n", i, GetLastError()); @@ -2754,7 +2754,7 @@ static void testEmptyStore(void) res = CertAddCertificateContextToStore(cert->hCertStore, cert2, CERT_STORE_ADD_NEW, &cert3); ok(res, "CertAddCertificateContextToStore failed\n"); - todo_wine + todo_wine_if(cert3) ok(cert3 && cert3 != cert2, "Unexpected cert3\n"); ok(cert3->hCertStore == cert->hCertStore, "Unexpected hCertStore\n"); @@ -2764,8 +2764,6 @@ static void testEmptyStore(void) ok(res, "CertDeleteCertificateContextFromStore failed\n"); ok(cert3->hCertStore == cert->hCertStore, "Unexpected hCertStore\n"); - CertFreeCertificateContext(cert3); - store = CertOpenStore(CERT_STORE_PROV_MEMORY, 0, 0, CERT_STORE_CREATE_NEW_FLAG, NULL); ok(store != NULL, "CertOpenStore failed\n"); @@ -2778,8 +2776,8 @@ static void testEmptyStore(void) ok(res, "CertDeleteCertificateContextFromStore failed\n"); ok(cert3->hCertStore == store, "Unexpected hCertStore\n"); - CertCloseStore(store, 0); - CertFreeCertificateContext(cert3); + res = CertCloseStore(store, CERT_CLOSE_STORE_CHECK_FLAG); + ok(res, "got error %#lx.\n", GetLastError()); res = CertCloseStore(cert->hCertStore, CERT_CLOSE_STORE_CHECK_FLAG); ok(!res && GetLastError() == E_UNEXPECTED, "CertCloseStore returned: %x(%lx)\n", res, GetLastError()); diff --git a/dlls/cryptui/main.c b/dlls/cryptui/main.c index 2bd6b0b9f6a..2edab44ffd6 100644 --- a/dlls/cryptui/main.c +++ b/dlls/cryptui/main.c @@ -987,7 +987,9 @@ static void cert_mgr_do_remove(HWND hwnd) PCCERT_CONTEXT cert = cert_mgr_index_to_cert(hwnd, selection); - CertDeleteCertificateFromStore(cert); + /* CertDeleteCertificateFromStore() releases certificate, duplicate it to keep context stored + * in the list valid. */ + CertDeleteCertificateFromStore(CertDuplicateCertificateContext(cert)); } } while (selection >= 0); cert_mgr_clear_cert_selection(hwnd); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7193
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/crypt32/store.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/dlls/crypt32/store.c b/dlls/crypt32/store.c index f41aad2c3ff..565368714b1 100644 --- a/dlls/crypt32/store.c +++ b/dlls/crypt32/store.c @@ -195,6 +195,11 @@ static context_t *MemStore_enumContext(WINE_MEMSTORE *store, struct list *list, return ret; } +static void memstore_free_context(context_t *context) +{ + Context_Free(context); +} + static BOOL MemStore_deleteContext(WINE_MEMSTORE *store, context_t *context) { BOOL in_list = FALSE; @@ -208,7 +213,7 @@ static BOOL MemStore_deleteContext(WINE_MEMSTORE *store, context_t *context) LeaveCriticalSection(&store->cs); if(in_list && !context->ref) - Context_Free(context); + memstore_free_context(context); return TRUE; } @@ -220,7 +225,7 @@ static void free_contexts(struct list *list) { TRACE("freeing %p\n", context); list_remove(&context->u.entry); - Context_Free(context); + memstore_free_context(context); } } @@ -228,7 +233,7 @@ static void MemStore_releaseContext(WINECRYPT_CERTSTORE *store, context_t *conte { /* Free the context only if it's not in a list. Otherwise it may be reused later. */ if(list_empty(&context->u.entry)) - Context_Free(context); + memstore_free_context(context); } static BOOL MemStore_addCert(WINECRYPT_CERTSTORE *store, context_t *cert, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7193
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/crypt32/crypt32_private.h | 1 + dlls/crypt32/store.c | 60 ++++++++++++++++++++++++---------- 2 files changed, 44 insertions(+), 17 deletions(-) diff --git a/dlls/crypt32/crypt32_private.h b/dlls/crypt32/crypt32_private.h index 9ac5b4a3067..0d0e9908df8 100644 --- a/dlls/crypt32/crypt32_private.h +++ b/dlls/crypt32/crypt32_private.h @@ -192,6 +192,7 @@ struct _context_t { struct list entry; void *ptr; } u; + struct list enum_entry; }; static inline context_t *context_from_ptr(const void *ptr) diff --git a/dlls/crypt32/store.c b/dlls/crypt32/store.c index 565368714b1..54904725124 100644 --- a/dlls/crypt32/store.c +++ b/dlls/crypt32/store.c @@ -80,9 +80,14 @@ typedef struct _WINE_MEMSTORE { WINECRYPT_CERTSTORE hdr; CRITICAL_SECTION cs; + /* Objects are deleteted from the main list upon deleting from store and from _enum lists + * upon deleting objects themselves. */ struct list certs; + struct list certs_enum; struct list crls; + struct list crls_enum; struct list ctls; + struct list ctls_enum; } WINE_MEMSTORE; void CRYPT_InitStore(WINECRYPT_CERTSTORE *store, DWORD dwFlags, CertStoreType type, const store_vtbl_t *vtbl) @@ -140,8 +145,8 @@ BOOL WINAPI I_CertUpdateStore(HCERTSTORE store1, HCERTSTORE store2, DWORD unk0, return TRUE; } -static BOOL MemStore_addContext(WINE_MEMSTORE *store, struct list *list, context_t *orig_context, - context_t *existing, context_t **ret_context, BOOL use_link) +static BOOL MemStore_addContext(WINE_MEMSTORE *store, struct list *list, struct list *list_enum, + context_t *orig_context, context_t *existing, context_t **ret_context, BOOL use_link) { context_t *context; @@ -152,15 +157,16 @@ static BOOL MemStore_addContext(WINE_MEMSTORE *store, struct list *list, context TRACE("adding %p\n", context); EnterCriticalSection(&store->cs); if (existing) { - context->u.entry.prev = existing->u.entry.prev; - context->u.entry.next = existing->u.entry.next; - context->u.entry.prev->next = &context->u.entry; - context->u.entry.next->prev = &context->u.entry; + list_add_before(&existing->u.entry, &context->u.entry); + list_remove(&existing->u.entry); list_init(&existing->u.entry); + list_add_before(&existing->enum_entry, &context->enum_entry); + /* existing will be removed from enum list on deleteing context. */ if(!existing->ref) Context_Release(existing); }else { list_add_head(list, &context->u.entry); + list_add_head(list_enum, &context->enum_entry); } LeaveCriticalSection(&store->cs); @@ -171,32 +177,49 @@ static BOOL MemStore_addContext(WINE_MEMSTORE *store, struct list *list, context return TRUE; } -static context_t *MemStore_enumContext(WINE_MEMSTORE *store, struct list *list, context_t *prev) +static context_t *MemStore_enumContext(WINE_MEMSTORE *store, struct list *list_enum, context_t *prev) { struct list *next; context_t *ret; EnterCriticalSection(&store->cs); if (prev) { - next = list_next(list, &prev->u.entry); + next = list_next(list_enum, &prev->enum_entry); Context_Release(prev); }else { - next = list_next(list, list); + next = list_head(list_enum); } + + while (next) + { + ret = LIST_ENTRY(next, context_t, enum_entry); + if (!list_empty(&ret->u.entry)) + break; + next = list_next(list_enum, next); + } + if (!next) + ret = NULL; LeaveCriticalSection(&store->cs); - if (!next) { + if (!ret) { SetLastError(CRYPT_E_NOT_FOUND); return NULL; } - ret = LIST_ENTRY(next, context_t, u.entry); Context_AddRef(ret); return ret; } static void memstore_free_context(context_t *context) { + WINE_MEMSTORE *store = (WINE_MEMSTORE *)context->store; + + TRACE(".\n"); + + EnterCriticalSection(&store->cs); + list_remove(&context->enum_entry); + LeaveCriticalSection(&store->cs); + Context_Free(context); } @@ -242,7 +265,7 @@ static BOOL MemStore_addCert(WINECRYPT_CERTSTORE *store, context_t *cert, WINE_MEMSTORE *ms = (WINE_MEMSTORE *)store; TRACE("(%p, %p, %p, %p)\n", store, cert, toReplace, ppStoreContext); - return MemStore_addContext(ms, &ms->certs, cert, toReplace, ppStoreContext, use_link); + return MemStore_addContext(ms, &ms->certs, &ms->certs_enum, cert, toReplace, ppStoreContext, use_link); } static context_t *MemStore_enumCert(WINECRYPT_CERTSTORE *store, context_t *prev) @@ -251,7 +274,7 @@ static context_t *MemStore_enumCert(WINECRYPT_CERTSTORE *store, context_t *prev) TRACE("(%p, %p)\n", store, prev); - return MemStore_enumContext(ms, &ms->certs, prev); + return MemStore_enumContext(ms, &ms->certs_enum, prev); } static BOOL MemStore_deleteCert(WINECRYPT_CERTSTORE *store, context_t *context) @@ -270,7 +293,7 @@ static BOOL MemStore_addCRL(WINECRYPT_CERTSTORE *store, context_t *crl, TRACE("(%p, %p, %p, %p)\n", store, crl, toReplace, ppStoreContext); - return MemStore_addContext(ms, &ms->crls, crl, toReplace, ppStoreContext, use_link); + return MemStore_addContext(ms, &ms->crls, &ms->crls_enum, crl, toReplace, ppStoreContext, use_link); } static context_t *MemStore_enumCRL(WINECRYPT_CERTSTORE *store, context_t *prev) @@ -279,7 +302,7 @@ static context_t *MemStore_enumCRL(WINECRYPT_CERTSTORE *store, context_t *prev) TRACE("(%p, %p)\n", store, prev); - return MemStore_enumContext(ms, &ms->crls, prev); + return MemStore_enumContext(ms, &ms->crls_enum, prev); } static BOOL MemStore_deleteCRL(WINECRYPT_CERTSTORE *store, context_t *context) @@ -298,7 +321,7 @@ static BOOL MemStore_addCTL(WINECRYPT_CERTSTORE *store, context_t *ctl, TRACE("(%p, %p, %p, %p)\n", store, ctl, toReplace, ppStoreContext); - return MemStore_addContext(ms, &ms->ctls, ctl, toReplace, ppStoreContext, use_link); + return MemStore_addContext(ms, &ms->ctls, &ms->ctls_enum, ctl, toReplace, ppStoreContext, use_link); } static context_t *MemStore_enumCTL(WINECRYPT_CERTSTORE *store, context_t *prev) @@ -307,7 +330,7 @@ static context_t *MemStore_enumCTL(WINECRYPT_CERTSTORE *store, context_t *prev) TRACE("(%p, %p)\n", store, prev); - return MemStore_enumContext(ms, &ms->ctls, prev); + return MemStore_enumContext(ms, &ms->ctls_enum, prev); } static BOOL MemStore_deleteCTL(WINECRYPT_CERTSTORE *store, context_t *context) @@ -396,8 +419,11 @@ static WINECRYPT_CERTSTORE *CRYPT_MemOpenStore(HCRYPTPROV hCryptProv, InitializeCriticalSectionEx(&store->cs, 0, RTL_CRITICAL_SECTION_FLAG_FORCE_DEBUG_INFO); store->cs.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": ContextList.cs"); list_init(&store->certs); + list_init(&store->certs_enum); list_init(&store->crls); + list_init(&store->crls_enum); list_init(&store->ctls); + list_init(&store->ctls_enum); /* Mem store doesn't need crypto provider, so close it */ if (hCryptProv && !(dwFlags & CERT_STORE_NO_CRYPT_RELEASE_FLAG)) CryptReleaseContext(hCryptProv, 0); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7193
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/crypt32/tests/store.c | 81 +++++++++++++++++++++++++++++++++++++- 1 file changed, 79 insertions(+), 2 deletions(-) diff --git a/dlls/crypt32/tests/store.c b/dlls/crypt32/tests/store.c index 48f06c54f7c..0e814ed6429 100644 --- a/dlls/crypt32/tests/store.c +++ b/dlls/crypt32/tests/store.c @@ -117,6 +117,19 @@ static const BYTE signedCTLWithCTLInnerContent[] = { 0x8e,0xe7,0x5f,0x76,0x2b,0xd1,0x6a,0x82,0xb3,0x30,0x25,0x61,0xf6,0x25,0x23, 0x57,0x6c,0x0b,0x47,0xb8 }; +static const BYTE certWithUsage[] = { 0x30, 0x81, 0x93, 0x02, 0x01, 0x01, 0x30, + 0x02, 0x06, 0x00, 0x30, 0x15, 0x31, 0x13, 0x30, 0x11, 0x06, 0x03, 0x55, 0x04, + 0x03, 0x13, 0x0a, 0x4a, 0x75, 0x61, 0x6e, 0x20, 0x4c, 0x61, 0x6e, 0x67, 0x00, + 0x30, 0x22, 0x18, 0x0f, 0x31, 0x36, 0x30, 0x31, 0x30, 0x31, 0x30, 0x31, 0x30, + 0x30, 0x30, 0x30, 0x30, 0x30, 0x5a, 0x18, 0x0f, 0x31, 0x36, 0x30, 0x31, 0x30, + 0x31, 0x30, 0x31, 0x30, 0x30, 0x30, 0x30, 0x30, 0x30, 0x5a, 0x30, 0x15, 0x31, + 0x13, 0x30, 0x11, 0x06, 0x03, 0x55, 0x04, 0x03, 0x13, 0x0a, 0x4a, 0x75, 0x61, + 0x6e, 0x20, 0x4c, 0x61, 0x6e, 0x67, 0x00, 0x30, 0x07, 0x30, 0x02, 0x06, 0x00, + 0x03, 0x01, 0x00, 0xa3, 0x2f, 0x30, 0x2d, 0x30, 0x2b, 0x06, 0x03, 0x55, 0x1d, + 0x25, 0x01, 0x01, 0xff, 0x04, 0x21, 0x30, 0x1f, 0x06, 0x08, 0x2b, 0x06, 0x01, + 0x05, 0x05, 0x07, 0x03, 0x03, 0x06, 0x08, 0x2b, 0x06, 0x01, 0x05, 0x05, 0x07, + 0x03, 0x02, 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x01, 0x01 }; + #define test_store_is_empty(store) _test_store_is_empty(__LINE__,store) static void _test_store_is_empty(unsigned line, HCERTSTORE store) { @@ -128,10 +141,16 @@ static void _test_store_is_empty(unsigned line, HCERTSTORE store) static void testMemStore(void) { + static const BYTE cert1_hash[] = + { 0x4a, 0x7f, 0x32, 0x1f, 0xcf, 0x3b, 0xc0, 0x87, 0x48, 0x2b, 0xa1, 0x86, 0x54, 0x18, 0xe4, 0x3a, 0x0e, 0x53, 0x7e, 0x2b, }; + static const BYTE cert2_hash[] = + { 0xef, 0x7c, 0x6f, 0x27, 0x13, 0xff, 0x84, 0x14, 0x7c, 0xb5, 0xf0, 0x0c, 0x5a, 0x82, 0x91, 0x78, 0x96, 0x82, 0x19, 0x5c, }; HCERTSTORE store1, store2; - PCCERT_CONTEXT context; + PCCERT_CONTEXT context, stored[2], copy; + unsigned int count; + BYTE hash[20]; BOOL ret; - DWORD GLE; + DWORD GLE, size; /* NULL provider */ store1 = CertOpenStore(0, 0, 0, 0, NULL); @@ -266,6 +285,64 @@ static void testMemStore(void) ok(!context, "Expected an empty store\n"); } + /* Test deleting and adding cert during enumeration. */ + ret = CertAddEncodedCertificateToStore(store1, X509_ASN_ENCODING, bigCert, + sizeof(bigCert), CERT_STORE_ADD_ALWAYS, NULL); + ok(ret, "CertAddEncodedCertificateToStore failed: %08lx\n", GetLastError()); + ret = CertAddEncodedCertificateToStore(store1, X509_ASN_ENCODING, bigCert2, + sizeof(bigCert2), CERT_STORE_ADD_ALWAYS, NULL); + ok(ret, "CertAddEncodedCertificateToStore failed: %08lx\n", GetLastError()); + + context = NULL; + count = 0; + while ((context = CertEnumCertificatesInStore(store1, context))) + { + ok(count < 2, "got %u.\n", count); + stored[count] = CertDuplicateCertificateContext(context); + ++count; + } + ok(count == 2, "got %u.\n", count); + + count = 0; + context = NULL; + while ((context = CertEnumCertificatesInStore(store1, context))) + { + size = sizeof(hash); + ret = CertGetCertificateContextProperty(context, CERT_HASH_PROP_ID, hash, &size); + ok(ret, "CertGetCertificateContextProperty failed: %08lx\n", GetLastError()); + ok(!memcmp(hash, cert1_hash, sizeof(hash)), "wrong cert.\n"); + copy = CertDuplicateCertificateContext(context); + ok(copy == context, "got %p, %p.\n", copy, context); + ret = CertDeleteCertificateFromStore(copy); + ok(ret, "CertDeleteCertificateFromStore failed: %08lx\n", GetLastError()); + if (!count) + { + ret = CertDeleteCertificateFromStore(stored[1]); + ok(ret, "CertDeleteCertificateFromStore failed: %08lx\n", GetLastError()); + ret = CertAddEncodedCertificateToStore(store1, X509_ASN_ENCODING, certWithUsage, + sizeof(certWithUsage), CERT_STORE_ADD_ALWAYS, NULL); + ok(ret, "CertAddEncodedCertificateToStore failed: %08lx\n", GetLastError()); + } + ++count; + } + ok(count == 1, "got %u.\n", count); + + count = 0; + context = NULL; + while ((context = CertEnumCertificatesInStore(store1, context))) + { + size = sizeof(hash); + ret = CertGetCertificateContextProperty(context, CERT_HASH_PROP_ID, hash, &size); + ok(ret, "CertGetCertificateContextProperty failed: %08lx\n", GetLastError()); + ok(!memcmp(hash, cert2_hash, sizeof(hash)), "wrong cert.\n"); + copy = CertDuplicateCertificateContext(context); + ret = CertDeleteCertificateFromStore(copy); + ok(ret, "CertDeleteCertificateFromStore failed: %08lx\n", GetLastError()); + ++count; + } + ok(count == 1, "got %u.\n", count); + CertFreeCertificateContext(stored[0]); + /* close an empty store */ ret = CertCloseStore(NULL, 0); ok(ret, "CertCloseStore failed: %ld\n", GetLastError()); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7193
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/crypt32/context.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/dlls/crypt32/context.c b/dlls/crypt32/context.c index 4a16d42296b..76034be3cf6 100644 --- a/dlls/crypt32/context.c +++ b/dlls/crypt32/context.c @@ -108,7 +108,12 @@ void Context_Release(context_t *context) LONG ref = InterlockedDecrement(&context->ref); TRACE("(%p) ref=%ld\n", context, ref); - assert(ref >= 0); + + if (ref < 0) + { + ERR( "ref %ld.\n", ref ); + return; + } if (!ref) { WINECRYPT_CERTSTORE *store = context->store; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7193
This fixes Battle.net error during install and updates which happen to have Battle.net's ephemeral cert installed on host (happens on Macs with native Battle.net client installed). The problem comes from CertDeleteCertificateFromStore() not releasing cert reference which results in leaked store which never flushes changes to registry due to that. BNet, MS documentation, MS samples in documentation all seem to assume that CertDeleteCertificateFromStore should actually decrement the refcount. And so indirectly do my tests, while I didn't find a good way to test that directly: native crypt32 looks extremely robust to extra certificate release and I couldn't ever get it to crash this way. However, closing store with CERT_CLOSE_STORE_CHECK_FLAG seem to indicate the cases when a cert (and consequently the store) has extra referencesat store close and testing with that confirms that CertDeleteCertificateFromStore() deletes the reference (duplicating cert before store close may trigger the error). I checked the code for the use CertDeleteCertificateFromStore() which would assume that it does not decrement refcount. I found one in rootstore.c and in cryptui.dll which are changed. I_CertUpdateStore() was leaking references in store1 instead, that looks also fixed by patch 1. Then, while looking at certificate enumeration which could be sensitive to refcounting changes I found that it is broken for the case of deleting certificate during enumeration even for the exact scenario illustrated in MS example as a recommended way to delete a cert during enumeration (https://learn.microsoft.com/en-us/windows/win32/seccrypto/example-c-program-...). That is tested and fixed in the patches. Then, given that: - it looks almost impossible to get crypt32 refcounting model correctly, and calling an extra CertFreeCertificateContext() after (at least right after) deleting from store arbitrary amount of times doesn't result in any issue on Windows; - Now when CertDeleteCertificateFromStore() decrement refcount app bugs doing an extra CertFreeCertificateContext after that (like we did on some occasion previously) will lead to crashes on Wine it seems to me it make sense to replace an assert in Context_Release() which I am doing in patch 5. This way such an extra CertFreeCertificateContext() calls have good chances to be harmless on Wine too. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7193#note_92768
Actually, it is probably better to just introduce 'deleted' flag instead of the extra list, also getting rid of counterintutive pre-existing checks for list_empty() on cert's list entry to detect that cert is deleted from the store. I will change that. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7193#note_92776
participants (2)
-
Paul Gofman -
Paul Gofman (＠gofman)