Skip to content

gh-132835: Add defensive NULL checks in mro resolution #134763

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

emmatyping
Copy link
Member

@emmatyping emmatyping commented May 27, 2025

Currently, there are a few places where type->tp_mro could theoretically be NULL, but are not in practice. We should defensively assert these NULL values don't occur to ensure that any changes do not introduce a crash and that state invariants are upheld.

The assertions added in this PR are all instances where a NULL value would get passed to something not expected a NULL, so it is better to catch an assertion failure than crash later on.

There are a few cases where it is OK for the return of lookup_tp_mro to be NULL, such as when passed to is_subtype_with_mro, which handles this explicitly.

Currently, there are a few places where tp_mro could theoretically
become NULL, but do not in practice. We should defensively check for
NULL values to ensure that any changes do not introduce a crash and that
state invariants are upheld.
@picnixz picnixz requested a review from JelleZijlstra May 27, 2025 15:13
@emmatyping emmatyping added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 1, 2025
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @emmatyping for commit c305485 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F134763%2Fmerge

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 1, 2025
@emmatyping
Copy link
Member Author

The WASI failure is unrelated and should be fixed by python/buildmaster-config#601

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@picnixz
Copy link
Member

picnixz commented Jun 1, 2025

@emmatyping In general, it's better to wait until buildbots are done before committing something new because otherwise we won't see the status of the build on GitHub (it's still visible on the buildbots page but it's harder to find) (usually, it takes around 3-4 hours for the interesting build bots, but the full run can take up to 12 hours)

@emmatyping
Copy link
Member Author

@picnixz Oh! Sorry, I seem to have misunderstood how the buildbots work with Github. It's a bit confusing that they only start showing up when they're ready as opposed to when they've been queued, it makes it much harder to know how many to expect unless you know already :/

I generally would add the label and see a number of buildbots show up as active or pending and thought those were the set of buildbots that would be run. I updated the branch when I didn't see any more active buildbots running.

I'll definitely be more patient with the builbots going forward. Thank you for letting me know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants