From 5d492b71915a77943c07c50153db942b8d139d80 Mon Sep 17 00:00:00 2001 From: Meet Chauhan <133805065+Meet-1010@users.noreply.github.com> Date: Tue, 25 Aug 2026 16:07:28 +0530 Subject: [PATCH] Make where() materialise the CA bundle exactly once The 'if _CACERT_PATH is None' test and the assignment that publishes the path are separate operations. Concurrent callers could each enter their own resource context and register their own atexit hook, but only the last _CACERT_CTX assignment survived, orphaning the earlier context managers. Under zipimport as_file() extracts a temporary file, so an orphan is a file nothing will clean up, and the duplicate atexit hooks can close a context whose path another thread is still holding. Use double-checked locking and publish _CACERT_PATH last, so no thread can observe a path whose context manager is not yet reachable for cleanup. Both the importlib.resources branch and the fallback branch are covered. The new test fails 5/5 runs without this change and passes with it. --- certifi/core.py | 76 ++++++++++++++++++++++------------- certifi/tests/test_certify.py | 76 +++++++++++++++++++++++++++++++++++ 2 files changed, 125 insertions(+), 27 deletions(-) diff --git a/certifi/core.py b/certifi/core.py index 1c9661cc..e83593e5 100644 --- a/certifi/core.py +++ b/certifi/core.py @@ -6,6 +6,14 @@ """ import sys import atexit +import threading + +# where() lazily materialises the CA bundle and publishes it through module +# globals. The `is None` test and the assignment are separate operations, so +# without this lock several threads can each enter the resource context and +# register their own atexit hook while only the last _CACERT_CTX assignment +# survives, orphaning the others. +_CACERT_LOCK = threading.Lock() def exit_cacert_ctx() -> None: _CACERT_CTX.__exit__(None, None, None) # type: ignore[union-attr] @@ -27,19 +35,26 @@ def where() -> str: global _CACERT_CTX global _CACERT_PATH if _CACERT_PATH is None: - # This is slightly janky, the importlib.resources API wants you to - # manage the cleanup of this file, so it doesn't actually return a - # path, it returns a context manager that will give you the path - # when you enter it and will do any cleanup when you leave it. In - # the common case of not needing a temporary file, it will just - # return the file system location and the __exit__() is a no-op. - # - # We also have to hold onto the actual context manager, because - # it will do the cleanup whenever it gets garbage collected, so - # we will also store that at the global level as well. - _CACERT_CTX = as_file(files("certifi").joinpath("cacert.pem")) - _CACERT_PATH = str(_CACERT_CTX.__enter__()) - atexit.register(exit_cacert_ctx) + with _CACERT_LOCK: + if _CACERT_PATH is not None: + return _CACERT_PATH + # This is slightly janky, the importlib.resources API wants you to + # manage the cleanup of this file, so it doesn't actually return a + # path, it returns a context manager that will give you the path + # when you enter it and will do any cleanup when you leave it. In + # the common case of not needing a temporary file, it will just + # return the file system location and the __exit__() is a no-op. + # + # We also have to hold onto the actual context manager, because + # it will do the cleanup whenever it gets garbage collected, so + # we will also store that at the global level as well. + ctx = as_file(files("certifi").joinpath("cacert.pem")) + path = str(ctx.__enter__()) + _CACERT_CTX = ctx + atexit.register(exit_cacert_ctx) + # published last, so no thread can observe a path whose + # context manager is not yet reachable for cleanup + _CACERT_PATH = path return _CACERT_PATH @@ -62,20 +77,27 @@ def where() -> str: global _CACERT_CTX global _CACERT_PATH if _CACERT_PATH is None: - # This is slightly janky, the importlib.resources API wants you - # to manage the cleanup of this file, so it doesn't actually - # return a path, it returns a context manager that will give - # you the path when you enter it and will do any cleanup when - # you leave it. In the common case of not needing a temporary - # file, it will just return the file system location and the - # __exit__() is a no-op. - # - # We also have to hold onto the actual context manager, because - # it will do the cleanup whenever it gets garbage collected, so - # we will also store that at the global level as well. - _CACERT_CTX = get_path("certifi", "cacert.pem") - _CACERT_PATH = str(_CACERT_CTX.__enter__()) - atexit.register(exit_cacert_ctx) + with _CACERT_LOCK: + if _CACERT_PATH is not None: + return _CACERT_PATH + # This is slightly janky, the importlib.resources API wants you + # to manage the cleanup of this file, so it doesn't actually + # return a path, it returns a context manager that will give + # you the path when you enter it and will do any cleanup when + # you leave it. In the common case of not needing a temporary + # file, it will just return the file system location and the + # __exit__() is a no-op. + # + # We also have to hold onto the actual context manager, because + # it will do the cleanup whenever it gets garbage collected, so + # we will also store that at the global level as well. + ctx = get_path("certifi", "cacert.pem") + path = str(ctx.__enter__()) + _CACERT_CTX = ctx + atexit.register(exit_cacert_ctx) + # published last, so no thread can observe a path whose + # context manager is not yet reachable for cleanup + _CACERT_PATH = path return _CACERT_PATH diff --git a/certifi/tests/test_certify.py b/certifi/tests/test_certify.py index 54670eae..c03639a9 100755 --- a/certifi/tests/test_certify.py +++ b/certifi/tests/test_certify.py @@ -1,7 +1,9 @@ import os +import threading import unittest import certifi +import certifi.core class TestCertifi(unittest.TestCase): @@ -16,3 +18,77 @@ def test_py_typed_exists(self) -> None: assert os.path.exists( os.path.join(os.path.dirname(certifi.__file__), 'py.typed') ) + + +class TestWhereThreadSafety(unittest.TestCase): + """where() must materialise the CA bundle exactly once. + + The `if _CACERT_PATH is None` test and the assignment that publishes the + path are separate operations, so concurrent callers could each enter their + own resource context and register their own atexit hook while only the + last _CACERT_CTX assignment survived, orphaning the others. Under + zipimport as_file() extracts a temporary file, so an orphaned context is a + file nothing will clean up. + """ + + def test_where_enters_the_resource_context_once(self) -> None: + core = certifi.core + if not hasattr(core, "_CACERT_PATH"): + self.skipTest("this build of certifi does not lazily materialise the bundle") + + original = (core._CACERT_PATH, core._CACERT_CTX, core.atexit.register) + registrations = [] + lock = threading.Lock() + + def counting_register(func, *args, **kwargs): + # Count rather than call through, so the test does not pile up real + # atexit hooks. + with lock: + registrations.append(func) + + n_threads = 4 + go = threading.Event() + results: list[str] = [] + + try: + core.atexit.register = counting_register + + for _ in range(50): + core._CACERT_PATH = None + core._CACERT_CTX = None + registrations.clear() + results.clear() + + def worker() -> None: + # An Event rather than a Barrier, so a runner that can only + # give us some of the threads still runs. + go.wait() + path = core.where() + with lock: + results.append(path) + + threads = [] + for _ in range(n_threads): + thread = threading.Thread(target=worker) + try: + thread.start() + except RuntimeError: + break + threads.append(thread) + + if len(threads) < 2: + go.set() + for thread in threads: + thread.join() + self.skipTest("could not start enough threads to test for the race") + + go.set() + for thread in threads: + thread.join() + go.clear() + + self.assertEqual(len(registrations), 1) + self.assertEqual(len(set(results)), 1) + finally: + go.set() + core._CACERT_PATH, core._CACERT_CTX, core.atexit.register = original