From 2394419db2eeef00c51b0e9d1ce3437cc5640fd5 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Mon, 3 Mar 2025 13:49:54 -0800 Subject: [PATCH 1/2] Fix use after free in list objects Set the items pointer in the list object to NULL after the items array is freed during list deallocation. Otherwise, we can end up with a list object added to the free list that contains a pointer to an already-freed items array. --- Lib/test/test_list.py | 20 +++++++++++++++++++- Objects/listobject.c | 1 + 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_list.py b/Lib/test/test_list.py index ad7accf2099f43..2a34fd04f35059 100644 --- a/Lib/test/test_list.py +++ b/Lib/test/test_list.py @@ -1,6 +1,9 @@ import sys -from test import list_tests +import textwrap +from test import list_tests, support from test.support import cpython_only +from test.support.import_helper import import_module +from test.support.script_helper import assert_python_failure import pickle import unittest @@ -309,5 +312,20 @@ def test_tier2_invalidates_iterator(self): a.append(4) self.assertEqual(list(it), []) + @support.cpython_only + def test_no_memory(self): + # gh-118331: Make sure we don't crash if list allocation fails + import_module("_testcapi") + code = textwrap.dedent(""" + import _testcapi, sys + # Prime the freelist + l = [None] + del l + _testcapi.set_nomemory(0) + l = [None] + """) + _, _, err = assert_python_failure("-c", code) + self.assertIn("MemoryError", err.decode("utf-8")) + if __name__ == "__main__": unittest.main() diff --git a/Objects/listobject.c b/Objects/listobject.c index 84faa5a32a1f2a..2893acf6d6e143 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -533,6 +533,7 @@ list_dealloc(PyObject *self) Py_XDECREF(op->ob_item[i]); } free_list_items(op->ob_item, false); + op->ob_item = NULL; } if (PyList_CheckExact(op)) { _Py_FREELIST_FREE(lists, op, PyObject_GC_Del); From aa811fe7aecc017af3989a5053b3fc08dc84d1e8 Mon Sep 17 00:00:00 2001 From: Matt Page Date: Mon, 3 Mar 2025 11:21:14 -0800 Subject: [PATCH 2/2] Mark `_PyList_FromStackRefStealOnSuccess` as escaping I think technically it's not escaping, because the only object that can be decrefed if allocation fails is an exact list, which cannot execute arbitrary code when it is destroyed. However, this seems less intrusive than trying to special cases objects in the assert in `_Py_Dealloc` that checks for non-null stackpointers and shouldn't matter for performance. --- Include/internal/pycore_opcode_metadata.h | 2 +- Include/internal/pycore_uop_metadata.h | 2 +- Python/executor_cases.c.h | 2 ++ Python/generated_cases.c.h | 2 ++ Tools/cases_generator/analyzer.py | 1 - 5 files changed, 6 insertions(+), 3 deletions(-) diff --git a/Include/internal/pycore_opcode_metadata.h b/Include/internal/pycore_opcode_metadata.h index b6d85490eef1f3..4aae1ffc350dfe 100644 --- a/Include/internal/pycore_opcode_metadata.h +++ b/Include/internal/pycore_opcode_metadata.h @@ -2028,7 +2028,7 @@ const struct opcode_metadata _PyOpcode_opcode_metadata[266] = { [BINARY_OP_SUBTRACT_FLOAT] = { true, INSTR_FMT_IXC0000, HAS_EXIT_FLAG | HAS_ERROR_FLAG }, [BINARY_OP_SUBTRACT_INT] = { true, INSTR_FMT_IXC0000, HAS_EXIT_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG }, [BINARY_SLICE] = { true, INSTR_FMT_IX, HAS_ERROR_FLAG | HAS_ESCAPES_FLAG }, - [BUILD_LIST] = { true, INSTR_FMT_IB, HAS_ARG_FLAG | HAS_ERROR_FLAG | HAS_ERROR_NO_POP_FLAG }, + [BUILD_LIST] = { true, INSTR_FMT_IB, HAS_ARG_FLAG | HAS_ERROR_FLAG | HAS_ERROR_NO_POP_FLAG | HAS_ESCAPES_FLAG }, [BUILD_MAP] = { true, INSTR_FMT_IB, HAS_ARG_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG }, [BUILD_SET] = { true, INSTR_FMT_IB, HAS_ARG_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG }, [BUILD_SLICE] = { true, INSTR_FMT_IB, HAS_ARG_FLAG | HAS_ERROR_FLAG }, diff --git a/Include/internal/pycore_uop_metadata.h b/Include/internal/pycore_uop_metadata.h index 0013540c496938..fe4857e827478c 100644 --- a/Include/internal/pycore_uop_metadata.h +++ b/Include/internal/pycore_uop_metadata.h @@ -136,7 +136,7 @@ const uint16_t _PyUop_Flags[MAX_UOP_ID+1] = { [_COPY_FREE_VARS] = HAS_ARG_FLAG, [_BUILD_STRING] = HAS_ARG_FLAG | HAS_ERROR_FLAG, [_BUILD_TUPLE] = HAS_ARG_FLAG | HAS_ERROR_FLAG | HAS_ERROR_NO_POP_FLAG, - [_BUILD_LIST] = HAS_ARG_FLAG | HAS_ERROR_FLAG | HAS_ERROR_NO_POP_FLAG, + [_BUILD_LIST] = HAS_ARG_FLAG | HAS_ERROR_FLAG | HAS_ERROR_NO_POP_FLAG | HAS_ESCAPES_FLAG, [_LIST_EXTEND] = HAS_ARG_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG, [_SET_UPDATE] = HAS_ARG_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG, [_BUILD_SET] = HAS_ARG_FLAG | HAS_ERROR_FLAG | HAS_ESCAPES_FLAG, diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h index e164f11620de41..29160b9f6634c5 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -2545,7 +2545,9 @@ _PyStackRef list; oparg = CURRENT_OPARG(); values = &stack_pointer[-oparg]; + _PyFrame_SetStackPointer(frame, stack_pointer); PyObject *list_o = _PyList_FromStackRefStealOnSuccess(values, oparg); + stack_pointer = _PyFrame_GetStackPointer(frame); if (list_o == NULL) { JUMP_TO_ERROR(); } diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index 8c3c0e3910b8d1..5216918560a487 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -1015,7 +1015,9 @@ _PyStackRef *values; _PyStackRef list; values = &stack_pointer[-oparg]; + _PyFrame_SetStackPointer(frame, stack_pointer); PyObject *list_o = _PyList_FromStackRefStealOnSuccess(values, oparg); + stack_pointer = _PyFrame_GetStackPointer(frame); if (list_o == NULL) { JUMP_TO_LABEL(error); } diff --git a/Tools/cases_generator/analyzer.py b/Tools/cases_generator/analyzer.py index 162a0fdb2cc459..cecfb6f3834d44 100644 --- a/Tools/cases_generator/analyzer.py +++ b/Tools/cases_generator/analyzer.py @@ -632,7 +632,6 @@ def has_error_without_pop(op: parser.CodeDef) -> bool: "_PyGen_GetGeneratorFromFrame", "_PyInterpreterState_GET", "_PyList_AppendTakeRef", - "_PyList_FromStackRefStealOnSuccess", "_PyList_ITEMS", "_PyLong_CompactValue", "_PyLong_DigitCount",