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

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

Merged
merged 5 commits into from
Oct 29, 2020

Conversation

Jongy
Copy link
Contributor

@Jongy Jongy commented Oct 24, 2020

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.
@@ -19,9 +19,21 @@ PyFunction_NewWithQualName(PyObject *code, PyObject *globals, PyObject *qualname
return NULL;
}

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

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

Jongy commented Oct 26, 2020

@serhiy-storchaka leftovers from #11112 :)

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

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

@serhiy-storchaka serhiy-storchaka added type-bug An unexpected behavior, bug, or error needs backport to 3.9 only security fixes needs backport to 3.8 labels Oct 27, 2020
@serhiy-storchaka
Copy link
Member

Thank you for catching this Jongy!

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

@Jongy
Copy link
Contributor Author

Jongy commented Oct 27, 2020

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
Copy link
Member

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
Copy link
Contributor Author

Jongy commented Oct 27, 2020

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.

@serhiy-storchaka serhiy-storchaka merged commit 3505261 into python:master Oct 29, 2020
@miss-islington
Copy link
Contributor

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
Copy link

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 29, 2020
…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]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 29, 2020
…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
Copy link

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

@serhiy-storchaka
Copy link
Member

Thank you for your contribution @Jongy!

miss-islington added a commit that referenced this pull request Oct 29, 2020
…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]>
serhiy-storchaka pushed a commit that referenced this pull request Oct 29, 2020
…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 Jongy deleted the bpo-42143 branch October 29, 2020 13:50
@Jongy
Copy link
Contributor Author

Jongy commented Oct 29, 2020

Thank you for reviewing, @serhiy-storchaka :)

shihai1991 added a commit to shihai1991/cpython that referenced this pull request Oct 29, 2020
…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)
  ...
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
…ing the func object (pythonGH-22953)

func_dealloc() does not handle partially-created objects. Best not to give it any.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants