From d7bb7c781771650a4edcdee9dfd1ab9c4083e9fd Mon Sep 17 00:00:00 2001 From: mpage Date: Wed, 5 Mar 2025 10:42:09 -0800 Subject: gh-118331: Fix a couple of issues when list allocation fails (#130811) * 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. * 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 +- Lib/test/test_list.py | 20 +++++++++++++++++++- Objects/listobject.c | 1 + Python/executor_cases.c.h | 2 ++ Python/generated_cases.c.h | 2 ++ Tools/cases_generator/analyzer.py | 1 - 7 files changed, 26 insertions(+), 4 deletions(-) diff --git a/Include/internal/pycore_opcode_metadata.h b/Include/internal/pycore_opcode_metadata.h index b6d8549..4aae1ff 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 0013540..fe4857e 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/Lib/test/test_list.py b/Lib/test/test_list.py index ad7accf..2a34fd0 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 @@ class ListTest(list_tests.CommonTest): 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 84faa5a..2893acf 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); diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h index e164f11..29160b9 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 8c3c0e3..5216918 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 162a0fd..cecfb6f 100644 --- a/Tools/cases_generator/analyzer.py +++ b/Tools/cases_generator/analyzer.py @@ -632,7 +632,6 @@ NON_ESCAPING_FUNCTIONS = ( "_PyGen_GetGeneratorFromFrame", "_PyInterpreterState_GET", "_PyList_AppendTakeRef", - "_PyList_FromStackRefStealOnSuccess", "_PyList_ITEMS", "_PyLong_CompactValue", "_PyLong_DigitCount", -- cgit v0.12