[3.9] bpo-44562: Remove invalid PyObject_GC_Del from error path of types.GenericAlias … (GH-27016) #27028
Conversation
Fidget-Spinner
added
the
🔨 test-with-buildbots
|
🤖 New build scheduled with the buildbot fleet by @Fidget-Spinner for commit d9e432e 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
bedevere-bot
removed
the
🔨 test-with-buildbots
| if (!setup_ga(alias, origin, args)) { | ||
| PyObject_GC_Del((PyObject *)alias); |
There was a problem hiding this comment.
I dislike the idea of having a code different in 3.9 and main. Why would 3.9 have different code?
It looks like a legit bug which also affects the main branch.
There was a problem hiding this comment.
I dislike tracking the object earlier, before it's fully initialized. It goes against https://bugs.python.org/issue44531
I would prefer to track the object if setup_ga() fails, so ga_dealloc() doesn't have to use PyObject_GC_UnTrack() (to not untrack if it is not track).
An alternative is to use PyObject_GC_UnTrack() in ga_dealloc(), only for this code path.
@pablogsal: I also have this issue in https://bugs.python.org/issue44531 Is it a good practice to track an object partially initialized (its traverse function may crash) only to be able to deallocate it?
I didn't check if ga_traverse() can be called (don't crash) if setup_ga() fails. In your PR, you change the function.
There was a problem hiding this comment.
The code is also like this on the main branch: https://github.com/python/cpython/blob/main/Objects/genericaliasobject.c#L655
This is a backport PR. The only part I changed was this line onwards https://github.com/python/cpython/pull/27028/files#diff-828d12085a29364c67442b193bb62906e3469fbe21367499fd62817f98190014R36.
I'm waiting for Pablo to merge #27021 first. This PR is just to test if I can fix the failures on macOS, because I can't repro them on main or 3.10.
There was a problem hiding this comment.
Sorry if I wasn't very clear, a quick summary:
- Calling
PyObject_GC_Delwhen object fails to init properly is apparently not good and caused buildbots to fail fortypes.Unionbpo-44553: Correct failure in tp_new for the union object #27008 (comment). - So after Pablo fixed
types.Union, I created PR bpo-44562: Remove invalid PyObject_GC_Del from error path of types.GenericAlias … #27016 fortypes.GenericAliasbecause it has the same pattern. - For some reason, macOS buildbot passed on main and 3.10, but failed on 3.9, even though I don't see any significant code change in
genericaliasobject.candgcmodule.c - We reverted that backport 3.9 PR, but now I think I found the issue so I'm resubmitting it.
The failures may be caused by-- I'm wrong!Py_VISITbeing called onNULLalias->parametersinga_traverse.
There was a problem hiding this comment.
Hmm, the macOS test is still failing, I think you're right here. It's probably triggering GC on improperly initialized object :(.
There was a problem hiding this comment.
@pablogsal: I also have this issue in https://bugs.python.org/issue44531 Is it a good practice to track an object partially initialized (its traverse function may crash) only to be able to deallocate it?
If you use PyGenericAlloc, the fields are initialized to NULL and the VISIT macro handles that correctly. Is not that is a good or bad practice, is that you cannot deallocate it without tracking using a regular DECREF
|
@pablogsal after applying your PR #27021 this patch passes. It looks like the unitialized fields was the culprit. IIUC, PyObject_GC_New doesn't set the fields to NULL so tp_traverse was called on unitialized data. |
Ok, let's backport this then |
|
Can you apply my PR + the backport here? |
Done. |
|
@pablogsal: Since you authored commit b324c4c, I prefer to let you merge this backport. |
(cherry picked from commit d33943a)
https://bugs.python.org/issue44562