[proxy] web.archive.org← back | site home | direct (HTTPS) ↗ | proxy home | ◑ dark◐ light

/ cpython Public

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-35983: improve and test old trashcan macros #12607

Closed
wants to merge 1 commit into from

Conversation

Copy link
Contributor

jdemeyer commented Mar 28, 2019

This is a follow-up to #11841. I made a separate PR since that one has been reviewed already.

I realized that the backwards-compatibility macros Py_TRASHCAN_SAFE_BEGIN(op) can be made safe by only using the trashcan if the class inherits directly from object. That case is safe and it's also a very common case (few builtin types have a non-trivial subtype).

https://bugs.python.org/issue35983

jdemeyer marked this pull request as ready for review Mar 29, 2019
jdemeyer requested a review from 1st1 as a code owner Mar 29, 2019
Copy link
Member

pitrou commented May 10, 2019

You need to merge/rebase and fix conflicts now.

Copy link
Contributor Author

jdemeyer commented May 14, 2019

skip news because this really goes together with #11841, it's not a separate issue.

Copy link
Contributor Author

jdemeyer commented May 14, 2019

You need to merge/rebase and fix conflicts now.

Done

Copy link
Member

pitrou commented May 14, 2019

I'm not sure it's safe to change the meaning of the old macros. @benjaminp @serhiy-storchaka What do you think?

Copy link
Contributor Author

jdemeyer commented May 14, 2019

I'm not sure it's safe to change the meaning of the old macros.

They are already changed as part of #11841. This is just changing them in a safer way.

Copy link
Member

pitrou commented May 14, 2019

AFAICT, Py_TRASHCAN_SAFE_BEGIN wasn't changed in #11841 (though it was implemented differently).

Copy link
Contributor Author

jdemeyer commented May 14, 2019

I see what you mean. Let me be very precise what this PR will fix and what it will break, hopefully clarifying things:

This will fix a crash in this case:

  1. class X uses Py_TRASHCAN_SAFE_BEGIN in its deallocator
  2. a class Y inherits from X
  3. a 50 levels deep nested instance of Y is deallocated

This will add a new crash in this case:

  1. class X uses Py_TRASHCAN_SAFE_BEGIN in its deallocator
  2. class X does not inherit directly from object, but from another base class
  3. a very deeply nested instance of X is deallocated, where this nesting does not involve any other trashcan-using class like tuple or list

I consider the first set of conditions more likely than the second, so this will fix more crashes than it introduces. But I cannot deny the possibility that it will break stuff.

Copy link
Contributor

ambv commented Aug 27, 2021

Closing in favor of GH-27693. See BPO-44874 and the linked mailing list thread for details.

ambv closed this Aug 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants