Skip to content

Commit 31059b3

Browse files
committed
gh-158200: Harden faulthandler code metadata reads
1 parent a000129 commit 31059b3

4 files changed

Lines changed: 135 additions & 24 deletions

File tree

‎Lib/test/test_faulthandler.py‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -707,6 +707,67 @@ def test_dump_traceback_later(self):
707707
def test_dump_traceback_later_repeat(self):
708708
self.check_dump_traceback_later(repeat=True)
709709

710+
@threading_helper.requires_working_threading()
711+
def test_dump_traceback_later_concurrent_code_deallocation(self):
712+
code = dedent("""
713+
import faulthandler
714+
import os
715+
import threading
716+
import time
717+
import types
718+
719+
stop = threading.Event()
720+
started = threading.Event()
721+
722+
def template():
723+
return 1
724+
725+
def churn_frames():
726+
started.set()
727+
while not stop.is_set():
728+
code = template.__code__.replace()
729+
function = types.FunctionType(code, {})
730+
function()
731+
732+
worker = threading.Thread(target=churn_frames)
733+
worker.start()
734+
started.wait()
735+
736+
with open(os.devnull, "w") as output:
737+
faulthandler.dump_traceback_later(
738+
1e-6, repeat=True, file=output)
739+
try:
740+
time.sleep(0.1)
741+
finally:
742+
faulthandler.cancel_dump_traceback_later()
743+
stop.set()
744+
worker.join()
745+
""")
746+
script_helper.assert_python_ok(
747+
'-c', code, PYTHONMALLOC='debug')
748+
749+
@threading_helper.requires_working_threading()
750+
@support.requires_gil_enabled()
751+
def test_dump_traceback_later_does_not_wait_for_gil(self):
752+
code = dedent("""
753+
import faulthandler
754+
import sys
755+
756+
sys.setswitchinterval(3600.0)
757+
faulthandler.dump_traceback_later(0.05, exit=True)
758+
while True:
759+
pass
760+
""")
761+
with support.SuppressCrashReport():
762+
process = subprocess.run(
763+
[sys.executable, '-I', '-c', code],
764+
stdout=subprocess.PIPE,
765+
stderr=subprocess.PIPE,
766+
timeout=support.SHORT_TIMEOUT,
767+
)
768+
self.assertEqual(process.returncode, 1)
769+
self.assertIn(b'Timeout', process.stderr)
770+
710771
def test_dump_traceback_later_cancel(self):
711772
self.check_dump_traceback_later(cancel=True)
712773

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
Improve :func:`faulthandler.dump_traceback_later` when another thread
2+
concurrently frees code metadata. The traceback walker now detects common
3+
freed-memory states instead of dereferencing them.

‎Objects/codeobject.c‎

Lines changed: 41 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
#include "pycore_interpframe.h" // FRAME_SPECIALS_SIZE
1111
#include "pycore_opcode_metadata.h" // _PyOpcode_Caches
1212
#include "pycore_opcode_utils.h" // RESUME_AT_FUNC_START
13+
#include "pycore_object.h" // _PyObject_IsFreed()
1314
#include "pycore_optimizer.h" // _Py_ExecutorDetach
1415
#include "pycore_pymem.h" // _PyMem_FreeDelayed()
1516
#include "pycore_pystate.h" // _PyInterpreterState_GET()
@@ -1035,17 +1036,53 @@ PyCode_Addr2Line(PyCodeObject *co, int addrq)
10351036
int
10361037
_PyCode_SafeAddr2Line(PyCodeObject *co, int addrq)
10371038
{
1039+
if (_PyObject_IsFreed((PyObject *)co) || !PyCode_Check(co)) {
1040+
return -1;
1041+
}
10381042
if (addrq < 0) {
10391043
return co->co_firstlineno;
10401044
}
1041-
if (co->_co_monitoring && co->_co_monitoring->lines) {
1042-
return _Py_Instrumentation_GetLine(co, co->_co_monitoring->lines, addrq/sizeof(_Py_CODEUNIT));
1045+
1046+
Py_ssize_t code_size = Py_SIZE(co);
1047+
if (code_size < 0 || addrq / (int)sizeof(_Py_CODEUNIT) >= code_size) {
1048+
return -1;
1049+
}
1050+
1051+
_PyCoMonitoringData *monitoring =
1052+
_Py_atomic_load_ptr_acquire(&co->_co_monitoring);
1053+
if (monitoring != NULL) {
1054+
if (_PyMem_IsPtrFreed(monitoring)) {
1055+
return -1;
1056+
}
1057+
_PyCoLineInstrumentationData *lines =
1058+
_Py_atomic_load_ptr_acquire(&monitoring->lines);
1059+
if (lines != NULL) {
1060+
if (_PyMem_IsPtrFreed(lines)
1061+
|| _PyObject_IsFreed((PyObject *)co)
1062+
|| !PyCode_Check(co))
1063+
{
1064+
return -1;
1065+
}
1066+
return _Py_Instrumentation_GetLine(
1067+
co, lines, addrq / sizeof(_Py_CODEUNIT));
1068+
}
1069+
}
1070+
1071+
if (_PyObject_IsFreed((PyObject *)co) || !PyCode_Check(co)) {
1072+
return -1;
10431073
}
1044-
if (!(addrq >= 0 && addrq < _PyCode_NBYTES(co))) {
1074+
PyObject *linetable = co->co_linetable;
1075+
if (_PyObject_IsFreed(linetable) || !PyBytes_Check(linetable)) {
10451076
return -1;
10461077
}
1078+
int firstlineno = co->co_firstlineno;
1079+
10471080
PyCodeAddressRange bounds;
1048-
_PyCode_InitAddressRange(co, &bounds);
1081+
_PyLineTable_InitAddressRange(
1082+
PyBytes_AS_STRING(linetable),
1083+
PyBytes_GET_SIZE(linetable),
1084+
firstlineno,
1085+
&bounds);
10491086
return _PyCode_CheckLineNumber(addrq, &bounds);
10501087
}
10511088

‎Python/traceback.c‎

Lines changed: 30 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
#include "pycore_frame.h" // PyFrameObject
88
#include "pycore_interp.h" // PyInterpreterState.gc
99
#include "pycore_interpframe.h" // _PyFrame_GetCode()
10+
#include "pycore_object.h" // _PyObject_IsFreed()
1011
#include "pycore_pyerrors.h" // _PyErr_GetRaisedException()
1112
#include "pycore_pystate.h" // _PyThreadState_GET()
1213
#include "pycore_traceback.h" // EXCEPTION_TB_HEADER
@@ -898,15 +899,17 @@ dump_char(int fd, char ch)
898899
void
899900
_Py_DumpASCII(int fd, PyObject *text)
900901
{
901-
PyASCIIObject *ascii = _PyASCIIObject_CAST(text);
902902
Py_ssize_t i, size;
903903
int truncated;
904904
int kind;
905905
void *data = NULL;
906906
Py_UCS4 ch;
907907

908-
if (!PyUnicode_Check(text))
908+
if (_PyObject_IsFreed(text) || !PyUnicode_Check(text)) {
909909
return;
910+
}
911+
912+
PyASCIIObject *ascii = _PyASCIIObject_CAST(text);
910913

911914
size = ascii->length;
912915
kind = ascii->state.kind;
@@ -1041,21 +1044,25 @@ dump_frame(int fd, _PyInterpreterFrame *frame)
10411044
return -1;
10421045
}
10431046

1044-
int res = 0;
10451047
PUTS(fd, " File ");
1046-
if (code->co_filename != NULL
1047-
&& PyUnicode_Check(code->co_filename))
1048-
{
1049-
PUTS(fd, "\"");
1050-
_Py_DumpASCII(fd, code->co_filename);
1051-
PUTS(fd, "\"");
1048+
if (_PyObject_IsFreed((PyObject *)code) || !PyCode_Check(code)) {
1049+
PUTS(fd, "???, line ??? in ???\n");
1050+
return -1;
10521051
}
1053-
else {
1054-
PUTS(fd, "???");
1055-
res = -1;
1052+
PyObject *filename = code->co_filename;
1053+
if (_PyObject_IsFreed(filename) || !PyUnicode_Check(filename)) {
1054+
PUTS(fd, "???, line ??? in ???\n");
1055+
return -1;
10561056
}
1057+
PUTS(fd, "\"");
1058+
_Py_DumpASCII(fd, filename);
1059+
PUTS(fd, "\"");
10571060

10581061
PUTS(fd, ", line ");
1062+
if (_PyObject_IsFreed((PyObject *)code) || !PyCode_Check(code)) {
1063+
PUTS(fd, "??? in ???\n");
1064+
return -1;
1065+
}
10591066
int lasti = _PyFrame_SafeGetLasti(frame);
10601067
int lineno = -1;
10611068
if (lasti >= 0) {
@@ -1065,20 +1072,23 @@ dump_frame(int fd, _PyInterpreterFrame *frame)
10651072
_Py_DumpDecimal(fd, (size_t)lineno);
10661073
}
10671074
else {
1068-
PUTS(fd, "???");
1069-
res = -1;
1075+
PUTS(fd, "??? in ???\n");
1076+
return -1;
10701077
}
10711078

10721079
PUTS(fd, " in ");
1073-
if (code->co_name != NULL && PyUnicode_Check(code->co_name)) {
1074-
_Py_DumpASCII(fd, code->co_name);
1080+
if (_PyObject_IsFreed((PyObject *)code) || !PyCode_Check(code)) {
1081+
PUTS(fd, "???\n");
1082+
return -1;
10751083
}
1076-
else {
1077-
PUTS(fd, "???");
1078-
res = -1;
1084+
PyObject *name = code->co_name;
1085+
if (_PyObject_IsFreed(name) || !PyUnicode_Check(name)) {
1086+
PUTS(fd, "???\n");
1087+
return -1;
10791088
}
1089+
_Py_DumpASCII(fd, name);
10801090
PUTS(fd, "\n");
1081-
return res;
1091+
return 0;
10821092
}
10831093

10841094
static int _Py_NO_SANITIZE_THREAD

0 commit comments

Comments
 (0)