bpo-42143: Ensure PyFunction_NewWithQualName() can't fail after creating the func object#22953

Jongy · 2020-10-24T23:58:13Z

func_dealloc() does not handle partially-created objects. Best not to give it any.

This fixes both issues I have described in the bpo.

https://bugs.python.org/issue42143

…ing the func object func_dealloc() does not handle partially-created objects. Best not to give it any.

Jongy · 2020-10-25T00:00:26Z

Objects/funcobject.c


+ /* __module__: If module name is in globals, use it.
+ Otherwise, use None. */
+ module = PyDict_GetItemWithError(globals, __name__);


This now runs before Py_INCREF(globals), not sure if relevant (because our caller should hold a reference to globals, right?)
Can put Py_INCREF(globals) before this call.

Jongy · 2020-10-26T19:28:31Z

@serhiy-storchaka leftovers from #11112 :)

serhiy-storchaka

BTW, the other possible way is to move initialization of func_qualname before setting func_module.

serhiy-storchaka · 2020-10-27T08:05:52Z

Thank you for catching this Jongy!

Since the bug is present in released versions, please add a NEWS entry for this fix.

Jongy · 2020-10-27T09:29:07Z

BTW, the other possible way is to move initialization of func_qualname before setting func_module.

Yes, I mentioned it in the bpo, but it will also require to check with _PyObject_GC_IS_TRACKED before untracking the object in func_dealloc. So I preferred the current solution.

Thank you for catching this Jongy!
Since the bug is present in released versions, please add a NEWS entry for this fix.

Sure, I'll add an entry later today 👍

I can also add a small test featuring my crashing snippet from the bpo - a test that makes sure the construction of a FunctionType handles exceptions gracefully. Do you think it is needed?

serhiy-storchaka · 2020-10-27T09:42:29Z

So I preferred the current solution.

Agree.

I can also add a small test featuring my crashing snippet from the bpo - a test that makes sure the construction of a FunctionType handles exceptions gracefully. Do you think it is needed?

It is not required, but it would be nice. Add test.support.gc_collect() to ensure that the function object be deallocated even on implementations without reference counters.

Jongy · 2020-10-27T22:03:45Z

Pushed a NEWS entry. Also added 2 small fixes (spacing + missing Py_XDECREF 😅 )

About the tests - I couldn't find a rightful place to put them in... If you got any idea, I'd be glad to hear, otherwise we can just merge it as is.

As for the commit message - I think the message of the first commit suffices.

miss-islington · 2020-10-29T09:58:56Z

Thanks @Jongy for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8, 3.9.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

bedevere-bot · 2020-10-29T09:59:25Z

GH-23021 is a backport of this pull request to the 3.9 branch.

…ing the func object (pythonGH-22953) func_dealloc() does not handle partially-created objects. Best not to give it any. (cherry picked from commit 3505261) Co-authored-by: Yonatan Goldschmidt <[email protected]>

bedevere-bot · 2020-10-29T09:59:34Z

GH-23022 is a backport of this pull request to the 3.8 branch.

serhiy-storchaka · 2020-10-29T09:59:46Z

Thank you for your contribution @Jongy!

…ing the func object (GH-22953) func_dealloc() does not handle partially-created objects. Best not to give it any. (cherry picked from commit 3505261) Co-authored-by: Yonatan Goldschmidt <[email protected]>

…ing the func object (GH-22953) (GH-23021) func_dealloc() does not handle partially-created objects. Best not to give it any. (cherry picked from commit 3505261) Co-authored-by: Yonatan Goldschmidt <[email protected]>

Jongy · 2020-10-29T13:56:48Z

Thank you for reviewing, @serhiy-storchaka :)

