Better tp_version_tag overflow checks in specializer

[Fidget-Spinner]

BPO	45648
Nosy	@markshannon, @Fidget-Spinner
PRs	bpo-45648: Add checks for tp_version_tag in specialize.c #29272

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2021-10-28.14:53:27.764>
labels = ['3.11']
title = 'Better tp_version_tag overflow checks in specializer'
updated_at = <Date 2021-10-28.14:53:42.278>
user = 'https://github.com/Fidget-Spinner'

bugs.python.org fields:

activity = <Date 2021-10-28.14:53:42.278>
actor = 'kj'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = []
creation = <Date 2021-10-28.14:53:27.764>
creator = 'kj'
dependencies = []
files = []
hgrepos = []
issue_num = 45648
keywords = ['patch']
message_count = 1.0
messages = ['405190']
nosy_count = 2.0
nosy_names = ['Mark.Shannon', 'kj']
pr_nums = ['29272']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = None
url = 'https://bugs.python.org/issue45648'
versions = ['Python 3.11']

Linked PRs

• gh-89811: Check for valid tp_version_tag in specializer #113558
• [3.12] Check for valid tp_version_tag in specializer (gh-89811) #114216
• [3.11] Check for valid tp_version_tag in specializer (gh-89811) #115045

[Fidget-Spinner]

Unless I'm mistaken, the current code in Python/specialize.c doesn't check for valid tp_version_tag, so version tag could overflow and it wouldn't know any better.

Ideally, we should add more robust tests for when tp_version_tag overflows, but that's a project for another day.

[lazorchakp]

Just want to report that I accidentally ran into this in the wild. I work on an instrumentation library that modifies builtin types like str and bytes, which involves a few calls to PyType_Modified. My tests call this function hundreds of thousands of times when enabling and disabling machinery.

Repeated calls to PyType_Modified eventually caused tp_version_tag for modified builtin types to reach _Py_MAX_GLOBAL_TYPE_VERSION_TAG, which in turn led to tp_version_tag == 0 on these types. Some of the recent LOAD_ATTR_* bytecode specializations rely on tp_version_tag to determine whether to continue with the specialization or fall back to the generic operation. However, with multiple types having tp_version_tag == 0, python was no longer able to correctly distinguish between different types, leading to a variety of strange (oddly hilarious) errors.

Here's a reproduction script:

import ctypes

PyType_Modified = ctypes.pythonapi.PyType_Modified
PyType_Modified.argtypes = [ctypes.py_object]

# reach _Py_MAX_GLOBAL_TYPE_VERSION_TAG
for i in range(2**17):
 PyType_Modified(list)
 [].__add__ # any attribute lookup forces a new tp_version_tag

# newly modified types will be assigned tp_version_tag 0
PyType_Modified(str)
PyType_Modified(bytes)


def get_capitalize(type_):
 return type_.capitalize


# multiple calls with `str` leads to LOAD_ATTR specialization
get_capitalize(str)
get_capitalize(str)

# specialized bytecode fails to differentiate `bytes` from `str`
# TypeError: descriptor 'capitalize' for 'str' objects doesn't apply to a 'bytes' object
get_capitalize(bytes)(b"boom")

I verified that this fails in 3.12.1 and 3.13.0a2 and does not fail in 3.11.7 (source builds using pyenv on an M1 mac). I'll be able to work around my particular issue, but I felt responsible to report this as a data point. It's possible that other instrumentation tools will eventually encounter similar issues.

Thanks for all of the recent interpreter improvements - it's been exciting to see these changes. If you'd like me to try refreshing the stale PR here or help out in any other way, I'd be happy to give it a shot.

[Fidget-Spinner]

@lazorchakp thanks for the reproducer! I can help guide you on making a PR towards fixing this if you're interested. Honestly I did not expect this to appear in the wild, so I forgot about it.

The fix would still be the same - to check tp_version_tag == 0 after analyze_descriptor calls in specialize.c.

[lazorchakp]

@Fidget-Spinner thanks for the quick response! I can definitely throw something together. This will be my first contribution to the project, so I'd really appreciate any guidance. I have a few conflicting priorities at the moment, but I'll work on getting a PR out and then hopefully you can catch anything I may have missed.

[Fidget-Spinner]

Sorry for the late reply. Do you need help or any guidance on this? If you need help I'll be happy to set up something (you can email me at my GH profile's email).

[lazorchakp]

That would be great. I think I have a reasonable solution, but I'm unsure how to test it thoroughly - I'll reach out directly

[Fidget-Spinner]

Another reproducer is at #113462.

I'm marking this as a release blocker for 3.12 and 3.13. We should still backport this to 3.11, but the chances of this occuring in that version seems to be lower.

Peter, please don't feel rushed to do this and have a good holiday! If it helps, I think you can submit a draft PR first and we can try to help you from there.

cc @Yhg1s (sorry for the holiday ping, please ignore this until you get back)