Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-118331: Fix a couple of issues when list allocation fails #130811

Merged
merged 3 commits into from
Mar 5, 2025

Conversation

mpage
Copy link
Contributor

@mpage mpage commented Mar 3, 2025

This fixes a couple of bugs that are triggered when list allocation fails. Specifically, when the list object is allocated
successfully, but allocation of the items array fails.

Use-after-free on the items array

We didn't set the items array pointer in the list object to NULL after freeing the items array. As a result, we could end up with a list object added to the free list that contained a pointer to an already-freed items array. A subsequent list allocation that successfully retrieved a list object from the free list but failed to allocate a new items would deallocate the
list object:

_PyListArray *array = list_allocate_array(size);
if (array == NULL) {
Py_DECREF(op);
return PyErr_NoMemory();
}

list_dealloc would then try to use the previously freed items array:

if (op->ob_item != NULL) {
/* Do it backwards, for Christian Tismer.
There's a simple test case where somehow this reduces
thrashing when a *very* large list is created and
immediately deleted. */
i = Py_SIZE(op);
while (--i >= 0) {
Py_XDECREF(op->ob_item[i]);
}
free_list_items(op->ob_item, false);
}

Incorrect stackpointer assertion

We check that either there is no Python code executing (frame is NULL) or the stack pointer for the current frame is set when executing _Py_Dealloc:

cpython/Objects/object.c

Lines 2987 to 2991 in bbf1979

#ifndef Py_GIL_DISABLED
/* This assertion doesn't hold for the free-threading build, as
* PyStackRef_CLOSE_SPECIALIZED is not implemented */
assert(tstate->current_frame == NULL || tstate->current_frame->stackpointer != NULL);
#endif

I think the intent here is to catch places in the interpreter loop that escape due to decrefs where we aren't setting / clearing the stack pointer correctly (e.g. due to shortcomings in our analysis or escaping calls that are incorrectly marked as non-escaping). The assertion is overly conservative. It will catch all potentially escaping decrefs, but it will also catch decrefs that can never escape. In this case, _PyList_FromStackRefStealOnSuccess is, correctly, I think, marked as non-escaping. The only decref it can perform is on an exact list (not a subtype). However, it triggers this assertion.

There are a few options I can see for fixing this:

  1. Get rid of the assertion.
  2. Detect and don't assert when we encounter objects whose destructors cannot execute arbitrary code.
  3. Mark _PyList_FromStackRefStealOnSuccess as escaping.

I went with (3) because it's correct, if pessimistic, and I don't think it'll impact performance too much. I think the assertion is worth keeping around and I'm not sure of a good way to do (2) generically. Happy to do something else if reviewers feel strongly otherwise.

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.
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.
@mpage mpage changed the title gh-118331: Fix use after free in list objects gh-118331: Fix a couple of issues when list allocation fails Mar 3, 2025
@mpage mpage marked this pull request as ready for review March 4, 2025 00:03
@mpage mpage requested a review from markshannon as a code owner March 4, 2025 00:03
@mpage mpage requested review from colesbury and corona10 March 4, 2025 00:03
Copy link
Contributor

@colesbury colesbury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

I think going with (3), treating _PyList_FromStackRefStealOnSuccess as possibly escaping, makes sense for now. I don't expect it to impact performance given that we started treating PyStackRef_CLOSE as escaping without problems, and STORE_FAST is executed ~35x more frequently than BUILD_LIST.

@mpage mpage merged commit d7bb7c7 into python:main Mar 5, 2025
54 checks passed
@encukou
Copy link
Member

encukou commented Mar 6, 2025

This commit introduced a failure in --with-tracerefs builds:

======================================================================
FAIL: test_no_memory (test.test_list.ListTest.test_no_memory)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/encukou/dev/cpython/Lib/test/test_list.py", line 328, in test_no_memory
    self.assertIn("MemoryError", err.decode("utf-8"))
    ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: 'MemoryError' not found in 'Fatal Python error: _PyRefchain_Trace: _Py_hashtable_set() memory allocation failed\nPython runtime state: initialized\n\nCurrent thread 0x00007ff7cc547740 (most recent call first):\n  File "<string>", line 7 in <module>\n\nExtension modules: _testcapi (total: 1)\n'

----------------------------------------------------------------------

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants