Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-91051: fix segfault when using all 8 type watchers #107853

Merged
merged 2 commits into from
Aug 11, 2023

Conversation

carljm
Copy link
Member

@carljm carljm commented Aug 11, 2023

The type watcher implementation adds char tp_watched; to the PyTypeObject struct, and then in PyType_Modified it assigns int bits = type->tp_watched; and then treats bits as a bitset, expecting that only the low 8 bits should ever be set.

On platforms where char is signed (e.g. x86), if the top bit in tp_watched is set, the cast to int bits will sign-extend with 1s, and we will thus have a lot of high bits set in bits that are not supposed to ever be set.

On x86 in a debug build, without the fix in this PR, the new test hits the assertion in PyType_Modified that only bit positions < TYPE_MAX_WATCHERS should be set, and aborts.

The fix is simple: tp_watched should be defined as unsigned char so that it is zero-extended, not sign-extended.

Note that the test is carefully designed to not assume there are no type watchers already active. It doesn't attempt to register a fixed number of type watchers, instead it just registers type watchers until it reaches TYPE_MAX_WATCHERS - 1, the last available slot and the one that can trigger this bug (since it will set the highest bit in tp_watched when watching a type.)

@carljm
Copy link
Member Author

carljm commented Aug 11, 2023

@Yhg1s I think this fix should be backported to 3.12.

Copy link
Contributor

@itamaro itamaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! we're going to want to backport to 3.12!

@ambv
Copy link
Contributor

ambv commented Aug 11, 2023

Closing and re-opening to retrigger CLA checks. Sorry for the noise.

@ambv ambv closed this Aug 11, 2023
@ambv ambv reopened this Aug 11, 2023
@carljm carljm added the needs backport to 3.12 bug and security fixes label Aug 11, 2023
@carljm carljm merged commit 66e4edd into python:main Aug 11, 2023
@miss-islington
Copy link
Contributor

Thanks @carljm for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

@carljm carljm deleted the typewatchersign branch August 11, 2023 18:42
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 11, 2023
…-107853)

(cherry picked from commit 66e4edd)

Co-authored-by: Carl Meyer <carl@oddbird.net>
@bedevere-bot
Copy link

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

@bedevere-bot bedevere-bot removed the needs backport to 3.12 bug and security fixes label Aug 11, 2023
Yhg1s pushed a commit that referenced this pull request Aug 16, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…) (#107876)

* gh-91051: fix segfault when using all 8 type watchers (GH-107853)
(cherry picked from commit 66e4edd)

Co-authored-by: Carl Meyer <carl@oddbird.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants