Skip to content

fix: null pointer dereference in attr_with_type_hint #5576

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

Merged
merged 15 commits into from
Mar 23, 2025

Conversation

InvincibleRMC
Copy link
Contributor

@InvincibleRMC InvincibleRMC commented Mar 21, 2025

Description

Fixes regression caught in #5575

Suggested changelog entry:

    Fix bug in ``attr_with_type_hint`` to allow objects to be in ``attr_with_type_hint``

@rwgk
Copy link
Collaborator

rwgk commented Mar 21, 2025

@timohl looks good to me (the suggestions I posted are pretty minor), could you please take a quick look, too?

@kvarg
Copy link

kvarg commented Mar 21, 2025

I believe theres still a dereference issue since the function record is also set to a nullptr. This line will be hit if the user has not yet created bindings for the class:

} else if (func_rec->is_new_style_constructor && arg_index == 0) {

I guess the correct behaviour would be a pybind11 failure?

@rwgk
Copy link
Collaborator

rwgk commented Mar 21, 2025

I believe theres still a dereference issue since the function record is also set to a nullptr. This line will be hit if the user has not yet created bindings for the class:

} else if (func_rec->is_new_style_constructor && arg_index == 0) {

I guess the correct behaviour would be a pybind11 failure?

Thanks @kvarg for pointing this out, I totally overlooked that func_rec can be nullptr. I believe we need additional guards in multiple places. We have is_annotation, but that's currently used in only one place.

Ideally, for each additional guard we're adding, we should have a corresponding unit test.

@InvincibleRMC
Copy link
Contributor Author

I believe theres still a dereference issue since the function record is also set to a nullptr. This line will be hit if the user has not yet created bindings for the class:

} else if (func_rec->is_new_style_constructor && arg_index == 0) {

I guess the correct behaviour would be a pybind11 failure?

I got it to have the same error as using m.attr() without creating bindings. "AttributeError: 'module' object has no attribute 'foo4'

@InvincibleRMC
Copy link
Contributor Author

Currently doing the following results in the following.

struct foo4 {};
 m.attr_with_type_hint<foo4>("foo4") = 3;

It makes the type hint test_submodule_pytypes(module_&)::foo4. This is the same result as doing it with m.def() so I assume this is the intended type.

Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

This seems fine as a fix for a regression.

But I realize now, it's generally not a great pattern to create a dummy object (the function record) to reuse a complex function. Careful factoring would be better, so that generate_type_signature<T>() does not involve a function record at all.

Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

I fixed a tiny typo: 07c8453

Waiting for GitHub Actions to finish, then I'll merge.

@rwgk rwgk merged commit 566894d into pybind:master Mar 23, 2025
80 checks passed
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Mar 23, 2025
@henryiii henryiii changed the title Fix null pointer dereference in attr_with_type_hint fix: null pointer dereference in attr_with_type_hint Mar 31, 2025
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Apr 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants