Skip to content

Commit ab4d60e

Browse files
committed
gh-146452: Fix pickle segfault when pickling dict with concurrent mutation
1 parent 0e54305 commit ab4d60e

3 files changed

Lines changed: 74 additions & 6 deletions

File tree

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
import pickle
2+
import threading
3+
import unittest
4+
5+
from test.support import threading_helper
6+
7+
8+
@threading_helper.requires_working_threading()
9+
class TestPickleFreeThreading(unittest.TestCase):
10+
11+
def test_pickle_dumps_with_concurrent_dict_mutation(self):
12+
# gh-146452: Pickling a dict while another thread mutates it
13+
# used to segfault. batch_dict_exact() iterated dict items via
14+
# PyDict_Next() which returns borrowed references, and a
15+
# concurrent pop/replace could free the value before Py_INCREF
16+
# got to it.
17+
shared = {str(i): list(range(20)) for i in range(50)}
18+
19+
def dumper():
20+
for _ in range(1000):
21+
try:
22+
pickle.dumps(shared)
23+
except RuntimeError:
24+
# "dictionary changed size during iteration" is expected
25+
pass
26+
27+
def mutator():
28+
for j in range(1000):
29+
key = str(j % 50)
30+
shared[key] = list(range(j % 20))
31+
if j % 10 == 0:
32+
shared.pop(key, None)
33+
shared[key] = [j]
34+
35+
threads = []
36+
for _ in range(10):
37+
threads.append(threading.Thread(target=dumper))
38+
threads.append(threading.Thread(target=mutator))
39+
40+
for t in threads:
41+
t.start()
42+
for t in threads:
43+
t.join()
44+
45+
# If we get here without a segfault, the test passed.
46+
47+
48+
if __name__ == "__main__":
49+
unittest.main()
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
Fix segfault in :mod:`pickle` when pickling a dictionary concurrently
2+
mutated by another thread in the free-threaded build. The dict iteration in
3+
``batch_dict_exact`` now holds a critical section to prevent borrowed
4+
references from being invalidated mid-iteration.

Modules/_pickle.c

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3454,7 +3454,7 @@ static int
34543454
batch_dict_exact(PickleState *state, PicklerObject *self, PyObject *obj)
34553455
{
34563456
PyObject *key = NULL, *value = NULL;
3457-
int i;
3457+
int i, has_next;
34583458
Py_ssize_t dict_size, ppos = 0;
34593459

34603460
const char mark_op = MARK;
@@ -3471,9 +3471,16 @@ batch_dict_exact(PickleState *state, PicklerObject *self, PyObject *obj)
34713471
Py_ssize_t total = 0;
34723472
do {
34733473
if (dict_size - total == 1) {
3474+
/* gh-146452: Prevent concurrent dict mutation from
3475+
invalidating the borrowed refs from PyDict_Next(). */
3476+
Py_BEGIN_CRITICAL_SECTION(obj);
34743477
PyDict_Next(obj, &ppos, &key, &value);
3475-
Py_INCREF(key);
3476-
Py_INCREF(value);
3478+
Py_XINCREF(key);
3479+
Py_XINCREF(value);
3480+
Py_END_CRITICAL_SECTION();
3481+
if (key == NULL) {
3482+
return 0;
3483+
}
34773484
if (save(state, self, key, 0) < 0) {
34783485
goto error;
34793486
}
@@ -3491,9 +3498,17 @@ batch_dict_exact(PickleState *state, PicklerObject *self, PyObject *obj)
34913498
i = 0;
34923499
if (_Pickler_Write(self, &mark_op, 1) < 0)
34933500
return -1;
3494-
while (PyDict_Next(obj, &ppos, &key, &value)) {
3495-
Py_INCREF(key);
3496-
Py_INCREF(value);
3501+
for (;;) {
3502+
/* gh-146452: same as above. */
3503+
Py_BEGIN_CRITICAL_SECTION(obj);
3504+
has_next = PyDict_Next(obj, &ppos, &key, &value);
3505+
if (has_next) {
3506+
Py_INCREF(key);
3507+
Py_INCREF(value);
3508+
}
3509+
Py_END_CRITICAL_SECTION();
3510+
if (!has_next)
3511+
break;
34973512
if (save(state, self, key, 0) < 0) {
34983513
goto error;
34993514
}

0 commit comments

Comments
 (0)