Better tp_version_tag overflow checks in specializer

@markshannon

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

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.

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.

@lazorchakp

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

@Fidget-Spinner

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

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

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

@Yhg1s

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)

GH-114216 is a backport of this pull request to the 3.12 branch.

Merge branch '3.12' into pythongh-89811-3.12-backport

[3.12] Check for valid tp_version_tag in specializer (gh-89811) (gh-1…

pythongh-89811: Check for valid tp_version_tag in specializer (python…

This is marked as a release-blocker with the 3.11 tag. The issue is still open but I am not sure if this was expected to be backported to 3.11. It's there anything left for us to do here or can I proceed with the release?

@pablogsal

@pablogsal This does need a backport to 3.11, but I'm currently working on it. Sorry for the delay - I'll have a PR up as soon as possible

pythongh-89811: Check for valid tp_version_tag in specializer (python…

GH-115045 is a backport of this pull request to the 3.12 branch.

GH-115045 is a backport of this pull request to the 3.11 branch.

[3.11] gh-89811: Check for valid tp_version_tag in specializer (GH-11…

pythongh-89811: Check for valid tp_version_tag in specializer (python…

Navigation Menu

Repository navigation

Better tp_version_tag overflow checks in specializer #89811

Linked PRs

9 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Better tp_version_tag overflow checks in specializer #89811

Description

Linked PRs

Activity

Fidget-Spinner commented on Oct 28, 2021

lazorchakp commented on Dec 12, 2023

Fidget-Spinner commented on Dec 12, 2023

lazorchakp commented on Dec 12, 2023

Fidget-Spinner commented on Dec 20, 2023

lazorchakp commented on Dec 21, 2023

Fidget-Spinner commented on Dec 27, 2023

9 remaining items

bedevere-app commented on Jan 17, 2024

pablogsal commented on Feb 5, 2024

lazorchakp commented on Feb 5, 2024

bedevere-app commented on Feb 5, 2024

bedevere-app commented on Feb 5, 2024

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions