GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — bpo-42143: Ensure PyFunction_NewWithQualName() can't fail after creating the func object by Jongy · Pull Request #22953 · python/cpython

Conversation

Jongy commented Oct 24, 2020 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ZackerySpytz reacted with thumbs up emoji

All reactions

👍 1 reaction


          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

Comment thread

Objects/funcobject.c


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

Jongy Oct 25, 2020

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Jongy commented Oct 26, 2020

Copy link

Copy Markdown

Contributor Author

@serhiy-storchaka leftovers from #11112 :)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka approved these changes

Oct 27, 2020

View reviewed changes

serhiy-storchaka left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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

serhiy-storchaka commented Oct 27, 2020

Copy link

Copy Markdown

Member

Thank you for catching this Jongy!

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Jongy commented Oct 27, 2020

Copy link

Copy Markdown

Contributor Author

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka commented Oct 27, 2020

Copy link

Copy Markdown

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 reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Jongy added 3 commits

October 28, 2020 00:00


          Add NEWS entry

44efc82


          Add missing XDECREF

67c0bc1


          Fix broken spacing

ecf9d50

Jongy commented Oct 27, 2020

Copy link

Copy Markdown

Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          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:master

Oct 29, 2020

miss-islington commented Oct 29, 2020

Copy link

Copy Markdown

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!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot removed awaiting merge needs backport to 3.9 labels

Oct 29, 2020

bedevere-bot commented Oct 29, 2020

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Oct 29, 2020


          bpo-42143: Ensure PyFunction_NewWithQualName() can't fail after creat…

2862ad0

…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


          bpo-42143: Ensure PyFunction_NewWithQualName() can't fail after creat…

c939d6c

…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 commented Oct 29, 2020

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot removed the needs backport to 3.8 label

Oct 29, 2020

serhiy-storchaka commented Oct 29, 2020

Copy link

Copy Markdown

Member

Thank you for your contribution @Jongy!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

miss-islington added a commit that referenced this pull request

Oct 29, 2020


          bpo-42143: Ensure PyFunction_NewWithQualName() can't fail after creat…

9ede1b0

…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


          bpo-42143: Ensure PyFunction_NewWithQualName() can't fail after creat…

60324d2

…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 deleted the bpo-42143 branch

October 29, 2020 13:50

Jongy commented Oct 29, 2020

Copy link

Copy Markdown

Contributor Author

Thank you for reviewing, @serhiy-storchaka :)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

shihai1991 added a commit to shihai1991/cpython that referenced this pull request

Oct 29, 2020


          Merge remote-tracking branch 'origin/master' into bpo_41073_add_types…

b3f992a

…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


          bpo-42143: Ensure PyFunction_NewWithQualName() can't fail after creat…

29812cf

…ing the func object (pythonGH-22953)

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

Reviewers

serhiy-storchaka

serhiy-storchaka approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

type-bug

An unexpected behavior, bug, or error

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

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.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

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!

Jongy Oct 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

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Jongy commented Oct 24, 2020 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.