gh-149816: fix a race when fetching OpenSSL digests

[picnixz]

@devdanzin Your didn't include a reproducer for this issue so could you actually check whether this fix would be suitable. I'm not a FT expert but the problem was essentially a race ref leak right?

• Issue: 22 free-threading race conditions #149816

[devdanzin]

That's not one of mine, but I had a go at reproducing it. Needed an instrumented build to log leaks, as neither TSan, LSan or ASan could see the leak. The leak reproduces without the fix and stops with the fix applied, that should be enough confirmation.

Claude notes: "the old code had a second, latent bug the fix also closes — in the (always-taken) same-pointer case the losing thread skipped up_ref, so its caller received a digest with no reference added (borrowed-as-owned → potential over-free/UAF if the cache entry were ever cleared). The fix's unconditional up_ref fixes that too.".

Below is the patch, reproducer for patched build and README that Claude came up with to test the fix.

---

diff --git a/Modules/_hashopenssl.c b/Modules/_hashopenssl.c
index f895c903748..434c3ff231c 100644
--- a/Modules/_hashopenssl.c
+++ b/Modules/_hashopenssl.c
@@ -688,6 +688,22 @@ disable_fips_property(Py_hash_type py_ht)
  *
  * If 'name' is an OpenSSL indexed name, the return value is cached.
  */
+#ifdef HASHLIB_EVP_RACE_COUNTER
+/*
+ * Opt-in diagnostic counter for gh-149816 finding (21) / gh-153019.
+ *
+ * Build with CFLAGS=-DHASHLIB_EVP_RACE_COUNTER to enable. Counts the number of
+ * threads that fetched a fresh EVP_MD but lost the race to publish it into the
+ * per-entry cache. On the buggy (pre-gh-153019) code below, each such event is
+ * one LEAKED EVP_MD reference (the overwritten `other_digest` is never freed).
+ *
+ * Exposed to Python as _hashlib._evp_md_race_events() /
+ * _hashlib._evp_md_race_events_reset(). No effect unless the macro is defined,
+ * and a no-op on GIL-enabled builds (the race cannot occur there).
+ */
+static int _evp_md_race_events = 0;
+#endif
+
 static PY_EVP_MD *
 get_openssl_evp_md_by_utf8name(_hashlibstate *state, const char *name,
                                Py_hash_type py_ht)
@@ -722,6 +738,14 @@ get_openssl_evp_md_by_utf8name(_hashlibstate *state, const char *name,
         }
         // if another thread same thing at same time make sure we got same ptr
         assert(other_digest == NULL || other_digest == digest);
+#ifdef HASHLIB_EVP_RACE_COUNTER
+        if (other_digest != NULL) {
+            /* Race loser: this thread's freshly fetched digest was overwritten
+             * in the cache and its reference (other_digest) is never freed —
+             * one leaked EVP_MD reference. (gh-149816 finding 21.) */
+            _Py_atomic_add_int(&_evp_md_race_events, 1);
+        }
+#endif
         if (digest != NULL && other_digest == NULL) {
             PY_EVP_MD_up_ref(digest);
         }
@@ -2682,7 +2706,29 @@ _hashlib_compare_digest_impl(PyObject *module, PyObject *a, PyObject *b)
 
 /* List of functions exported by this module */
 
+#ifdef HASHLIB_EVP_RACE_COUNTER
+static PyObject *
+_hashlib_evp_md_race_events(PyObject *module, PyObject *Py_UNUSED(ignored))
+{
+    return PyLong_FromLong(_Py_atomic_load_int(&_evp_md_race_events));
+}
+
+static PyObject *
+_hashlib_evp_md_race_events_reset(PyObject *module, PyObject *Py_UNUSED(ignored))
+{
+    _Py_atomic_store_int(&_evp_md_race_events, 0);
+    Py_RETURN_NONE;
+}
+#endif
+
 static struct PyMethodDef EVP_functions[] = {
+#ifdef HASHLIB_EVP_RACE_COUNTER
+    {"_evp_md_race_events", _hashlib_evp_md_race_events, METH_NOARGS,
+     PyDoc_STR("Number of EVP_MD cache publish race-losers observed "
+               "(gh-149816 finding 21; each is one leaked ref on the buggy build).")},
+    {"_evp_md_race_events_reset", _hashlib_evp_md_race_events_reset, METH_NOARGS,
+     PyDoc_STR("Reset the EVP_MD race-loser counter to zero.")},
+#endif
     _HASHLIB_HASH_NEW_METHODDEF
     PBKDF2_HMAC_METHODDEF
     _HASHLIB_SCRYPT_METHODDEF

"""Reproducer for gh-149816 finding (21): "Racy EVP_MD cache overwrite leaks
references in Modules/_hashopenssl.c" — the bug fixed by PR gh-153019.

Buggy code (get_openssl_evp_md_by_utf8name, pre-fix):

    digest = FT_ATOMIC_LOAD_PTR_RELAXED(entry->evp);   // slot starts NULL
    if (digest == NULL) {
        digest = PY_EVP_MD_fetch(entry->ossl_name, NULL);        // slow fetch, +1
        other_digest = _Py_atomic_exchange_ptr(&entry->evp, digest);
    }
    ...
    assert(other_digest == NULL || other_digest == digest);
    if (digest != NULL && other_digest == NULL)
        PY_EVP_MD_up_ref(digest);

The race window is the whole EVP_MD_fetch() call. If several threads hit the
FIRST use of an uncached digest simultaneously, they all read the slot as NULL,
all fetch, and all atomic-exchange. Every store past the first overwrites (and
leaks) the previous thread's freshly fetched digest: `other_digest` is that
extra reference, and it is never freed.

OpenSSL's method store makes EVP_MD_fetch return the *same* refcounted pointer
across threads, so `other_digest == digest` (the assert never fires) and the
leak is a refcount inflation on a still-reachable object — invisible to TSan,
ASan, and LSan. That is why this finding had no reproducer. To observe it, build
_hashopenssl.c with -DHASHLIB_EVP_RACE_COUNTER (see hashlib_evp_race_counter.patch),
which exposes _hashlib._evp_md_race_events().

Requires: free-threaded build, OpenSSL 3.0+. Under PYTHON_GIL=1 / -X gil=1 the
race cannot occur and the counter stays 0.

Usage:
    ./python repro_ft_hashopenssl_race.py           # free-threaded: race events > 0
    ./python -X gil=1 repro_ft_hashopenssl_race.py  # serialized:    race events == 0
"""
import sys
import threading

import _hashlib  # use the C module directly; don't let hashlib pre-touch slots

# Names backed by entries in _hashopenssl.c's py_hashes[] table (each has two
# lazily-filled cache slots: evp [usedforsecurity=True] and evp_nosecurity).
NAMES = [
    "md5", "sha1", "sha224", "sha256", "sha384", "sha512",
    "sha512_224", "sha512_256",
    "sha3_224", "sha3_256", "sha3_384", "sha3_512",
    "shake_128", "shake_256", "blake2s", "blake2b",
]

NTHREADS = 64

HAS_COUNTER = hasattr(_hashlib, "_evp_md_race_events")
print("gil_enabled:", sys._is_gil_enabled(),
      "| race counter available:", HAS_COUNTER, flush=True)


# A spin barrier releases all threads within a cache-coherency delay of each
# other — far tighter than threading.Barrier's condition-variable wakeup, which
# is too staggered relative to the ~microsecond EVP_MD_fetch window.
def hammer(name, used_for_security, ready, go):
    with ready[1]:
        ready[0] += 1
    while not go[0]:  # spin until main flips the flag — near-simultaneous release
        pass
    try:
        _hashlib.new(name, b"", usedforsecurity=used_for_security)
    except ValueError:
        pass  # digest not provided by this OpenSSL build


def main():
    if HAS_COUNTER:
        _hashlib._evp_md_race_events_reset()

    windows = 0
    for name in NAMES:
        for ufs in (True, False):
            go = [False]
            ready = [0, threading.Lock()]
            ts = [
                threading.Thread(target=hammer, args=(name, ufs, ready, go))
                for _ in range(NTHREADS)
            ]
            for t in ts:
                t.start()
            while ready[0] < NTHREADS:  # wait until every thread is spinning
                pass
            go[0] = True               # release them all at once
            for t in ts:
                t.join()
            windows += 1

    print(f"done: {windows} first-touch race windows", flush=True)
    if HAS_COUNTER:
        n = _hashlib._evp_md_race_events()
        kind = "LEAKED EVP_MD refs" if n else "race events"
        print(f"race-losers: {n}  "
              f"({'BUG: ' + str(n) + ' ' + kind if n else 'none — no leak'})",
              flush=True)
        # Non-zero on a free-threaded *buggy* build; 0 when serialized or fixed.
        sys.exit(1 if n else 0)


main()

EVP_MD cache race-leak counter (gh-149816 finding 21 / gh-153019)

A reusable, opt-in instrumentation for demonstrating and verifying the fix for
the racy EVP_MD cache overwrite in Modules/_hashopenssl.c
(get_openssl_evp_md_by_utf8name).

Why it's needed

When two threads first-touch an uncached digest slot on a free-threaded build,
both EVP_MD_fetch() and both publish via _Py_atomic_exchange_ptr. Every store
past the first leaks the overwritten reference (other_digest, never freed).

OpenSSL's method store returns the same refcounted EVP_MD* for concurrent
fetches, so other_digest == digest — the debug assert never fires and the leak
is a refcount inflation on a still-reachable object. That makes it invisible to
TSan, ASan, and LSan, which is why the finding shipped without a reproducer.

This patch adds an atomic counter of race-losers and exposes it to Python, so the
leak is directly observable and countable.

Files

• hashlib_evp_race_counter.patch — gated (#ifdef HASHLIB_EVP_RACE_COUNTER)
  instrumentation of Modules/_hashopenssl.c. Adds a counter, increments it at
  the race-loser site, and exposes _hashlib._evp_md_race_events() /
  _hashlib._evp_md_race_events_reset(). No effect unless the macro is defined.
• repro_ft_hashopenssl_race.py — self-contained stress reproducer. Uses a spin
  barrier under free-threading (tight release into the fetch window) and falls
  back to threading.Barrier under the GIL. Reads the counter if present.

Build (free-threaded, e.g. the matrix debug-ft-nojit-tsan tree)

cd <cpython-ft-tree>
git apply /path/to/hashlib_evp_race_counter.patch
rm -f Modules/_hashopenssl.o
make -j"$(nproc)" CFLAGS_NODIST="-DHASHLIB_EVP_RACE_COUNTER"

Run

# Buggy build (pre-gh-153019), free-threaded -> race-losers > 0, exit 1:
./python repro_ft_hashopenssl_race.py
#   race-losers: 1815  (BUG: 1815 LEAKED EVP_MD refs)

# Same binary, GIL forced on -> race cannot occur, 0, exit 0:
./python -X gil=1 repro_ft_hashopenssl_race.py
#   race-losers: 0  (none — no leak)

Run under TSan with TSAN_OPTIONS="halt_on_error=0 exitcode=0" to also confirm
no data races (the accesses are atomic; there are none in either code shape).

Verifying the fix (gh-153019)

The patch targets the pre-fix code, where the leaked reference is the
other_digest returned by _Py_atomic_exchange_ptr. After applying the fix the
function no longer has other_digest; the equivalent "race-loser" site is the
compare-exchange failure branch. To keep counting on a fixed tree, move the
increment there:

        else {
            _Py_atomic_add_int(&_evp_md_race_events, 1);  // race loser (now freed)
            PY_EVP_MD_free(fresh);   // fixed: loser frees its fetch, no leak
            digest = expected;
        }

On the fixed build the counter reports the same number of race-losers under
the same contention (~1800), but each one now frees its fetch instead of leaking
— demonstrating the fix handles the identical race with zero leaked references.
./python -m test test_hashlib passes and TSan reports no races.

[picnixz]

On the fixed build the counter reports the same number of race-losers under
the same contention (~1800), but each one now frees its fetch instead of leaking
— demonstrating the fix handles the identical race with zero leaked references.
./python -m test test_hashlib passes and TSan reports no races.

IIUC my fix is also correct so.

[picnixz]

@devdanzin You say that it's not visible under A/TSan, but would it be visible under valgrind? I have other plans for today so I'm not sure I can check that myself but in general, if it's not visible under A/TSan, it should be visible under valgrind I guess?