Skip to content

Make where() materialise the CA bundle exactly once - #433

Open
Meet-1010 wants to merge 1 commit into
certifi:masterfrom
Meet-1010:thread-safe-where
Open

Make where() materialise the CA bundle exactly once#433
Meet-1010 wants to merge 1 commit into
certifi:masterfrom
Meet-1010:thread-safe-where

Conversation

@Meet-1010

Copy link
Copy Markdown

The problem

where() materialises the CA bundle lazily:

global _CACERT_CTX
global _CACERT_PATH
if _CACERT_PATH is None:
    _CACERT_CTX = as_file(files("certifi").joinpath("cacert.pem"))
    _CACERT_PATH = str(_CACERT_CTX.__enter__())
    atexit.register(exit_cacert_ctx)

return _CACERT_PATH

The is None test and the assignment that publishes the path are separate operations. Concurrent callers can all pass the test; each opens its own context manager and registers its own atexit hook, but only the last assignment to _CACERT_CTX survives, so the earlier context managers are orphaned — nothing holds them, and exit_cacert_ctx runs once per extra registration against whichever context happens to be current.

In the common filesystem install as_file returns the real path and __exit__ is a no-op, so the damage is bounded to redundant atexit registrations. When certifi is imported from a zip or wheel, as_file extracts a temporary file and __exit__ deletes it — then an orphaned context is a temp file nobody cleans up, and the duplicate hooks can delete a path another thread is still holding.

This is not free-threading-specific

It reproduces with the GIL enabled too — the GIL just hid it after the first call warmed the cache. Measured with 16 threads over 200 trials on CPython 3.14.4:

trials where the context was entered more than once
free-threaded, GIL disabled 200/200
same interpreter, PYTHON_GIL=1 1/200 (the very first, cold, call)

So this is a general thread-safety bug that free-threading makes reliable rather than occasional.

The change

Double-checked locking, with _CACERT_PATH published last so no thread can observe a path whose context manager is not yet reachable for cleanup. Both the sys.version_info >= (3, 11) branch and the fallback branch are patched.

Tests

TestWhereThreadSafety in certifi/tests/test_certify.py counts entries into the resource context by temporarily replacing certifi.core.atexit.register with a counter, and asserts exactly one per cold start. It fails 5/5 runs without the change and passes with it.

It waits on an Event rather than a Barrier and skips if fewer than two threads can be started, so a thread-constrained runner degrades rather than deadlocks. All globals and the patched register are restored in a finally.

Full suite: 4 passed, both free-threaded and with PYTHON_GIL=1.

Environment: CPython 3.14.4 free-threaded, macOS 15 / arm64.

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.
)


class TestWhereThreadSafety(unittest.TestCase):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test involves so much monkeypatching that I'm personally pretty skeptical of its usefulness.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can probably drop teh test entirely tbh

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants