GH-99293: Fix stale method caches and assertion errors in SWIG-generated modules - #99294
GH-99293: Fix stale method caches and assertion errors in SWIG-generated modules#99294Yhg1s wants to merge 2 commits into
Conversation
Sorry, something went wrong.
modules. Extension modules generated by SWIG up to version 4.1.0 clear the Py_TPFLAGS_VALID_VERSION_TAG bit from tp_flags when modifying the type, as they should, but do not update tp_version_tag as typeobject.c expects. (For example, merely one of many instances I've found: https://github.com/OSGeo/gdal/blob/6bd07b20b3e55c2fc94da611244a615a4fd2991f/swig/python/extensions/osr_wrap.cpp#L2296) In release builds this means a potentially stale cached entry is used, and in debug builds (or release builds that happen to enable assertions, which as it turns out is actually a good idea) it triggers an assertion error. SWIG 4.1.0 avoids this problem by using PyType_Modified(), but there are a lot of older versions of SWIG and of checked-in generated extension modules around.
Fidget-Spinner
left a comment
There was a problem hiding this comment.
LGTM. However, let's wait a bit for others to review.
Sorry, something went wrong.
| assert(_PyType_HasFeature(type, Py_TPFLAGS_VALID_VERSION_TAG)); | ||
| return entry->value; | ||
| } | ||
|
|
There was a problem hiding this comment.
For other reviewers wondering what happens down this code path if Py_TPFLAGS_VALID_VERSION_TAG is not set:
- A normal lookup happens where we walk the MRO.
- We assign a new version tag to the type. This sets Py_TPFLAGS_VALID_VERSION_TAG.
So this should be safe.
Sorry, something went wrong.
| @@ -0,0 +1 @@ | |||
| Fix an issue where extension modules could inadvertently trigger an assertion error in typeobject.c by clearing the Py_TPFLAGS_VALID_VERSION_TAG tp_flag without clearing the tp_version_tag field. (Extension modules should use PyType_Modified() instead, however.) | |||
There was a problem hiding this comment.
I just noticed Py_TPFLAGS_VALID_VERSION_TAG isn't actually documented.
| Fix an issue where extension modules could inadvertently trigger an assertion error in typeobject.c by clearing the Py_TPFLAGS_VALID_VERSION_TAG tp_flag without clearing the tp_version_tag field. (Extension modules should use PyType_Modified() instead, however.) | |
| Fix an issue where extension modules could inadvertently trigger an assertion error in ``typeobject.c`` by clearing the ``Py_TPFLAGS_VALID_VERSION_TAG`` :c:member:`~PyTypeObject.tp_flags` without clearing the :c:member:`~PyTypeObject.tp_version_tag` field. (Extension modules should use :c:func:`PyType_Modified` instead, however.) |
Sorry, something went wrong.
|
Copying a comment I made elsewhere: The code will crash return incorrect cached methods and values on 3.11 when running specialised instructions. Those only use tp_version_tag as a guard and don't check Py_TPFLAGS_VALID_VERSION_TAG for speed reasons. One fix is to add a guard to all specialised LOAD_METHOD/ATTR instructions to check for the flag (this will slow down all those instructions a little). Another fix is to ask everyone to regenerate their code with a newer SWIG for Python 3.11.0. |
Sorry, something went wrong.
|
This PR is stale because it has been open for 30 days with no activity. |
Sorry, something went wrong.
Extension modules generated by SWIG up to version 4.1.0 clear the Py_TPFLAGS_VALID_VERSION_TAG bit from tp_flags when modifying the type, as they should, but do not update tp_version_tag as typeobject.c expects. (For example, merely one of many instances I've found: https://github.com/OSGeo/gdal/blob/6bd07b20b3e55c2fc94da611244a615a4fd2991f/swig/python/extensions/osr_wrap.cpp#L2296)
In release builds this means a potentially stale cached entry is used, and in debug builds (or release builds that happen to enable assertions, which as it turns out is actually a good idea) it triggers an assertion error.
SWIG 4.1.0 avoids this problem by using PyType_Modified(), but there are a lot of older versions of SWIG and of checked-in generated extension modules around.