Skip to content

gh-140530: fix a reference leak in an error path for raise exc from cause #140908

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.

Sign up for GitHub

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

Merged
picnixz merged 1 commit into from
Nov 9, 2025
Merged

gh-140530: fix a reference leak in an error path for raise exc from cause #140908

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 7 additions & 11 deletions Lib/test/test_raise.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,18 +186,14 @@ def test_class_cause(self):
self.fail("No exception raised")

def test_class_cause_nonexception_result(self):
class ConstructsNone(BaseException):
Copy link
Member

@Fidget-Spinner Fidget-Spinner Nov 2, 2025

Choose a reason for hiding this comment

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

I think we should keep the old test and add the new test in?

Copy link
Member Author

@picnixz picnixz Nov 2, 2025

Choose a reason for hiding this comment

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

The old test is actually testing the same code. The problem was that the returned value was immortal...

Copy link
Member Author

@picnixz picnixz Nov 2, 2025

Choose a reason for hiding this comment

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

FTR:

if (PyExceptionClass_Check(cause)) {
	fixed_cause = _PyObject_CallNoArgs(cause);
	if (fixed_cause == NULL)
		goto raise_error;
	if (!PyExceptionInstance_Check(fixed_cause)) {
		_PyErr_Format(tstate, PyExc_TypeError,
			"calling %R should have returned an instance of "
			"BaseException, not %R",
			cause, Py_TYPE(fixed_cause));
        Py_DECREF(fixed_cause);
		goto raise_error;
	}
	Py_DECREF(cause);
}

Since cause is an exception class, we called __new__ and fixed_cause would have been None. We would then move to PyExceptionInstance_Check but since fixed_cause is immortal the refleak didn't appear.

@classmethod
# See https://github.com/python/cpython/issues/140530.
class ConstructMortal(BaseException):
def __new__(*args, **kwargs):
return None
try:
raise IndexError from ConstructsNone
except TypeError as e:
self.assertIn("should have returned an instance of BaseException", str(e))
except IndexError:
self.fail("Wrong kind of exception raised")
else:
self.fail("No exception raised")
return ["mortal value"]

msg = ".*should have returned an instance of BaseException.*"
with self.assertRaisesRegex(TypeError, msg):
raise IndexError from ConstructMortal

def test_instance_cause(self):
cause = KeyError()
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Core_and_Builtins/2025-11-02-12-47-38.gh-issue-140530.S934bp.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fix a reference leak when ``raise exc from cause`` fails. Patch by Bénédikt
Tran.
1 change: 1 addition & 0 deletions Python/ceval.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2148,6 +2148,7 @@ do_raise(PyThreadState *tstate, PyObject *exc, PyObject *cause)
"calling %R should have returned an instance of "
"BaseException, not %R",
cause, Py_TYPE(fixed_cause));
Py_DECREF(fixed_cause);
goto raise_error;
}
Py_DECREF(cause);
Copy link
Member

@Fidget-Spinner Fidget-Spinner Nov 7, 2025

Choose a reason for hiding this comment

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

Just checking so I'm clear: PyException_SetCause steals a reference right? That's why we're not decrefing fixed_cause here?

Copy link
Member Author

@picnixz picnixz Nov 7, 2025

Choose a reason for hiding this comment

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

AFAIR, yes. PyException_Set* always steal refs IIRC, but I will check this again (it's always a pain to remember which function steals or borrows)

Copy link
Member

@efimov-mikhail efimov-mikhail Nov 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, PyException_SetCause steals a reference to cause:

cpython/Objects/exceptions.c

Lines 548 to 551 in 7ae440f

/* Steals a reference to cause */
void
PyException_SetCause(PyObject *self, PyObject *cause)
{

Expand Down
Loading