Skip to content

/ cpython

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

Jump to bottom

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

Merged
merged 1 commit into from Jul 3, 2021
Merged

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

merged 1 commit into from Jul 3, 2021
+2 −2

Conversation

@pablogsal
Copy link
Member

@pablogsal pablogsal commented Jul 3, 2021 •
edited by bedevere-bot

https://bugs.python.org/issue44553
@pablogsal
bpo-44553: Correct failure in tp_new for the union object
162eeeb
@pablogsal
Copy link
Member Author

@pablogsal pablogsal commented Jul 3, 2021

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

@pablogsal pablogsal commented Jul 3, 2021

I'm landing this as soon as the tests pass to unblock the buildbots
@pablogsal
pablogsal reviewed Jul 3, 2021
View changes
Objects/unionobject.c
if (result->args == NULL) {
PyObject_GC_Del(result);

This comment has been minimized.

Sign in to view
@pablogsal

pablogsal Jul 3, 2021
Author Member

@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.

This comment has been minimized.

Sign in to view
@Fidget-Spinner

Fidget-Spinner Jul 4, 2021
Contributor

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?

This comment has been minimized.

Sign in to view
@pablogsal

pablogsal Jul 4, 2021
Author Member

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
12 checks passed
12 checks passed
@github-actions
Check for source changes
Details
@github-actions
Check if generated files are up to date
Details
@github-actions
Windows (x86)
Details
@github-actions
Windows (x64)
Details
@github-actions
macOS
Details
@github-actions
Ubuntu
Details
@github-actions
Ubuntu SSL tests with OpenSSL
Details
@github-actions
Address sanitizer Address sanitizer
Details
Azure Pipelines PR #20210703.31 succeeded
Details
@travis-ci
Travis CI - Pull Request Build Passed
Details
@bedevere-bot
bedevere/issue-number Issue number 44553 found
Details
@bedevere-bot
bedevere/news "skip news" label found
@pablogsal pablogsal deleted the pablogsal:bpo-44553 branch Jul 3, 2021
@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Jul 3, 2021

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

@bedevere-bot bedevere-bot commented Jul 3, 2021

GH-27009 is a backport of this pull request to the 3.10 branch.
miss-islington added a commit to miss-islington/cpython that referenced this pull request Jul 3, 2021
@pablogsal @miss-islington
bpo-44553: Correct failure in tp_new for the union object (pythonGH-2…
5f5d50c 
…7008)

(cherry picked from commit bc3961485639cc73de7c4c7eed1b56f3c74939bf)

Co-authored-by: Pablo Galindo <Pablogsal@gmail.com>
pablogsal added a commit that referenced this pull request Jul 3, 2021
@miss-islington @pablogsal
bpo-44553: Correct failure in tp_new for the union object (GH-27008) (G…
000b9e8 
…H-27009)

(cherry picked from commit bc39614)

Co-authored-by: Pablo Galindo <Pablogsal@gmail.com>

Co-authored-by: Pablo Galindo <Pablogsal@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fidget-Spinner Fidget-Spinner

@serhiy-storchaka serhiy-storchaka

Assignees
No one assigned
Labels
CLA signed skip news
Projects
None yet
Milestone
No milestone
5 participants
@pablogsal @miss-islington @bedevere-bot @Fidget-Spinner @the-knights-who-say-ni