Segfault in _pickle.c:batch_dict_exact when pickling dict with concurrent mutation (free-threading)

[overlorde]

Bug report

Bug description

I've been testing the free-threaded build for thread safety issues in unaudited modules (ref #116738 — _pickle.c is unchecked). Found a segfault when pickle.dumps() runs concurrently with dict mutations on the same dict.

The crash happens in batch_dict_exact at Modules/_pickle.c:3496 — it iterates dict items via _PyDict_Next which gives borrowed references, but there's no critical section on the dict. If another thread pops or replaces a key mid-iteration, the borrowed reference goes stale and Py_INCREF hits a freed/NULL pointer.

Sometimes it raises RuntimeError: dictionary changed size during iteration cleanly, but other times the reference gets invalidated before the size check and it just segfaults.

Reproducer

import pickle
import threading

shared = {str(i): list(range(50)) for i in range(100)}

for trial in range(2000):
    barrier = threading.Barrier(3)

    def dumper():
        try:
            barrier.wait()
            pickle.dumps(shared)
        except Exception:
            pass

    def mutator():
        try:
            barrier.wait()
            for j in range(200):
                key = str(j % 100)
                shared[key] = list(range(j % 50))
                if j % 10 == 0:
                    shared.pop(str(j % 100), None)
                    shared[str(j % 100)] = [j]
        except Exception:
            pass

    threads = [
        threading.Thread(target=dumper),
        threading.Thread(target=dumper),
        threading.Thread(target=mutator),
    ]
    for t in threads: t.start()
    for t in threads: t.join()

Crashes within a few hundred iterations usually. I ran it 10 times, got 9 segfaults.

Tested on

3.14.0a4 free-threading (stock install, no sanitizers):

$ python3.14t reproducer.py
Segmentation fault (core dumped)

3.15.0a7+ free-threading (built with --with-address-sanitizer):

AddressSanitizer:DEADLYSIGNAL
==1334802==ERROR: AddressSanitizer: SEGV on unknown address 0x00000000000c
    #0 _Py_atomic_load_uint32_relaxed Include/cpython/pyatomic_gcc.h:367
    #1 Py_INCREF Include/refcount.h:267
    #2 batch_dict_exact Modules/_pickle.c:3496
    #3 save_dict Modules/_pickle.c:3563
    #4 save Modules/_pickle.c:4568
    #5 dump Modules/_pickle.c:4768
    #6 _pickle_dumps_impl Modules/_pickle.c:7992

Root cause

• batch_dict_exact calls PyDict_Next() which returns borrowed references to key and value
• Between PyDict_Next() returning and Py_INCREF(key) / Py_INCREF(value), another thread can mutate the dict
• The borrowed reference points to a freed object, Py_INCREF dereferences it and segfaults
• The size check at line 3512 (PyDict_GET_SIZE(obj) != dict_size) only catches mutations that change dict size, and only after the batch — too late to prevent the stale reference
• Same issue exists in both the single-item path (line 3474) and the batch loop (line 3494)

Possible fix

• Wrap PyDict_Next() + Py_INCREF(key) + Py_INCREF(value) in Py_BEGIN_CRITICAL_SECTION(obj) / Py_END_CRITICAL_SECTION() to prevent dict mutation while converting borrowed refs to owned refs
• Keep save() calls outside the critical section since they can reenter Python
• Same approach as the set iteration path at line 3644 which already uses Py_BEGIN_CRITICAL_SECTION(obj) with _PySet_NextEntryRef
• Both PyDict_Next call sites in batch_dict_exact need this (line 3474 and line 3494)

CPython versions tested on

3.14, CPython main branch

Operating systems tested on

Linux

Linked PRs

• gh-146452: Fix pickle segfault when pickling dict with concurrent mutation #146470

[overlorde]

Would like to get assigned to that issue.

[overlorde]

The PR patches the two PyDict_Next call sites in batch_dict_exact, but the same borrowed reference problem exists wherever PyDict_Next is used without a critical section — there are 87 call sites across the codebase.

For sets, this was solved by adding _PySet_NextEntryRef which returns owned references and asserts the caller holds the critical section (used in _pickle.c at line 3660). There's no dict equivalent yet.

Would it make sense to add a _PyDict_NextRef following the same pattern as _PySet_NextEntryRef? That way each call site can be migrated with a simple function swap instead of wrapping every PyDict_Next + Py_INCREF in a critical section independently. Happy to work on that if it sounds reasonable.

[mjbommar]

Possibly useful finding from adjacent issue:

A similar crash class fires on free-threaded builds when two threads share a single Pickler instance, with no external mutator.

Lock-test on 3.14.3t over 10 subprocesses each:

• shared Pickler with no Python lock = 8-10/10 SIGSEGV;
• shared Pickler with threading.Lock around dump() = 0/10;
• fresh Pickler per dump() = 0/10.

Crashes land in save_dict per faulthandler, same neighborhood as your fix. Repro is two threads calling p.dump(obj_a) / p.dump(obj_b) on a shared pickle.Pickler(io.BytesIO()).

Root cause looks like BEGIN_USING_PICKLER at lines 710-720 being a non-atomic check-then-set on running; under PEP 703 two threads can both pass the guard (race window is in C, below Python instrumentation resolution, but the lock-test contrast nails it). Once both threads slip past, they hit the same PyDict_Next + Py_INCREF borrowed-ref window on self->memo that #146470 wraps for the object dict. Atomic compare-exchange on the running flag (and the analogous BEGIN_USING_UNPICKLER) would restore the documented RuntimeError: "Pickler object is already used" and prevent the second-thread entry.

Happy to help but seems like you have the ball in this PR.