From 8ba1079f9bd9a139ae78bd52fa33df7bff9cd02a Mon Sep 17 00:00:00 2001 From: neonene Date: Mon, 29 Jul 2019 17:19:26 +0900 Subject: [PATCH 01/12] bpo-37702: Fix SSL certificate-store-handles leak In Windows, CertCloseStore(hCollectionStore) closes only hCollectionStore. To avoid out-of-memory, CertCloseStore() shuld be called to each hSystemStore which was added to hCollectionStore. --- .../2019-07-29-16-49-31.bpo-37702.Lj2f5e.rst | 1 + Modules/_ssl.c | 84 ++++++++++++++----- 2 files changed, 65 insertions(+), 20 deletions(-) create mode 100644 Misc/NEWS.d/next/Windows/2019-07-29-16-49-31.bpo-37702.Lj2f5e.rst diff --git a/Misc/NEWS.d/next/Windows/2019-07-29-16-49-31.bpo-37702.Lj2f5e.rst b/Misc/NEWS.d/next/Windows/2019-07-29-16-49-31.bpo-37702.Lj2f5e.rst new file mode 100644 index 00000000000000..61fd2a997b9a1d --- /dev/null +++ b/Misc/NEWS.d/next/Windows/2019-07-29-16-49-31.bpo-37702.Lj2f5e.rst @@ -0,0 +1 @@ +Fix memory leak in ssl certification. diff --git a/Modules/_ssl.c b/Modules/_ssl.c index da30cbb758e27e..018c7c091601d6 100644 --- a/Modules/_ssl.c +++ b/Modules/_ssl.c @@ -5544,7 +5544,9 @@ parseKeyUsage(PCCERT_CONTEXT pCertCtx, DWORD flags) } static HCERTSTORE -ssl_collect_certificates(const char *store_name) +ssl_collect_certificates(const char *store_name, + DWORD *system_stores, size_t system_stores_count, + HCERTSTORE *system_store_handles) { /* this function collects the system certificate stores listed in * system_stores into a collection certificate store for being @@ -5553,14 +5555,6 @@ ssl_collect_certificates(const char *store_name) */ HCERTSTORE hCollectionStore = NULL, hSystemStore = NULL; - static DWORD system_stores[] = { - CERT_SYSTEM_STORE_LOCAL_MACHINE, - CERT_SYSTEM_STORE_LOCAL_MACHINE_ENTERPRISE, - CERT_SYSTEM_STORE_LOCAL_MACHINE_GROUP_POLICY, - CERT_SYSTEM_STORE_CURRENT_USER, - CERT_SYSTEM_STORE_CURRENT_USER_GROUP_POLICY, - CERT_SYSTEM_STORE_SERVICES, - CERT_SYSTEM_STORE_USERS}; size_t i, storesAdded; BOOL result; @@ -5570,12 +5564,14 @@ ssl_collect_certificates(const char *store_name) return NULL; } storesAdded = 0; - for (i = 0; i < sizeof(system_stores) / sizeof(DWORD); i++) { + for (i = 0; i < system_stores_count; i++) { + system_store_handles[i] = NULL; hSystemStore = CertOpenStore(CERT_STORE_PROV_SYSTEM_A, 0, (HCRYPTPROV)NULL, CERT_STORE_READONLY_FLAG | system_stores[i], store_name); if (hSystemStore) { + system_store_handles[i] = hSystemStore; result = CertAddStoreToCollection(hCollectionStore, hSystemStore, CERT_PHYSICAL_STORE_ADD_ENABLE_FLAG, 0); if (result) { @@ -5626,12 +5622,26 @@ _ssl_enum_certificates_impl(PyObject *module, const char *store_name) PCCERT_CONTEXT pCertCtx = NULL; PyObject *keyusage = NULL, *cert = NULL, *enc = NULL, *tup = NULL; PyObject *result = NULL; + static DWORD system_stores[] = { + CERT_SYSTEM_STORE_LOCAL_MACHINE, + CERT_SYSTEM_STORE_LOCAL_MACHINE_ENTERPRISE, + CERT_SYSTEM_STORE_LOCAL_MACHINE_GROUP_POLICY, + CERT_SYSTEM_STORE_CURRENT_USER, + CERT_SYSTEM_STORE_CURRENT_USER_GROUP_POLICY, + CERT_SYSTEM_STORE_SERVICES, + CERT_SYSTEM_STORE_USERS}; + BOOL success; + size_t i, system_stores_count = sizeof(system_stores) / sizeof(DWORD); + HCERTSTORE system_store_handles[sizeof(system_stores) / sizeof(DWORD)]; result = PyList_New(0); if (result == NULL) { return NULL; } - hCollectionStore = ssl_collect_certificates(store_name); + hCollectionStore = ssl_collect_certificates(store_name, + system_stores, + system_stores_count, + system_store_handles); if (hCollectionStore == NULL) { Py_DECREF(result); return PyErr_SetFromWindowsErr(GetLastError()); @@ -5686,10 +5696,20 @@ _ssl_enum_certificates_impl(PyObject *module, const char *store_name) Py_XDECREF(keyusage); Py_XDECREF(tup); - /* CERT_CLOSE_STORE_FORCE_FLAG forces freeing of memory for all contexts - associated with the store, in this case our collection store and the - associated system stores. */ - if (!CertCloseStore(hCollectionStore, CERT_CLOSE_STORE_FORCE_FLAG)) { + /* When a collection store and its sibling stores are closed + with CertCloseStore using CERT_CLOSE_STORE_FORCE_FLAG, + the collection store must be closed before its sibling stores. + (https://docs.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-certaddstoretocollection) */ + success = CertCloseStore(hCollectionStore, CERT_CLOSE_STORE_FORCE_FLAG); + for (i = 0; i < system_stores_count; i++) { + if (system_store_handles[i] == NULL) { + continue; + } + if (!CertCloseStore(system_store_handles[i], CERT_CLOSE_STORE_FORCE_FLAG)) { + success = 0; + } + } + if (!success) { /* This error case might shadow another exception.*/ Py_XDECREF(result); return PyErr_SetFromWindowsErr(GetLastError()); @@ -5718,12 +5738,26 @@ _ssl_enum_crls_impl(PyObject *module, const char *store_name) PCCRL_CONTEXT pCrlCtx = NULL; PyObject *crl = NULL, *enc = NULL, *tup = NULL; PyObject *result = NULL; + static DWORD system_stores[] = { + CERT_SYSTEM_STORE_LOCAL_MACHINE, + CERT_SYSTEM_STORE_LOCAL_MACHINE_ENTERPRISE, + CERT_SYSTEM_STORE_LOCAL_MACHINE_GROUP_POLICY, + CERT_SYSTEM_STORE_CURRENT_USER, + CERT_SYSTEM_STORE_CURRENT_USER_GROUP_POLICY, + CERT_SYSTEM_STORE_SERVICES, + CERT_SYSTEM_STORE_USERS}; + BOOL success; + size_t i, system_stores_count = sizeof(system_stores) / sizeof(DWORD); + HCERTSTORE system_store_handles[sizeof(system_stores) / sizeof(DWORD)]; result = PyList_New(0); if (result == NULL) { return NULL; } - hCollectionStore = ssl_collect_certificates(store_name); + hCollectionStore = ssl_collect_certificates(store_name, + system_stores, + system_stores_count, + system_store_handles); if (hCollectionStore == NULL) { Py_DECREF(result); return PyErr_SetFromWindowsErr(GetLastError()); @@ -5767,10 +5801,20 @@ _ssl_enum_crls_impl(PyObject *module, const char *store_name) Py_XDECREF(enc); Py_XDECREF(tup); - /* CERT_CLOSE_STORE_FORCE_FLAG forces freeing of memory for all contexts - associated with the store, in this case our collection store and the - associated system stores. */ - if (!CertCloseStore(hCollectionStore, CERT_CLOSE_STORE_FORCE_FLAG)) { + /* When a collection store and its sibling stores are closed + with CertCloseStore using CERT_CLOSE_STORE_FORCE_FLAG, + the collection store must be closed before its sibling stores. + (https://docs.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-certaddstoretocollection) */ + success = CertCloseStore(hCollectionStore, CERT_CLOSE_STORE_FORCE_FLAG); + for (i = 0; i < system_stores_count; i++) { + if (system_store_handles[i] == NULL) { + continue; + } + if (!CertCloseStore(system_store_handles[i], CERT_CLOSE_STORE_FORCE_FLAG)) { + success = 0; + } + } + if (!success) { /* This error case might shadow another exception.*/ Py_XDECREF(result); return PyErr_SetFromWindowsErr(GetLastError()); From bdfd6babdb1595e198dc24ecca58c94fefe59a68 Mon Sep 17 00:00:00 2001 From: neonene <53406459+neonene@users.noreply.github.com> Date: Sun, 25 Aug 2019 00:40:52 +0900 Subject: [PATCH 02/12] Update 2019-07-29-16-49-31.bpo-37702.Lj2f5e.rst --- .../next/Windows/2019-07-29-16-49-31.bpo-37702.Lj2f5e.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Windows/2019-07-29-16-49-31.bpo-37702.Lj2f5e.rst b/Misc/NEWS.d/next/Windows/2019-07-29-16-49-31.bpo-37702.Lj2f5e.rst index 61fd2a997b9a1d..1b71718efc3430 100644 --- a/Misc/NEWS.d/next/Windows/2019-07-29-16-49-31.bpo-37702.Lj2f5e.rst +++ b/Misc/NEWS.d/next/Windows/2019-07-29-16-49-31.bpo-37702.Lj2f5e.rst @@ -1 +1,2 @@ -Fix memory leak in ssl certification. +Fix memory leak on Windows in creating an SSLContext object or +running urllib.request.urlopen('https://...'). From 5bbf09a6149f36d521d13e88e8bfc26194504a0d Mon Sep 17 00:00:00 2001 From: neonene <53406459+neonene@users.noreply.github.com> Date: Sun, 25 Aug 2019 00:41:22 +0900 Subject: [PATCH 03/12] Update Modules/_ssl.c Co-Authored-By: Ashwin Ramaswami --- Modules/_ssl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_ssl.c b/Modules/_ssl.c index 018c7c091601d6..1502626201d615 100644 --- a/Modules/_ssl.c +++ b/Modules/_ssl.c @@ -5805,7 +5805,7 @@ _ssl_enum_crls_impl(PyObject *module, const char *store_name) with CertCloseStore using CERT_CLOSE_STORE_FORCE_FLAG, the collection store must be closed before its sibling stores. (https://docs.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-certaddstoretocollection) */ - success = CertCloseStore(hCollectionStore, CERT_CLOSE_STORE_FORCE_FLAG); + BOOL success = CertCloseStore(hCollectionStore, CERT_CLOSE_STORE_FORCE_FLAG); for (i = 0; i < system_stores_count; i++) { if (system_store_handles[i] == NULL) { continue; From f8b70e096c5517b2a1ec3ea25df33844f76e0b9e Mon Sep 17 00:00:00 2001 From: neonene <53406459+neonene@users.noreply.github.com> Date: Sun, 25 Aug 2019 00:51:29 +0900 Subject: [PATCH 04/12] Update _ssl.c --- Modules/_ssl.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/Modules/_ssl.c b/Modules/_ssl.c index 1502626201d615..268af231bc2faa 100644 --- a/Modules/_ssl.c +++ b/Modules/_ssl.c @@ -5630,7 +5630,6 @@ _ssl_enum_certificates_impl(PyObject *module, const char *store_name) CERT_SYSTEM_STORE_CURRENT_USER_GROUP_POLICY, CERT_SYSTEM_STORE_SERVICES, CERT_SYSTEM_STORE_USERS}; - BOOL success; size_t i, system_stores_count = sizeof(system_stores) / sizeof(DWORD); HCERTSTORE system_store_handles[sizeof(system_stores) / sizeof(DWORD)]; @@ -5700,7 +5699,7 @@ _ssl_enum_certificates_impl(PyObject *module, const char *store_name) with CertCloseStore using CERT_CLOSE_STORE_FORCE_FLAG, the collection store must be closed before its sibling stores. (https://docs.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-certaddstoretocollection) */ - success = CertCloseStore(hCollectionStore, CERT_CLOSE_STORE_FORCE_FLAG); + BOOL success = CertCloseStore(hCollectionStore, CERT_CLOSE_STORE_FORCE_FLAG); for (i = 0; i < system_stores_count; i++) { if (system_store_handles[i] == NULL) { continue; @@ -5746,7 +5745,6 @@ _ssl_enum_crls_impl(PyObject *module, const char *store_name) CERT_SYSTEM_STORE_CURRENT_USER_GROUP_POLICY, CERT_SYSTEM_STORE_SERVICES, CERT_SYSTEM_STORE_USERS}; - BOOL success; size_t i, system_stores_count = sizeof(system_stores) / sizeof(DWORD); HCERTSTORE system_store_handles[sizeof(system_stores) / sizeof(DWORD)]; From a814e6aa494a1dc41b94a30a3fa9f914f1c504fb Mon Sep 17 00:00:00 2001 From: neonene <53406459+neonene@users.noreply.github.com> Date: Sun, 25 Aug 2019 18:41:33 +0900 Subject: [PATCH 05/12] Update _ssl.c MS doesn't recommend using CERT_CLOSE_STORE_FORCE_FLAG. https://blogs.msdn.microsoft.com/winsdk/2010/01/29/passing-the-flag-cert_close_store_force_flag-to-certclosestore-may-cause-your-application-to-crash/ And I don't want to leave confusing comments on the flag that is no longer necessary to close all handles. So, I decided to change the flag back to 0. --- Modules/_ssl.c | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/Modules/_ssl.c b/Modules/_ssl.c index 268af231bc2faa..f88c8211062990 100644 --- a/Modules/_ssl.c +++ b/Modules/_ssl.c @@ -5580,10 +5580,9 @@ ssl_collect_certificates(const char *store_name, } } if (storesAdded == 0) { - CertCloseStore(hCollectionStore, CERT_CLOSE_STORE_FORCE_FLAG); + CertCloseStore(hCollectionStore, 0); return NULL; } - return hCollectionStore; } @@ -5695,16 +5694,12 @@ _ssl_enum_certificates_impl(PyObject *module, const char *store_name) Py_XDECREF(keyusage); Py_XDECREF(tup); - /* When a collection store and its sibling stores are closed - with CertCloseStore using CERT_CLOSE_STORE_FORCE_FLAG, - the collection store must be closed before its sibling stores. - (https://docs.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-certaddstoretocollection) */ - BOOL success = CertCloseStore(hCollectionStore, CERT_CLOSE_STORE_FORCE_FLAG); + BOOL success = CertCloseStore(hCollectionStore, 0); for (i = 0; i < system_stores_count; i++) { if (system_store_handles[i] == NULL) { continue; } - if (!CertCloseStore(system_store_handles[i], CERT_CLOSE_STORE_FORCE_FLAG)) { + if (!CertCloseStore(system_store_handles[i], 0)) { success = 0; } } @@ -5713,7 +5708,6 @@ _ssl_enum_certificates_impl(PyObject *module, const char *store_name) Py_XDECREF(result); return PyErr_SetFromWindowsErr(GetLastError()); } - return result; } @@ -5799,16 +5793,12 @@ _ssl_enum_crls_impl(PyObject *module, const char *store_name) Py_XDECREF(enc); Py_XDECREF(tup); - /* When a collection store and its sibling stores are closed - with CertCloseStore using CERT_CLOSE_STORE_FORCE_FLAG, - the collection store must be closed before its sibling stores. - (https://docs.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-certaddstoretocollection) */ - BOOL success = CertCloseStore(hCollectionStore, CERT_CLOSE_STORE_FORCE_FLAG); + BOOL success = CertCloseStore(hCollectionStore, 0); for (i = 0; i < system_stores_count; i++) { if (system_store_handles[i] == NULL) { continue; } - if (!CertCloseStore(system_store_handles[i], CERT_CLOSE_STORE_FORCE_FLAG)) { + if (!CertCloseStore(system_store_handles[i], 0)) { success = 0; } } From a62f754332525491953b1791fdbb15286ffc9b00 Mon Sep 17 00:00:00 2001 From: neonene <53406459+neonene@users.noreply.github.com> Date: Mon, 26 Aug 2019 00:32:19 +0900 Subject: [PATCH 06/12] bpo-37702: comment for function call to each handle --- Modules/_ssl.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Modules/_ssl.c b/Modules/_ssl.c index f88c8211062990..e52821f60281db 100644 --- a/Modules/_ssl.c +++ b/Modules/_ssl.c @@ -5694,6 +5694,7 @@ _ssl_enum_certificates_impl(PyObject *module, const char *store_name) Py_XDECREF(keyusage); Py_XDECREF(tup); + /* There's no function argument to close all handles together. (bpo-37702) */ BOOL success = CertCloseStore(hCollectionStore, 0); for (i = 0; i < system_stores_count; i++) { if (system_store_handles[i] == NULL) { @@ -5793,6 +5794,7 @@ _ssl_enum_crls_impl(PyObject *module, const char *store_name) Py_XDECREF(enc); Py_XDECREF(tup); + /* There's no function argument to close all handles together. (bpo-37702) */ BOOL success = CertCloseStore(hCollectionStore, 0); for (i = 0; i < system_stores_count; i++) { if (system_store_handles[i] == NULL) { From f12ab78299779f70e0fad91f4418aef56d30c58f Mon Sep 17 00:00:00 2001 From: neonene <53406459+neonene@users.noreply.github.com> Date: Tue, 27 Aug 2019 06:12:02 +0900 Subject: [PATCH 07/12] close handles in ssl_collect_certificates() --- Modules/_ssl.c | 45 ++++++++++++++++++++++++--------------------- 1 file changed, 24 insertions(+), 21 deletions(-) diff --git a/Modules/_ssl.c b/Modules/_ssl.c index e52821f60281db..e6c8f5494dbf8d 100644 --- a/Modules/_ssl.c +++ b/Modules/_ssl.c @@ -5553,10 +5553,8 @@ ssl_collect_certificates(const char *store_name, * enumerated. The store must be readable to be added to the * store collection. */ - - HCERTSTORE hCollectionStore = NULL, hSystemStore = NULL; + HCERTSTORE hCollectionStore, hSystemStore; size_t i, storesAdded; - BOOL result; hCollectionStore = CertOpenStore(CERT_STORE_PROV_COLLECTION, 0, (HCRYPTPROV)NULL, 0, NULL); @@ -5572,18 +5570,23 @@ ssl_collect_certificates(const char *store_name, system_stores[i], store_name); if (hSystemStore) { system_store_handles[i] = hSystemStore; - result = CertAddStoreToCollection(hCollectionStore, hSystemStore, - CERT_PHYSICAL_STORE_ADD_ENABLE_FLAG, 0); - if (result) { - ++storesAdded; + if (CertAddStoreToCollection(hCollectionStore, hSystemStore, + CERT_PHYSICAL_STORE_ADD_ENABLE_FLAG, 0)) { + storesAdded = 1; } } } - if (storesAdded == 0) { - CertCloseStore(hCollectionStore, 0); - return NULL; + if (storesAdded) { + return hCollectionStore; + } + + CertCloseStore(hCollectionStore, 0); + for (i = 0; i < system_stores_count; i++) { + if (system_store_handles[i]) { + CertCloseStore(system_store_handles[i], 0); + } } - return hCollectionStore; + return NULL; } /* code from Objects/listobject.c */ @@ -5694,13 +5697,13 @@ _ssl_enum_certificates_impl(PyObject *module, const char *store_name) Py_XDECREF(keyusage); Py_XDECREF(tup); - /* There's no function argument to close all handles together. (bpo-37702) */ + /* CertCloseStore() has no option to close all handles together. + And if CERT_CLOSE_STORE_FORCE_FLAG (not recommended) is used, + hCollectionStore must be closed before hSystemStore. (bpo-37702) */ BOOL success = CertCloseStore(hCollectionStore, 0); for (i = 0; i < system_stores_count; i++) { - if (system_store_handles[i] == NULL) { - continue; - } - if (!CertCloseStore(system_store_handles[i], 0)) { + if (system_store_handles[i] && + !CertCloseStore(system_store_handles[i], 0)) { success = 0; } } @@ -5794,13 +5797,13 @@ _ssl_enum_crls_impl(PyObject *module, const char *store_name) Py_XDECREF(enc); Py_XDECREF(tup); - /* There's no function argument to close all handles together. (bpo-37702) */ + /* CertCloseStore() has no option to close all handles together. + And if CERT_CLOSE_STORE_FORCE_FLAG (not recommended) is used, + hCollectionStore must be closed before hSystemStore. (bpo-37702) */ BOOL success = CertCloseStore(hCollectionStore, 0); for (i = 0; i < system_stores_count; i++) { - if (system_store_handles[i] == NULL) { - continue; - } - if (!CertCloseStore(system_store_handles[i], 0)) { + if (system_store_handles[i] && + !CertCloseStore(system_store_handles[i], 0)) { success = 0; } } From 5e079bf2c2b82f817629c8799f1f35136fb5f26c Mon Sep 17 00:00:00 2001 From: neonene <53406459+neonene@users.noreply.github.com> Date: Tue, 27 Aug 2019 08:17:27 +0900 Subject: [PATCH 08/12] comments (correct english) --- Modules/_ssl.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/Modules/_ssl.c b/Modules/_ssl.c index e6c8f5494dbf8d..caf0bb555bdbc3 100644 --- a/Modules/_ssl.c +++ b/Modules/_ssl.c @@ -5697,9 +5697,9 @@ _ssl_enum_certificates_impl(PyObject *module, const char *store_name) Py_XDECREF(keyusage); Py_XDECREF(tup); - /* CertCloseStore() has no option to close all handles together. - And if CERT_CLOSE_STORE_FORCE_FLAG (not recommended) is used, - hCollectionStore must be closed before hSystemStore. (bpo-37702) */ + /* CertCloseStore() can't close handles together with any argument. + In case of using CERT_CLOSE_STORE_FORCE_FLAG (not recommended), + we must close hCollectionStore before hSystemStore. (bpo-37702) */ BOOL success = CertCloseStore(hCollectionStore, 0); for (i = 0; i < system_stores_count; i++) { if (system_store_handles[i] && @@ -5797,9 +5797,9 @@ _ssl_enum_crls_impl(PyObject *module, const char *store_name) Py_XDECREF(enc); Py_XDECREF(tup); - /* CertCloseStore() has no option to close all handles together. - And if CERT_CLOSE_STORE_FORCE_FLAG (not recommended) is used, - hCollectionStore must be closed before hSystemStore. (bpo-37702) */ + /* CertCloseStore() can't close handles together with any argument. + In case of using CERT_CLOSE_STORE_FORCE_FLAG (not recommended), + we must close hCollectionStore before hSystemStore. (bpo-37702) */ BOOL success = CertCloseStore(hCollectionStore, 0); for (i = 0; i < system_stores_count; i++) { if (system_store_handles[i] && From 2963e039a2fde2cab0e29f7d3dc1403737dbce73 Mon Sep 17 00:00:00 2001 From: neonene <53406459+neonene@users.noreply.github.com> Date: Tue, 27 Aug 2019 12:45:25 +0900 Subject: [PATCH 09/12] indent, comments (correct english) --- Modules/_ssl.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/Modules/_ssl.c b/Modules/_ssl.c index caf0bb555bdbc3..56676ef1065be2 100644 --- a/Modules/_ssl.c +++ b/Modules/_ssl.c @@ -5697,13 +5697,13 @@ _ssl_enum_certificates_impl(PyObject *module, const char *store_name) Py_XDECREF(keyusage); Py_XDECREF(tup); - /* CertCloseStore() can't close handles together with any argument. + /* CertCloseStore() with any argument can't close handles together. In case of using CERT_CLOSE_STORE_FORCE_FLAG (not recommended), - we must close hCollectionStore before hSystemStore. (bpo-37702) */ + we must close hCollectionStore before we close each hSystemStore. */ BOOL success = CertCloseStore(hCollectionStore, 0); for (i = 0; i < system_stores_count; i++) { if (system_store_handles[i] && - !CertCloseStore(system_store_handles[i], 0)) { + !CertCloseStore(system_store_handles[i], 0)) { success = 0; } } @@ -5797,13 +5797,13 @@ _ssl_enum_crls_impl(PyObject *module, const char *store_name) Py_XDECREF(enc); Py_XDECREF(tup); - /* CertCloseStore() can't close handles together with any argument. + /* CertCloseStore() with any argument can't close handles together. In case of using CERT_CLOSE_STORE_FORCE_FLAG (not recommended), - we must close hCollectionStore before hSystemStore. (bpo-37702) */ + we must close hCollectionStore before we close each hSystemStore. */ BOOL success = CertCloseStore(hCollectionStore, 0); for (i = 0; i < system_stores_count; i++) { if (system_store_handles[i] && - !CertCloseStore(system_store_handles[i], 0)) { + !CertCloseStore(system_store_handles[i], 0)) { success = 0; } } From e47aa0443e000d057be04980bf58e2b8a0782a26 Mon Sep 17 00:00:00 2001 From: neonene <53406459+neonene@users.noreply.github.com> Date: Wed, 28 Aug 2019 07:26:19 +0900 Subject: [PATCH 10/12] make the system store list a global static --- Modules/_ssl.c | 69 ++++++++++++++++++++------------------------------ 1 file changed, 28 insertions(+), 41 deletions(-) diff --git a/Modules/_ssl.c b/Modules/_ssl.c index 56676ef1065be2..2267829fcab735 100644 --- a/Modules/_ssl.c +++ b/Modules/_ssl.c @@ -5543,9 +5543,21 @@ parseKeyUsage(PCCERT_CONTEXT pCertCtx, DWORD flags) return retval; } +static DWORD system_stores[] = { + CERT_SYSTEM_STORE_LOCAL_MACHINE, + CERT_SYSTEM_STORE_LOCAL_MACHINE_ENTERPRISE, + CERT_SYSTEM_STORE_LOCAL_MACHINE_GROUP_POLICY, + CERT_SYSTEM_STORE_CURRENT_USER, + CERT_SYSTEM_STORE_CURRENT_USER_GROUP_POLICY, + CERT_SYSTEM_STORE_SERVICES, + CERT_SYSTEM_STORE_USERS +}; + +#define SYSTEM_STORES_COUNT sizeof(system_stores) / sizeof(DWORD) + + static HCERTSTORE ssl_collect_certificates(const char *store_name, - DWORD *system_stores, size_t system_stores_count, HCERTSTORE *system_store_handles) { /* this function collects the system certificate stores listed in @@ -5561,8 +5573,9 @@ ssl_collect_certificates(const char *store_name, if (!hCollectionStore) { return NULL; } + storesAdded = 0; - for (i = 0; i < system_stores_count; i++) { + for (i = 0; i < SYSTEM_STORES_COUNT; i++) { system_store_handles[i] = NULL; hSystemStore = CertOpenStore(CERT_STORE_PROV_SYSTEM_A, 0, (HCRYPTPROV)NULL, @@ -5581,7 +5594,7 @@ ssl_collect_certificates(const char *store_name, } CertCloseStore(hCollectionStore, 0); - for (i = 0; i < system_stores_count; i++) { + for (i = 0; i < SYSTEM_STORES_COUNT; i++) { if (system_store_handles[i]) { CertCloseStore(system_store_handles[i], 0); } @@ -5620,29 +5633,19 @@ static PyObject * _ssl_enum_certificates_impl(PyObject *module, const char *store_name) /*[clinic end generated code: output=5134dc8bb3a3c893 input=915f60d70461ea4e]*/ { - HCERTSTORE hCollectionStore = NULL; + HCERTSTORE system_store_handles[SYSTEM_STORES_COUNT]; PCCERT_CONTEXT pCertCtx = NULL; PyObject *keyusage = NULL, *cert = NULL, *enc = NULL, *tup = NULL; PyObject *result = NULL; - static DWORD system_stores[] = { - CERT_SYSTEM_STORE_LOCAL_MACHINE, - CERT_SYSTEM_STORE_LOCAL_MACHINE_ENTERPRISE, - CERT_SYSTEM_STORE_LOCAL_MACHINE_GROUP_POLICY, - CERT_SYSTEM_STORE_CURRENT_USER, - CERT_SYSTEM_STORE_CURRENT_USER_GROUP_POLICY, - CERT_SYSTEM_STORE_SERVICES, - CERT_SYSTEM_STORE_USERS}; - size_t i, system_stores_count = sizeof(system_stores) / sizeof(DWORD); - HCERTSTORE system_store_handles[sizeof(system_stores) / sizeof(DWORD)]; + size_t i; result = PyList_New(0); if (result == NULL) { return NULL; } - hCollectionStore = ssl_collect_certificates(store_name, - system_stores, - system_stores_count, - system_store_handles); + + HCERTSTORE hCollectionStore = ssl_collect_certificates(store_name, + system_store_handles); if (hCollectionStore == NULL) { Py_DECREF(result); return PyErr_SetFromWindowsErr(GetLastError()); @@ -5697,11 +5700,8 @@ _ssl_enum_certificates_impl(PyObject *module, const char *store_name) Py_XDECREF(keyusage); Py_XDECREF(tup); - /* CertCloseStore() with any argument can't close handles together. - In case of using CERT_CLOSE_STORE_FORCE_FLAG (not recommended), - we must close hCollectionStore before we close each hSystemStore. */ BOOL success = CertCloseStore(hCollectionStore, 0); - for (i = 0; i < system_stores_count; i++) { + for (i = 0; i < SYSTEM_STORES_COUNT; i++) { if (system_store_handles[i] && !CertCloseStore(system_store_handles[i], 0)) { success = 0; @@ -5731,29 +5731,19 @@ static PyObject * _ssl_enum_crls_impl(PyObject *module, const char *store_name) /*[clinic end generated code: output=bce467f60ccd03b6 input=a1f1d7629f1c5d3d]*/ { - HCERTSTORE hCollectionStore = NULL; + HCERTSTORE system_store_handles[SYSTEM_STORES_COUNT]; PCCRL_CONTEXT pCrlCtx = NULL; PyObject *crl = NULL, *enc = NULL, *tup = NULL; PyObject *result = NULL; - static DWORD system_stores[] = { - CERT_SYSTEM_STORE_LOCAL_MACHINE, - CERT_SYSTEM_STORE_LOCAL_MACHINE_ENTERPRISE, - CERT_SYSTEM_STORE_LOCAL_MACHINE_GROUP_POLICY, - CERT_SYSTEM_STORE_CURRENT_USER, - CERT_SYSTEM_STORE_CURRENT_USER_GROUP_POLICY, - CERT_SYSTEM_STORE_SERVICES, - CERT_SYSTEM_STORE_USERS}; - size_t i, system_stores_count = sizeof(system_stores) / sizeof(DWORD); - HCERTSTORE system_store_handles[sizeof(system_stores) / sizeof(DWORD)]; + size_t i; result = PyList_New(0); if (result == NULL) { return NULL; } - hCollectionStore = ssl_collect_certificates(store_name, - system_stores, - system_stores_count, - system_store_handles); + + HCERTSTORE hCollectionStore = ssl_collect_certificates(store_name, + system_store_handles); if (hCollectionStore == NULL) { Py_DECREF(result); return PyErr_SetFromWindowsErr(GetLastError()); @@ -5797,11 +5787,8 @@ _ssl_enum_crls_impl(PyObject *module, const char *store_name) Py_XDECREF(enc); Py_XDECREF(tup); - /* CertCloseStore() with any argument can't close handles together. - In case of using CERT_CLOSE_STORE_FORCE_FLAG (not recommended), - we must close hCollectionStore before we close each hSystemStore. */ BOOL success = CertCloseStore(hCollectionStore, 0); - for (i = 0; i < system_stores_count; i++) { + for (i = 0; i < SYSTEM_STORES_COUNT; i++) { if (system_store_handles[i] && !CertCloseStore(system_store_handles[i], 0)) { success = 0; From 9c09ed72568efcd6f0caad96707dc108b2c46179 Mon Sep 17 00:00:00 2001 From: neonene <53406459+neonene@users.noreply.github.com> Date: Wed, 28 Aug 2019 17:09:41 +0900 Subject: [PATCH 11/12] surround #define replacement with parenthesis Not necessary for now. --- Modules/_ssl.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Modules/_ssl.c b/Modules/_ssl.c index 2267829fcab735..1050aaf79a24ae 100644 --- a/Modules/_ssl.c +++ b/Modules/_ssl.c @@ -5553,8 +5553,7 @@ static DWORD system_stores[] = { CERT_SYSTEM_STORE_USERS }; -#define SYSTEM_STORES_COUNT sizeof(system_stores) / sizeof(DWORD) - +#define SYSTEM_STORES_COUNT (sizeof(system_stores) / sizeof(DWORD)) static HCERTSTORE ssl_collect_certificates(const char *store_name, From b03a5537c46b1e3a5c04b00cc0b98e3e215eb8d6 Mon Sep 17 00:00:00 2001 From: neonene <53406459+neonene@users.noreply.github.com> Date: Thu, 29 Aug 2019 23:11:20 +0900 Subject: [PATCH 12/12] add a function to close handles --- Modules/_ssl.c | 62 ++++++++++++++++++++------------------------------ 1 file changed, 25 insertions(+), 37 deletions(-) diff --git a/Modules/_ssl.c b/Modules/_ssl.c index 1050aaf79a24ae..af6b669d26017d 100644 --- a/Modules/_ssl.c +++ b/Modules/_ssl.c @@ -5555,9 +5555,22 @@ static DWORD system_stores[] = { #define SYSTEM_STORES_COUNT (sizeof(system_stores) / sizeof(DWORD)) +static BOOL +cert_close_stores(HCERTSTORE hCollectionStore, HCERTSTORE *system_store_handles) +{ + size_t i; + BOOL success = CertCloseStore(hCollectionStore, 0); + for (i = 0; i < SYSTEM_STORES_COUNT; i++) { + if (system_store_handles[i] && + !CertCloseStore(system_store_handles[i], 0)) { + success = 0; + } + } + return success; +} + static HCERTSTORE -ssl_collect_certificates(const char *store_name, - HCERTSTORE *system_store_handles) +ssl_collect_certificates(const char *store_name, HCERTSTORE *system_store_handles) { /* this function collects the system certificate stores listed in * system_stores into a collection certificate store for being @@ -5572,7 +5585,6 @@ ssl_collect_certificates(const char *store_name, if (!hCollectionStore) { return NULL; } - storesAdded = 0; for (i = 0; i < SYSTEM_STORES_COUNT; i++) { system_store_handles[i] = NULL; @@ -5588,17 +5600,11 @@ ssl_collect_certificates(const char *store_name, } } } - if (storesAdded) { - return hCollectionStore; - } - - CertCloseStore(hCollectionStore, 0); - for (i = 0; i < SYSTEM_STORES_COUNT; i++) { - if (system_store_handles[i]) { - CertCloseStore(system_store_handles[i], 0); - } + if (!storesAdded) { + cert_close_stores(hCollectionStore, system_store_handles); + return NULL; } - return NULL; + return hCollectionStore; } /* code from Objects/listobject.c */ @@ -5632,19 +5638,17 @@ static PyObject * _ssl_enum_certificates_impl(PyObject *module, const char *store_name) /*[clinic end generated code: output=5134dc8bb3a3c893 input=915f60d70461ea4e]*/ { + HCERTSTORE hCollectionStore; HCERTSTORE system_store_handles[SYSTEM_STORES_COUNT]; PCCERT_CONTEXT pCertCtx = NULL; PyObject *keyusage = NULL, *cert = NULL, *enc = NULL, *tup = NULL; PyObject *result = NULL; - size_t i; result = PyList_New(0); if (result == NULL) { return NULL; } - - HCERTSTORE hCollectionStore = ssl_collect_certificates(store_name, - system_store_handles); + hCollectionStore = ssl_collect_certificates(store_name, system_store_handles); if (hCollectionStore == NULL) { Py_DECREF(result); return PyErr_SetFromWindowsErr(GetLastError()); @@ -5699,14 +5703,7 @@ _ssl_enum_certificates_impl(PyObject *module, const char *store_name) Py_XDECREF(keyusage); Py_XDECREF(tup); - BOOL success = CertCloseStore(hCollectionStore, 0); - for (i = 0; i < SYSTEM_STORES_COUNT; i++) { - if (system_store_handles[i] && - !CertCloseStore(system_store_handles[i], 0)) { - success = 0; - } - } - if (!success) { + if (!cert_close_stores(hCollectionStore, system_store_handles)) { /* This error case might shadow another exception.*/ Py_XDECREF(result); return PyErr_SetFromWindowsErr(GetLastError()); @@ -5730,19 +5727,17 @@ static PyObject * _ssl_enum_crls_impl(PyObject *module, const char *store_name) /*[clinic end generated code: output=bce467f60ccd03b6 input=a1f1d7629f1c5d3d]*/ { + HCERTSTORE hCollectionStore; HCERTSTORE system_store_handles[SYSTEM_STORES_COUNT]; PCCRL_CONTEXT pCrlCtx = NULL; PyObject *crl = NULL, *enc = NULL, *tup = NULL; PyObject *result = NULL; - size_t i; result = PyList_New(0); if (result == NULL) { return NULL; } - - HCERTSTORE hCollectionStore = ssl_collect_certificates(store_name, - system_store_handles); + hCollectionStore = ssl_collect_certificates(store_name, system_store_handles); if (hCollectionStore == NULL) { Py_DECREF(result); return PyErr_SetFromWindowsErr(GetLastError()); @@ -5786,14 +5781,7 @@ _ssl_enum_crls_impl(PyObject *module, const char *store_name) Py_XDECREF(enc); Py_XDECREF(tup); - BOOL success = CertCloseStore(hCollectionStore, 0); - for (i = 0; i < SYSTEM_STORES_COUNT; i++) { - if (system_store_handles[i] && - !CertCloseStore(system_store_handles[i], 0)) { - success = 0; - } - } - if (!success) { + if (!cert_close_stores(hCollectionStore, system_store_handles)) { /* This error case might shadow another exception.*/ Py_XDECREF(result); return PyErr_SetFromWindowsErr(GetLastError());