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

asserting that the tstate pointer is non-NULL using assert #122928

Closed
Wulian233 opened this issue Aug 12, 2024 · 5 comments
Closed

asserting that the tstate pointer is non-NULL using assert #122928

Wulian233 opened this issue Aug 12, 2024 · 5 comments
Labels
type-bug An unexpected behavior, bug, or error

Comments

@Wulian233
Copy link
Contributor

Wulian233 commented Aug 12, 2024

Bug report

Bug description:

https://github.com/python/cpython/blob/main/Python/pystate.c#L3016

To prevent dangling pointer issues, adding a check for NULL ensures that invalid pointers are not dereferenced

Linked PRs

@ZeroIntensity
Copy link
Member

As mentioned in the PR, a NULL check doesn't fix dangling pointers.

With that being said, calling _PyThreadState_MustExit with an invalid thread state sounds like a bug, is there a specific case of this happening?

@neonene
Copy link
Contributor

neonene commented Aug 13, 2024

My understanding is that a problematic dangling pointer could be caused by the legacy non-isolated subinterpreter created with Py_NewInterpreter(), allowing daemon threads. Now that Python uses and recommends the new isolated subinterpreter, I do not think the concern is worth much consideration.

See also #109794 (comment), #104341 (comment).

@picnixz
Copy link
Contributor

picnixz commented Aug 14, 2024

I'm wondering whether the issue should still mention dangling pointer since a dangling pointer is a valid pointer (hence non-null) pointing to invalid memory. It's more about asserting that the pointer is non-NULL using assert (namely, enforcing pre-conditions).

@neonene
Copy link
Contributor

neonene commented Aug 15, 2024

Please change the title, or make the PR belong to gh-109793. The current title is not good for closing here as "completed".

@Wulian233 Wulian233 changed the title tstate might be a dangling pointer asserting that the tstate pointer is non-NULL using assert Aug 15, 2024
@vstinner
Copy link
Member

The change is not needed: #122929 (review)

@ZeroIntensity ZeroIntensity closed this as not planned Won't fix, can't repro, duplicate, stale Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

5 participants