Make where() materialise the CA bundle exactly once

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
 

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