Skip to content

Commit 5bb3bbb

Browse files
authored
gh-144446: Fix some frame object thread-safety issues (gh-144479)
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 45d4a34 commit 5bb3bbb

File tree

4 files changed

+164
-7
lines changed

4 files changed

+164
-7
lines changed
Lines changed: 151 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,151 @@
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+
def trace_func(frame, event, arg):
85+
return trace_func
86+
87+
def writer(frame):
88+
frame.f_trace = trace_func
89+
frame.f_trace = None
90+
91+
run_with_frame(writer)
92+
93+
def test_concurrent_f_trace_read_write(self):
94+
# Test concurrent reads and writes of f_trace on a live frame.
95+
def trace_func(frame, event, arg):
96+
return trace_func
97+
98+
def reader(frame):
99+
_ = frame.f_trace
100+
101+
def writer(frame):
102+
frame.f_trace = trace_func
103+
frame.f_trace = None
104+
105+
run_with_frame([reader, writer, reader, writer])
106+
107+
def test_concurrent_f_trace_opcodes_write(self):
108+
def writer(frame):
109+
frame.f_trace_opcodes = True
110+
frame.f_trace_opcodes = False
111+
112+
run_with_frame(writer)
113+
114+
def test_concurrent_f_trace_opcodes_read_write(self):
115+
# Test concurrent reads and writes of f_trace_opcodes on a live frame.
116+
def reader(frame):
117+
_ = frame.f_trace_opcodes
118+
119+
def writer(frame):
120+
frame.f_trace_opcodes = True
121+
frame.f_trace_opcodes = False
122+
123+
run_with_frame([reader, writer, reader, writer])
124+
125+
def test_concurrent_frame_clear(self):
126+
# Test race between frame.clear() and attribute reads.
127+
def create_frame():
128+
x = 1
129+
y = 2
130+
return sys._getframe()
131+
132+
frame = create_frame()
133+
134+
def reader():
135+
for _ in range(10):
136+
try:
137+
_ = frame.f_locals
138+
_ = frame.f_code
139+
_ = frame.f_lineno
140+
except ValueError:
141+
# Frame may be cleared
142+
pass
143+
144+
def clearer():
145+
frame.clear()
146+
147+
threading_helper.run_concurrently([reader, reader, clearer])
148+
149+
150+
if __name__ == "__main__":
151+
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 frame object attributes
2+
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)