Skip to content

bpo-44553: Correct failure in tp_new for the union object #27008

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

Merged
merged 1 commit into from
Jul 3, 2021

Conversation

pablogsal
Copy link
Member

@pablogsal pablogsal commented Jul 3, 2021

@pablogsal
Copy link
Member Author

Before this PR:

❯ ./python -m test test_types -m test_or_type_operator_with_genericalias
0:00:00 load avg: 0.92 [1/1] test_types
Objects/object.c:1915: _Py_ForgetReference: Assertion failed: invalid object chain
Enable tracemalloc to get the memory block allocation traceback

object address  : 0x7ffff671f110
object refcount : 0
object type     : 0x555555a84dc0
object type name: tuple
object repr     : <refcnt 0 at 0x7ffff671f110>

Fatal Python error: _PyObject_AssertFailed: _PyObject_AssertFailed

after this PR:

❯ ./python -m test test_types -m test_or_type_operator_with_genericalias
0:00:00 load avg: 0.75 Run tests sequentially
0:00:00 load avg: 0.75 [1/1] test_types

== Tests result: SUCCESS ==

1 test OK.

Total duration: 224 ms
Tests result: SUCCESS

@pablogsal
Copy link
Member Author

I'm landing this as soon as the tests pass to unblock the buildbots

if (result->args == NULL) {
PyObject_GC_Del(result);
Copy link
Member Author

Choose a reason for hiding this comment

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

@Fidget-Spinner For next occasions, the problem with this is that PyObject_GC_Del cannot be called like that because it overrides a bunch of cleanups like the call to _Py_ForgetReference and other possible handling.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry bout that. I'll keep that in mind in the future.

BTW, does this mean this should be changed as well? https://github.com/python/cpython/blob/main/Objects/genericaliasobject.c#L654
I'm confused as to why it doesn't break the buildbots, or maybe because none of our tests make the object setup fail so PyObject_GC_Del is never called?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that has exactly the same problem. I assume is not being reaches by the tests and that's why we are not seeing corruption.

@pablogsal pablogsal merged commit bc39614 into python:main Jul 3, 2021
@pablogsal pablogsal deleted the bpo-44553 branch July 3, 2021 20:00
@miss-islington
Copy link
Contributor

Thanks @pablogsal for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-27009 is a backport of this pull request to the 3.10 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Jul 3, 2021
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 3, 2021
pablogsal added a commit that referenced this pull request Jul 3, 2021
…H-27009)

(cherry picked from commit bc39614)

Co-authored-by: Pablo Galindo <[email protected]>

Co-authored-by: Pablo Galindo <[email protected]>
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.

5 participants