[PATCH v3 0/7] MR7193: crypt32: A refcount fix and cert enumeraton improvement.
-- v3: crypt32: Don't assert in Context_Release() on invalid refcount. crypt32/tests: Add a test for deleting and adding certs during enumeration. crypt32: Only remove cert from mem store list when deleting it. crypt32: Release existing cert context in add_cert_to_store(). crypt32: Don't try to release zero-refcount context in MemStore_addContext(). crypt32: Factor out memstore_free_context() function. crypt32: Release cert context in CertDeleteCertificateFromStore(). https://gitlab.winehq.org/wine/wine/-/merge_requests/7193
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..2ea9a898c6a 100644 --- a/dlls/crypt32/store.c +++ b/dlls/crypt32/store.c @@ -140,6 +140,11 @@ BOOL WINAPI I_CertUpdateStore(HCERTSTORE store1, HCERTSTORE store2, DWORD unk0, return TRUE; } +static void memstore_free_context(context_t *context) +{ + Context_Free(context); +} + static BOOL MemStore_addContext(WINE_MEMSTORE *store, struct list *list, context_t *orig_context, context_t *existing, context_t **ret_context, BOOL use_link) { @@ -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> That is essentially a no-op code, or that would be asserting in Context_Release(). --- dlls/crypt32/store.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/dlls/crypt32/store.c b/dlls/crypt32/store.c index 2ea9a898c6a..97b745e661e 100644 --- a/dlls/crypt32/store.c +++ b/dlls/crypt32/store.c @@ -162,8 +162,6 @@ static BOOL MemStore_addContext(WINE_MEMSTORE *store, struct list *list, context context->u.entry.prev->next = &context->u.entry; context->u.entry.next->prev = &context->u.entry; list_init(&existing->u.entry); - if(!existing->ref) - Context_Release(existing); }else { list_add_head(list, &context->u.entry); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7193
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/crypt32/cert.c | 13 ++++++++++++- dlls/crypt32/tests/cert.c | 6 ++++-- dlls/crypt32/tests/store.c | 6 ++++-- 3 files changed, 20 insertions(+), 5 deletions(-) diff --git a/dlls/crypt32/cert.c b/dlls/crypt32/cert.c index 7b1edaa2236..75f451435b4 100644 --- a/dlls/crypt32/cert.c +++ b/dlls/crypt32/cert.c @@ -212,6 +212,7 @@ static BOOL add_cert_to_store(WINECRYPT_CERTSTORE *store, const CERT_CONTEXT *ce if (existing) { TRACE("found matching certificate, not adding\n"); + CertFreeCertificateContext(existing); SetLastError(CRYPT_E_EXISTS); return FALSE; } @@ -231,7 +232,9 @@ static BOOL add_cert_to_store(WINECRYPT_CERTSTORE *store, const CERT_CONTEXT *ce { Context_CopyProperties(existing, cert); if (ret_context) - *ret_context = CertDuplicateCertificateContext(existing); + *ret_context = existing; + else + CertFreeCertificateContext(existing); return TRUE; } break; @@ -239,6 +242,7 @@ static BOOL add_cert_to_store(WINECRYPT_CERTSTORE *store, const CERT_CONTEXT *ce if (existing && CompareFileTime(&existing->pCertInfo->NotBefore, &cert->pCertInfo->NotBefore) >= 0) { TRACE("existing certificate is newer, not adding\n"); + CertFreeCertificateContext(existing); SetLastError(CRYPT_E_EXISTS); return FALSE; } @@ -249,6 +253,7 @@ static BOOL add_cert_to_store(WINECRYPT_CERTSTORE *store, const CERT_CONTEXT *ce if (CompareFileTime(&existing->pCertInfo->NotBefore, &cert->pCertInfo->NotBefore) >= 0) { TRACE("existing certificate is newer, not adding\n"); + CertFreeCertificateContext(existing); SetLastError(CRYPT_E_EXISTS); return FALSE; } @@ -267,7 +272,10 @@ static BOOL add_cert_to_store(WINECRYPT_CERTSTORE *store, const CERT_CONTEXT *ce ret = store->vtbl->certs.addContext(store, context_from_ptr(cert), existing ? context_from_ptr(existing) : NULL, (ret_context || inherit_props) ? &new_context : NULL, use_link); if(!ret) + { + CertFreeCertificateContext(existing); return FALSE; + } if(inherit_props) Context_CopyProperties(context_ptr(new_context), existing); @@ -277,6 +285,9 @@ static BOOL add_cert_to_store(WINECRYPT_CERTSTORE *store, const CERT_CONTEXT *ce else if(new_context) Context_Release(new_context); + if (existing) + CertFreeCertificateContext(existing); + TRACE("returning %d\n", ret); return ret; } diff --git a/dlls/crypt32/tests/cert.c b/dlls/crypt32/tests/cert.c index 396f6a2e390..aeab4c02713 100644 --- a/dlls/crypt32/tests/cert.c +++ b/dlls/crypt32/tests/cert.c @@ -275,10 +275,12 @@ static void testAddCert(void) CertFreeCertificateContext(context); } - CertCloseStore(collection, 0); + ret = CertCloseStore(collection, CERT_CLOSE_STORE_CHECK_FLAG); + ok(ret, "got error %#lx.\n", GetLastError()); } - CertCloseStore(store, 0); + ret = CertCloseStore(store, CERT_CLOSE_STORE_CHECK_FLAG); + ok(ret, "got error %#lx.\n", GetLastError()); } static void checkHash(const BYTE *data, DWORD dataLen, ALG_ID algID, diff --git a/dlls/crypt32/tests/store.c b/dlls/crypt32/tests/store.c index 48f06c54f7c..874a920e1d7 100644 --- a/dlls/crypt32/tests/store.c +++ b/dlls/crypt32/tests/store.c @@ -2962,8 +2962,10 @@ static void test_I_UpdateStore(void) certs = countCertsInStore(store1); ok(certs == 0, "Expected 0 certs, got %ld\n", certs); - CertCloseStore(store1, 0); - CertCloseStore(store2, 0); + 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()); } static const BYTE pfxdata[] = -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7193
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/crypt32/context.c | 2 ++ dlls/crypt32/crypt32_private.h | 1 + dlls/crypt32/store.c | 55 ++++++++++++++++++++-------------- 3 files changed, 36 insertions(+), 22 deletions(-) diff --git a/dlls/crypt32/context.c b/dlls/crypt32/context.c index 4a16d42296b..1a982f7bd0a 100644 --- a/dlls/crypt32/context.c +++ b/dlls/crypt32/context.c @@ -42,6 +42,7 @@ context_t *Context_CreateDataContext(size_t contextSize, const context_vtbl_t *v context->vtbl = vtbl; context->ref = 1; + context->deleted_from_store = FALSE; context->linked = NULL; store->vtbl->addref(store); @@ -64,6 +65,7 @@ context_t *Context_CreateLinkContext(unsigned int contextSize, context_t *linked memcpy(context_ptr(context), context_ptr(linked), contextSize); context->vtbl = linked->vtbl; context->ref = 1; + context->deleted_from_store = FALSE; context->linked = linked; context->properties = linked->properties; Context_AddRef(linked); diff --git a/dlls/crypt32/crypt32_private.h b/dlls/crypt32/crypt32_private.h index 9ac5b4a3067..6548dd36ab0 100644 --- a/dlls/crypt32/crypt32_private.h +++ b/dlls/crypt32/crypt32_private.h @@ -185,6 +185,7 @@ typedef struct { struct _context_t { const context_vtbl_t *vtbl; LONG ref; + BOOL deleted_from_store; struct WINE_CRYPTCERTSTORE *store; struct _context_t *linked; CONTEXT_PROPERTY_LIST *properties; diff --git a/dlls/crypt32/store.c b/dlls/crypt32/store.c index 97b745e661e..3291d0522b6 100644 --- a/dlls/crypt32/store.c +++ b/dlls/crypt32/store.c @@ -80,6 +80,8 @@ 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 crls; struct list ctls; @@ -142,6 +144,14 @@ BOOL WINAPI I_CertUpdateStore(HCERTSTORE store1, HCERTSTORE store2, DWORD unk0, static void memstore_free_context(context_t *context) { + WINE_MEMSTORE *store = (WINE_MEMSTORE *)context->store; + + TRACE(".\n"); + + EnterCriticalSection(&store->cs); + list_remove(&context->u.entry); + LeaveCriticalSection(&store->cs); + Context_Free(context); } @@ -157,11 +167,8 @@ 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_init(&existing->u.entry); + list_add_before(&existing->u.entry, &context->u.entry); + existing->deleted_from_store = TRUE; }else { list_add_head(list, &context->u.entry); } @@ -184,34 +191,37 @@ static context_t *MemStore_enumContext(WINE_MEMSTORE *store, struct list *list, next = list_next(list, &prev->u.entry); Context_Release(prev); }else { - next = list_next(list, list); + next = list_head(list); + } + + while (next) + { + ret = LIST_ENTRY(next, context_t, u.entry); + if (!ret->deleted_from_store) + break; + next = list_next(list, 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 BOOL MemStore_deleteContext(WINE_MEMSTORE *store, context_t *context) { - BOOL in_list = FALSE; - - EnterCriticalSection(&store->cs); - if (!list_empty(&context->u.entry)) { - list_remove(&context->u.entry); - list_init(&context->u.entry); - in_list = TRUE; + if (!context->deleted_from_store) + { + context->deleted_from_store = TRUE; + if (!context->ref) + memstore_free_context(context); } - LeaveCriticalSection(&store->cs); - - if(in_list && !context->ref) - memstore_free_context(context); return TRUE; } @@ -223,14 +233,15 @@ static void free_contexts(struct list *list) { TRACE("freeing %p\n", context); list_remove(&context->u.entry); - memstore_free_context(context); + if (!context->deleted_from_store) + memstore_free_context(context); } } static void MemStore_releaseContext(WINECRYPT_CERTSTORE *store, context_t *context) { - /* Free the context only if it's not in a list. Otherwise it may be reused later. */ - if(list_empty(&context->u.entry)) + /* Free the context only if it is deleted from store. Otherwise it may be reused later. */ + if (context->deleted_from_store) memstore_free_context(context); } -- 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 874a920e1d7..fc52c2ce28d 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 1a982f7bd0a..812fb51e36b 100644 --- a/dlls/crypt32/context.c +++ b/dlls/crypt32/context.c @@ -110,7 +110,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
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=151045 Your paranoid android. === debian11b (64 bit WoW report) === kernel32: comm.c:1586: Test failed: Unexpected time 1001, expected around 500 user32: input.c:4306: Test succeeded inside todo block: button_down_hwnd_todo 1: got MSG_TEST_WIN hwnd 0000000000A200E6, msg WM_LBUTTONDOWN, wparam 0x1, lparam 0x320032 winstation.c:979: Test failed: unexpected foreground window 0000000000010020
v3: - add patch properly releasing existing certificate in add_cert_to_store(). After removing dead code in the previous update I got curious if that code was actually intended to do something and found that refcounting issue on adding cert to store when existing cert is in play (the modified test with cert.c properly shows leaked reference without the changes). I believe doing any refcounting on 'existing' cert in MemStore_addContext() would be wrong, the store doesn't hold references to stored objects. That seems to be already correctly handled in CertAddCTLContextToStore() and CertAddCRLContextToStore(). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7193#note_92791
The other theoretical option would be to do Context_Free(existing) for zero refcount instead of Context_Release (which is done and makes sense in a few other places), but I believe zero refcount just should not happen there, the caller should either be owning the cert (that is, having some refcount) or the cert with zero refcount may exist in memstore but it should not be accessed directly from the outside in this state. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7193#note_92792
participants (3)
-
Marvin -
Paul Gofman -
Paul Gofman (＠gofman)