gh-151818: Fix double-free in _CALL_LIST_APPEND on allocation failure#151826
Open
timurmamedov1 wants to merge 2 commits into
Open
gh-151818: Fix double-free in _CALL_LIST_APPEND on allocation failure#151826timurmamedov1 wants to merge 2 commits into
timurmamedov1 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a reference-management bug in the specialized CALL_LIST_APPEND fast path that could double-close/decref an argument on list-resize allocation failure, leading to a double-free/crash. The change adjusts ownership so the list append consumes an independent reference while the eval-stack PyStackRef remains valid for unwinding.
Changes:
- Update
CALL_LIST_APPENDimplementations to passPy_NewRef(PyStackRef_AsPyObjectBorrow(arg))to_PyList_AppendTakeRef()and explicitlyPyStackRef_CLOSE(arg)only on success. - Adjust stack-pointer synchronization in generated interpreter/test-case code to correctly pop/close
argon the success path. - Simplify Tier 2 uop variants for
_CALL_LIST_APPEND(retain only_CALL_LIST_APPEND_r33) and update uop metadata/IDs accordingly; addHAS_ESCAPES_FLAGfor_CALL_LIST_APPEND.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Python/generated_cases.c.h | Fix ref ownership for _CALL_LIST_APPEND and adjust stack-pointer updates/cleanup to avoid double-close on error. |
| Python/executor_cases.c.h | Fix Tier 2 _CALL_LIST_APPEND_r33 to avoid stealing arg and restore stack correctly on error; remove other variants. |
| Python/bytecodes.c | Update _CALL_LIST_APPEND uop definition to use an independent ref for list append and close arg only on success. |
| Modules/_testinternalcapi/test_cases.c.h | Mirrors interpreter generated-case fix for internal testing harness. |
| Include/internal/pycore_uop_metadata.h | Mark _CALL_LIST_APPEND as escaping and restrict caching configuration to the remaining _r33 variant. |
| Include/internal/pycore_uop_ids.h | Remove _CALL_LIST_APPEND_r03/r13/r23 IDs and renumber subsequent uops. |
| Misc/NEWS.d/next/Core_and_Builtins/2026-06-20-18-00-00.gh-issue-151818.LsApDf.rst | Adds NEWS entry documenting the crash fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+5108
to
+5114
| PyObject *arg_o = PyStackRef_AsPyObjectBorrow(arg); | ||
| int err = _PyList_AppendTakeRef((PyListObject *)self_o, Py_NewRef(arg_o)); | ||
| UNLOCK_OBJECT(self_o); | ||
| if (err) { | ||
| ERROR_NO_POP(); | ||
| } | ||
| PyStackRef_CLOSE(arg); |
timurmamedov1
force-pushed
the
gh-151818-fix-list-append-double-free
branch
from
b054bf2 to
f3a6786
Compare
…ailure _CALL_LIST_APPEND stole the arg stackref via PyStackRef_AsPyObjectSteal and passed it to _PyList_AppendTakeRef. When the list's backing array failed to grow, _PyList_AppendTakeRef decreffed the item, but the stale stackref remained on the value stack. The exception unwinder then closed it a second time, causing a double-free / use-after-free. Fix by giving _PyList_AppendTakeRef a separate reference via Py_NewRef and closing the stackref explicitly on success. On the error path the stackref still holds a valid reference, so the exception unwinder can safely close it.
CALL_LIST_APPEND is not in the Sphinx opcode domain, so use double backticks instead of :opcode: to avoid a broken cross-reference.
timurmamedov1
force-pushed
the
gh-151818-fix-list-append-double-free
branch
from
c9176ae to
ca1f525
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
_CALL_LIST_APPENDstole theargstackref viaPyStackRef_AsPyObjectStealand passed it to_PyList_AppendTakeRef. When the list's backing array failed to grow,_PyList_AppendTakeRefdecref'd the item, but the stale stackref remained on the value stack.exception_unwindthen closed it a second time. Double-free on debug builds (_Py_NegativeRefcount), segfault on release builds.Fix: Give
_PyList_AppendTakeRefa separate reference viaPy_NewRefand close the stackref explicitly on success. On the error path the stackref still holds a valid reference, so the exception unwinder can safely close it.This trades one extra
Py_INCREFon the hot path for correct error handling. The sibling ops (LIST_APPEND,SET_ADD,MAP_ADD) avoid this by usingERROR_IFwhich drops dead inputs, but_CALL_LIST_APPENDhas additional live inputs (callable,self) that prevent usingERROR_IFdirectly.Related to gh-151119 / gh-151538 (different bug in the same op - missing eval-stack sync), but this is a distinct bug as noted by @devdanzin.
list.append()when the list grows underMemoryError(_CALL_LIST_APPEND) #151818