Skip to content

Commit 25f9328

Browse files
committed
gh-144446: Fix some frame object thread-safety issues
Fix thread-safety issues when accessing frame attributes while another thread is executing the frame: - Add critical section to frame_repr() to prevent races when accessing the frame's code object and line number - Add _Py_NO_SANITIZE_THREAD to PyUnstable_InterpreterFrame_GetLasti() to allow intentional racy reads of instr_ptr. - Fix take_ownership() to not write to the original frame's f_executable
1 parent 914fbec commit 25f9328

File tree

4 files changed

+166
-7
lines changed

4 files changed

+166
-7
lines changed
Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,153 @@
1+
import functools
2+
import sys
3+
import threading
4+
import unittest
5+
6+
from test.support import threading_helper
7+
8+
threading_helper.requires_working_threading(module=True)
9+
10+
11+
def run_with_frame(funcs, runner=None, iters=10):
12+
"""Run funcs with a frame from another thread that is currently executing.
13+
14+
Args:
15+
funcs: A function or list of functions that take a frame argument
16+
runner: Optional function to run in the executor thread. If provided,
17+
it will be called and should return eventually. The frame
18+
passed to funcs will be the runner's frame.
19+
iters: Number of iterations each func should run
20+
"""
21+
if not isinstance(funcs, list):
22+
funcs = [funcs]
23+
24+
frame_var = None
25+
e = threading.Event()
26+
b = threading.Barrier(len(funcs) + 1)
27+
28+
if runner is None:
29+
def runner():
30+
j = 0
31+
for i in range(100):
32+
j += i
33+
34+
def executor():
35+
nonlocal frame_var
36+
frame_var = sys._getframe()
37+
e.set()
38+
b.wait()
39+
runner()
40+
41+
def func_wrapper(func):
42+
e.wait()
43+
frame = frame_var
44+
b.wait()
45+
for _ in range(iters):
46+
func(frame)
47+
48+
test_funcs = [functools.partial(func_wrapper, f) for f in funcs]
49+
threading_helper.run_concurrently([executor] + test_funcs)
50+
51+
52+
class TestFrameRaces(unittest.TestCase):
53+
def test_concurrent_f_lasti(self):
54+
run_with_frame(lambda frame: frame.f_lasti)
55+
56+
def test_concurrent_f_lineno(self):
57+
run_with_frame(lambda frame: frame.f_lineno)
58+
59+
def test_concurrent_f_code(self):
60+
run_with_frame(lambda frame: frame.f_code)
61+
62+
def test_concurrent_f_back(self):
63+
run_with_frame(lambda frame: frame.f_back)
64+
65+
def test_concurrent_f_globals(self):
66+
run_with_frame(lambda frame: frame.f_globals)
67+
68+
def test_concurrent_f_builtins(self):
69+
run_with_frame(lambda frame: frame.f_builtins)
70+
71+
def test_concurrent_f_locals(self):
72+
run_with_frame(lambda frame: frame.f_locals)
73+
74+
def test_concurrent_f_trace_read(self):
75+
run_with_frame(lambda frame: frame.f_trace)
76+
77+
def test_concurrent_f_trace_opcodes_read(self):
78+
run_with_frame(lambda frame: frame.f_trace_opcodes)
79+
80+
def test_concurrent_repr(self):
81+
run_with_frame(lambda frame: repr(frame))
82+
83+
def test_concurrent_f_trace_write(self):
84+
"""Test writing to f_trace of a live frame."""
85+
def trace_func(frame, event, arg):
86+
return trace_func
87+
88+
def writer(frame):
89+
frame.f_trace = trace_func
90+
frame.f_trace = None
91+
92+
run_with_frame(writer)
93+
94+
def test_concurrent_f_trace_read_write(self):
95+
"""Test concurrent reads and writes of f_trace on a live frame."""
96+
def trace_func(frame, event, arg):
97+
return trace_func
98+
99+
def reader(frame):
100+
_ = frame.f_trace
101+
102+
def writer(frame):
103+
frame.f_trace = trace_func
104+
frame.f_trace = None
105+
106+
run_with_frame([reader, writer, reader, writer])
107+
108+
def test_concurrent_f_trace_opcodes_write(self):
109+
"""Test writing to f_trace_opcodes of a live frame."""
110+
def writer(frame):
111+
frame.f_trace_opcodes = True
112+
frame.f_trace_opcodes = False
113+
114+
run_with_frame(writer)
115+
116+
def test_concurrent_f_trace_opcodes_read_write(self):
117+
"""Test concurrent reads and writes of f_trace_opcodes on a live frame."""
118+
def reader(frame):
119+
_ = frame.f_trace_opcodes
120+
121+
def writer(frame):
122+
frame.f_trace_opcodes = True
123+
frame.f_trace_opcodes = False
124+
125+
run_with_frame([reader, writer, reader, writer])
126+
127+
def test_concurrent_frame_clear(self):
128+
"""Test race between frame.clear() and attribute reads."""
129+
def create_frame():
130+
x = 1
131+
y = 2
132+
return sys._getframe()
133+
134+
frame = create_frame()
135+
136+
def reader():
137+
for _ in range(10):
138+
try:
139+
_ = frame.f_locals
140+
_ = frame.f_code
141+
_ = frame.f_lineno
142+
except ValueError:
143+
# Frame may be cleared
144+
pass
145+
146+
def clearer():
147+
frame.clear()
148+
149+
threading_helper.run_concurrently([reader, reader, clearer])
150+
151+
152+
if __name__ == "__main__":
153+
unittest.main()
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
Fix data races in the free-threaded build when reading :class:`frame` object
2+
attributes while another thread is executing the frame.

Objects/frameobject.c

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1049,11 +1049,11 @@ static PyObject *
10491049
frame_lasti_get_impl(PyFrameObject *self)
10501050
/*[clinic end generated code: output=03275b4f0327d1a2 input=0225ed49cb1fbeeb]*/
10511051
{
1052-
int lasti = _PyInterpreterFrame_LASTI(self->f_frame);
1052+
int lasti = PyUnstable_InterpreterFrame_GetLasti(self->f_frame);
10531053
if (lasti < 0) {
10541054
return PyLong_FromLong(-1);
10551055
}
1056-
return PyLong_FromLong(lasti * sizeof(_Py_CODEUNIT));
1056+
return PyLong_FromLong(lasti);
10571057
}
10581058

10591059
/*[clinic input]
@@ -2053,11 +2053,15 @@ static PyObject *
20532053
frame_repr(PyObject *op)
20542054
{
20552055
PyFrameObject *f = PyFrameObject_CAST(op);
2056+
PyObject *result;
2057+
Py_BEGIN_CRITICAL_SECTION(f);
20562058
int lineno = PyFrame_GetLineNumber(f);
20572059
PyCodeObject *code = _PyFrame_GetCode(f->f_frame);
2058-
return PyUnicode_FromFormat(
2060+
result = PyUnicode_FromFormat(
20592061
"<frame at %p, file %R, line %d, code %S>",
20602062
f, code->co_filename, lineno, code->co_name);
2063+
Py_END_CRITICAL_SECTION();
2064+
return result;
20612065
}
20622066

20632067
static PyMethodDef frame_methods[] = {

Python/frame.c

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ take_ownership(PyFrameObject *f, _PyInterpreterFrame *frame)
5454
_PyFrame_Copy(frame, new_frame);
5555
// _PyFrame_Copy takes the reference to the executable,
5656
// so we need to restore it.
57-
frame->f_executable = PyStackRef_DUP(new_frame->f_executable);
57+
new_frame->f_executable = PyStackRef_DUP(new_frame->f_executable);
5858
f->f_frame = new_frame;
5959
new_frame->owner = FRAME_OWNED_BY_FRAME_OBJECT;
6060
if (_PyFrame_IsIncomplete(new_frame)) {
@@ -135,14 +135,14 @@ PyUnstable_InterpreterFrame_GetCode(struct _PyInterpreterFrame *frame)
135135
return PyStackRef_AsPyObjectNew(frame->f_executable);
136136
}
137137

138-
int
138+
// NOTE: We allow racy accesses to the instruction pointer from other threads
139+
// for sys._current_frames() and similar APIs.
140+
int _Py_NO_SANITIZE_THREAD
139141
PyUnstable_InterpreterFrame_GetLasti(struct _PyInterpreterFrame *frame)
140142
{
141143
return _PyInterpreterFrame_LASTI(frame) * sizeof(_Py_CODEUNIT);
142144
}
143145

144-
// NOTE: We allow racy accesses to the instruction pointer from other threads
145-
// for sys._current_frames() and similar APIs.
146146
int _Py_NO_SANITIZE_THREAD
147147
PyUnstable_InterpreterFrame_GetLine(_PyInterpreterFrame *frame)
148148
{

0 commit comments

Comments
 (0)