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