…lots1 * origin/master: (365 commits) bpo-42029: Remove IRIX code (pythonGH-23023) bpo-42143: Ensure PyFunction_NewWithQualName() can't fail after creating the func object (pythonGH-22953) bpo-34204: Use pickle.DEFAULT_PROTOCOL in shelve (pythonGH-19639) bpo-41805: Documentation for PEP 585 (pythonGH-22615) bpo-42161: Micro-optimize _collections._count_elements() (pythonGH-23008) bpo-42161: Remove private _PyLong_Zero and _PyLong_One (pythonGH-23003) bpo-42099: Fix reference to ob_type in unionobject.c and ceval (pythonGH-22829) bpo-41659: Disallow curly brace directly after primary (pythonGH-22996) bpo-6761: Enhance __call__ documentation (pythonGH-7987) bpo-42161: Modules/ uses _PyLong_GetZero() and _PyLong_GetOne() (pythonGH-22998) bpo-41474, Makefile: Add dependency on cpython/frameobject.h (pythonGH-22999) bpo-42157: Rename unicodedata.ucnhash_CAPI (pythonGH-22994) bpo-42161: Use _PyLong_GetZero() and _PyLong_GetOne() (pythonGH-22995) bpo-30681: Support invalid date format or value in email Date header (pythonGH-22090) bpo-42161: Add _PyLong_GetZero() and _PyLong_GetOne() (pythonGH-22993) bpo-42123: Run the parser two times and only enable invalid rules on the second run (pythonGH-22111) bpo-42157: Convert unicodedata.UCD to heap type (pythonGH-22991) bpo-42157: unicodedata avoids references to UCD_Type (pythonGH-22990) bpo-39101: Fixes BaseException hang in IsolatedAsyncioTestCase. (pythonGH-22654) bpo-1635741: _PyUnicode_Name_CAPI moves to internal C API (pythonGH-22713) ...

…ing the func object (pythonGH-22953) func_dealloc() does not handle partially-created objects. Best not to give it any.

bpo-42143: Ensure PyFunction_NewWithQualName() can't fail after creat…
57b3641
…ing the func object func_dealloc() does not handle partially-created objects. Best not to give it any.

the-knights-who-say-ni added the CLA signed label Oct 24, 2020

bedevere-bot added the awaiting review label Oct 24, 2020

Jongy commented Oct 25, 2020
View reviewed changes

serhiy-storchaka approved these changes Oct 27, 2020
View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Oct 27, 2020

serhiy-storchaka added type-bug An unexpected behavior, bug, or error needs backport to 3.9 labels Oct 27, 2020

Jongy added 3 commits October 28, 2020 00:00

Add NEWS entry
44efc82

Add missing XDECREF
67c0bc1

Fix broken spacing
ecf9d50

Fix RST syntax
52c1da4

Jongy requested a review from serhiy-storchaka October 28, 2020 23:23

serhiy-storchaka approved these changes Oct 29, 2020
View reviewed changes

serhiy-storchaka merged commit 3505261 into python:masterOct 29, 2020

bedevere-bot removed awaiting merge needs backport to 3.9 labels Oct 29, 2020

bedevere-bot removed the needs backport to 3.8 label Oct 29, 2020

Jongy deleted the bpo-42143 branch October 29, 2020 13:50

Uh oh!

bpo-42143: Ensure PyFunction_NewWithQualName() can't fail after creating the func object#22953

bpo-42143: Ensure PyFunction_NewWithQualName() can't fail after creating the func object #22953

Uh oh!

Conversation

Jongy commented Oct 24, 2020• edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JongyOct 25, 2020

Choose a reason for hiding this comment

Uh oh!

Jongy commented Oct 26, 2020

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented Oct 27, 2020

Uh oh!

Jongy commented Oct 27, 2020

Uh oh!

serhiy-storchaka commented Oct 27, 2020

Uh oh!

Jongy commented Oct 27, 2020

Uh oh!

miss-islington commented Oct 29, 2020

Uh oh!

bedevere-bot commented Oct 29, 2020

Uh oh!

bedevere-bot commented Oct 29, 2020

Uh oh!

serhiy-storchaka commented Oct 29, 2020

Uh oh!

Jongy commented Oct 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Jongy commented Oct 24, 2020•
edited by bedevere-bot
Loading