GeezerVison is showing github.com — `_PyStaticType_Dealloc` does not invalidate type version tag · Issue #101696 · python/cpython
_PyStaticType_Dealloc does not invalidate type version tag #101696
Labels 3.11only security fixesonly security fixes3.12only security fixesonly security fixesinterpreter-core(Objects, Python, Grammar, and Parser dirs)(Objects, Python, Grammar, and Parser dirs)type-bugAn unexpected behavior, bug, or errorAn unexpected behavior, bug, or errortype-crashA hard crash of the interpreter, possibly with a core dumpA hard crash of the interpreter, possibly with a core dump Description_PyStaticType_Dealloc does not invalidate the type version tag and when the interpreter is reinitialized it still points to the old index. It should be cleared after the runtime finalization to avoid a rare case where there is a cache hit from the old index.
Found while working on https://github.com/python/cpython/actions/runs/4112993949/jobs/7098592004
Linked PRs
Activitykumaraditya303 commented on Feb 8, 2023 ContributorAuthor More actions Reacted by Erlend E. Aasland erlend-aasland commented on Feb 8, 2023 Good job debugging that! 👏🏻👏🏻👏🏻 changed the title [-]`_PyStaticType_Dealloc` does not invalidates type version tag[/-] [+]`_PyStaticType_Dealloc` does not invalidate type version tag[/+] on Feb 9, 2023markshannon commented on Feb 9, 2023 Given that static types should be immutable and immortal, I think a better approach would be reserve low values (up to a 1000 or so) for static classes. Once allocated, a static type would keep the same version number for the lifetime of the process.
It might be handy to pre-allocate a few values for the most common types, but that's not necessary for correctness. Reacted by Erlend E. Aasland kumaraditya303 commented on Feb 9, 2023 ContributorAuthor More actions
Given that static types should be immutable and immortal, I think a better approach would be reserve low values (up to a 1000 or so) for static classes. Once allocated, a static type would keep the same version number for the lifetime of the process. It might be handy to pre-allocate a few values for the most common types, but that's not necessary for correctness.
Yeah, it makes sense and is on my todo list for a long time See #95795 Reacted by Eric Snow and Erlend E. Aasland erlend-aasland commented on Feb 9, 2023 Makes sense to me as well. As a proof-of-concept, reverting #101697 and applying the following seems to work:
Details
diff --git a/Objects/typeobject.c b/Objects/typeobject.c
index bf6ccdb77a..bc48845300 100644
--- a/Objects/typeobject.c
+++ b/Objects/typeobject.c
@@ -229,6 +229,13 @@ _PyType_CheckConsistency(PyTypeObject *type)
CHECK(type->tp_traverse != NULL);
}
+ if (_PyType_HasFeature(type, Py_TPFLAGS_VALID_VERSION_TAG)) {
+ CHECK(type->tp_version_tag != 0);
+ }
+ else {
+ CHECK(type->tp_version_tag == 0);
+ }
+
if (type->tp_flags & Py_TPFLAGS_DISALLOW_INSTANTIATION) {
CHECK(type->tp_new == NULL);
CHECK(PyDict_Contains(type->tp_dict, &_Py_ID(__new__)) == 0);
@@ -4469,8 +4476,6 @@ _PyStaticType_Dealloc(PyTypeObject *type)
}
type->tp_flags &= ~Py_TPFLAGS_READY;
- type->tp_flags &= ~Py_TPFLAGS_VALID_VERSION_TAG;
- type->tp_version_tag = 0;
if (type->tp_flags & _Py_TPFLAGS_STATIC_BUILTIN) {
_PyStaticType_ClearWeakRefs(type);
@@ -6968,6 +6973,8 @@ PyType_Ready(PyTypeObject *type)
/* Historically, all static types were immutable. See bpo-43908 */
if (!(type->tp_flags & Py_TPFLAGS_HEAPTYPE)) {
+ type->tp_version_tag = next_version_tag++;
+ type->tp_flags |= Py_TPFLAGS_VALID_VERSION_TAG;
type->tp_flags |= Py_TPFLAGS_IMMUTABLETYPE;
}
ericsnowcurrently commented on Feb 9, 2023 @erlend-aasland, your patch seems correct. I was going to ask what's the goal of your fix, since the merged fix addressed the originally described failure and gh-95795 addresses the rest. However, I think your fix might also deal with non-builtin static types (e.g. from community extension modules), which would probably be worth fixing too (regardless of how remote the possible failure).
I have some additional feedback, but I can wait until you've made a PR. (Feel free to request a review from me.) Reacted by Erlend E. Aasland erlend-aasland commented on Feb 9, 2023 Thanks for the comment, @ericsnowcurrently. I've alredy made a PR; I've requested your review. The PR is slightly different from my proposed patch here. I would also like to add some additional asserts, for example in PyType_Modified and assign_version_tag (naming might differ; I'm on my phone right now.) erlend-aasland commented on Feb 9, 2023 As for the goal, I think Mark's reasoning is correct; making sure all immutable types have a lifelong (and immutable) valid version feels more correct than a fixup during finalise. kumaraditya303 commented on Feb 9, 2023 ContributorAuthor More actions
As for the goal, I think Mark's reasoning is correct; making sure all immutable types have a lifelong (and immutable) valid version feels more correct than a fixup during finalise.
That's the ideal goal yes but your PR only achieve valid version for only one cycle 1 of interpreter initialization, it is reset as PyType_Ready is called on every cycle so not quite same as valid across the whole process lifetime. One side-effect is that it will now reset the tag for extension modules too which I avoided in my PR but I don't see any issues with doing that either.
#95795 is the complete solution for this, there is a lot of info about this in that thread.
Reacted by Eric Snow and Erlend E. Aasland erlend-aasland commented on Feb 9, 2023 True, Kumar, my PR is not a complete solution. kumaraditya303 commented on Feb 10, 2023 ContributorAuthor More actions
However, I think your fix might also deal with non-builtin static types (e.g. from community extension modules), which would probably be worth fixing too (regardless of how remote the possible failure).
Actually #101742 does not does this. The reason is that once a type is initialized via PyType_Ready it is never deallocated (in the sense that the tp_dict and other things are never cleared) and if the interpreter is reinitialized then it uses the same old initialized type. PyType_Ready returns early if type is already initialized by previous interpreter.
|
int |
|
PyType_Ready(PyTypeObject *type) |
|
{ |
|
if (type->tp_flags & Py_TPFLAGS_READY) { |
|
assert(_PyType_CheckConsistency(type)); |
|
return 0; |
|
} |
So because of this #101742 has no effect because that code is only called once and subsequent initializations are skipped entirely. This happens in core static types because they are deallocated and reinitialized on every cycle unlike extension modules. ericsnowcurrently commented on Feb 10, 2023
Actually #101742 does not does this.
Good point. erlend-aasland commented on Feb 10, 2023 Based on the discussion, I suggest closing #101742 and instead go for a complete fix, as proposed in Kumar's issue #95795. I could give it a go, just for fun1. OTOH, I don't want to waste review time, so I'm fine with leaving it to Kumar.
Footnotes
-
it'd be a nice opportunity for me to getting to know other parts of the code base better. ↩
Reacted by Eric Snow and Kumar Aditya kumaraditya303 commented on Feb 11, 2023 ContributorAuthor More actions The backport is merged so closing.
I could give it a go, just for fun
@erlend-aasland I'd say give it a shot, the points of implementation are written here #95795 (comment) and we can work together on this. Reacted by Erlend E. Aasland erlend-aasland commented on Feb 11, 2023 Thanks, Kumar. I'll see if I can have something for you by the next weekend.
MetadataMetadata3.11only security fixesonly security fixes3.12only security fixesonly security fixesinterpreter-core(Objects, Python, Grammar, and Parser dirs)(Objects, Python, Grammar, and Parser dirs)type-bugAn unexpected behavior, bug, or errorAn unexpected behavior, bug, or errortype-crashA hard crash of the interpreter, possibly with a core dumpA hard crash of the interpreter, possibly with a core dump DevelopmentNo branches or pull requestsIssue actions
|