From d7bb7c781771650a4edcdee9dfd1ab9c4083e9fd Mon Sep 17 00:00:00 2001 From: mpage Date: Wed, 5 Mar 2025 10:42:09 -0800 Subject: [PATCH] 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 b6d85490eef1..4aae1ffc350d 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 0013540c4969..fe4857e82747 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 ad7accf2099f..2a34fd04f350 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 84faa5a32a1f..2893acf6d6e1 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 e164f11620de..29160b9f6634 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 8c3c0e3910b8..5216918560a4 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 162a0fdb2cc4..cecfb6f3834d 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", -- 2.47.3