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

Detect and fail if using mismatched holders #1161

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jagerman
Copy link
Member

This adds a check when registering a class or a function with a holder return that the same wrapped type hasn't been previously seen using a different holder type.

This fixes #1138 by detecting the failure; currently attempting to use two different holder types (e.g. a unique_ptr and shared_ptr) in difference places can segfault because we don't have any type safety on the holder instances.

This an alternative to #1139, which I don't think will work for pybind as is (I'll comment in that PR).

This adds a check when registering a class or a function with a holder
return that the same wrapped type hasn't been previously seen using a
different holder type.

This fixes pybind#1138 by detecting the failure; currently attempting to use
two different holder types (e.g. a unique_ptr<T> and shared_ptr<T>) in
difference places can segfault because we don't have any type safety on
the holder instances.
@EricCousineau-TRI
Copy link
Collaborator

(Follow up from #1138)

Just a couple of comments / questions (quoting from this PR and your comment in the other PR):

Conversion between holder types--e.g. being able to convert an rvalue unique_ptr into a shared_ptr seems feasible, but I think it's going to need a different path than stealing the pointer and stashing it in the new type. std::shared_ptr has a std::unique_ptr && constructor for doing exactly this promotion

I have covered a constrained case of this under #1146 - by permitting moveing an rvalue unique_ptr<T> = unique_ptr<T, default_deleter<T>> holder (from class_) into a shared_ptr<T> casted return type:
https://github.com/pybind/pybind11/pull/1146/files#diff-2443092219154d8d844074fa320e45f7R1134

This also adds in a layer to check type mismatches when erasing the holders (via void* passing, and const-casting stuff) in the internal code (for additional checks):
https://github.com/pybind/pybind11/pull/1146/files#diff-caf3c493ee8ade1a606a667ccd7378d5R397
The caveat is that this only supports a specific conversion (only default_deleter<> for unique_ptr<>) due to type erasure - but I am not sure how this can be side-stepped.

Is there a way something like this could be incorporated into this new PR that you have?
(I could then shave it off of that WIP PR)

There are some other cases this PR doesn't catch that the other one does:

To follow up, sorry, I should have followed up on the old PR. The newer PR, #1146 (which is unfortunately large, so it got lost in the details), had a mechanism to handle this; however, this setup (using typeids) does seem much better!

and ideally, detect it at compile time rather than run-time

Can I ask if you have an idea of how this would work? I cannot think of anything at the moment.

@jagerman
Copy link
Member Author

Can I ask if you have an idea of how this would work? I cannot think of anything at the moment.

Well, just brainstorming a bit (because I haven't really thought this through): One possible way is with a declaration along the lines of py::implicitly_convertible<A, B>(), but for holders. Maybe something like: py::holder_convertible<A, std::unique_ptr<A>, std::shared_ptr<A>>(); which could then stash a lambda in the type information that could then be used to allow the conversion.

Or perhaps more flexible, a py::holder_conversion<A>(f) where f is a function taking a holder by rvalue reference and returning another holder, e.g.:

py::holder_conversion<A>([](std::unique_ptr<A> &&a) {
    return std::shared_ptr(std::move(a)); });

That would also allow you to do things like go the other way (albeit with a required run-time check):

py::holder_conversion<B>([](std::shared_ptr<B> &&b) {
    if (b.use_count() > 1)
        throw std::runtime_error("Cannot convert a shared_ptr with multiple refs");
    return std::unique_ptr<B>(b.release());
});

@EricCousineau-TRI
Copy link
Collaborator

Maybe something like: py::holder_convertible<A, std::unique_ptr, std::shared_ptr>();

That makes sense, but this seems like it would be strictly runtime rather than compile-time.
Also, for common holder types (shared_ptr, unique_ptr), would it be possible have these commonly registered when class_ is constructed?

a py::holder_conversion(f) where f is a function taking a holder

That makes sense too, and seems like it could ultimately be the underlying implementation for the first alternative.

That would also allow you to do things like go the other way

AFAIK, it is not possible to convert a shared_ptr to a unique_ptr in STL, as there is no thread-safe release mechanism :(

That being said, could some sort of mechanism like this be used to more intuitively release unique_ptr-held instances to C++?

(If this is too off-topic to this PR, I can move these comments to another thread.)

@wjakob
Copy link
Member

wjakob commented Nov 7, 2017

This strikes me as fairly heavy weight (code added to every function dispatcher). Can't we just point out in the docs that you're not supposed to mix holder types? It strikes me as a fairly rare design pattern to use unique_ptr and shared_ptr interchangeably for a type.

@EricCousineau-TRI
Copy link
Collaborator

True - but if it doesn't add too much weight, could there still be a way to raise an alarm if attempting to convert holder types?
Could the conversion check be delegated strictly to the holder type converters, copyable_holder_caster and move_only_holder_caster? It seems like it should be viable there (I believe).

(And just to check, are you talking about the suggestions for conversions? Or are you talking about the original PR?)

@jagerman
Copy link
Member Author

jagerman commented Nov 7, 2017

This strikes me as fairly heavy weight (code added to every function dispatcher).

I somewhat agree and am a bit ambivalent about this PR, but it's not that heavy: the code is only added to function and class initialization, not function dispatching.

@jagerman
Copy link
Member Author

jagerman commented Nov 7, 2017

Oh, I think you were referring to the holder conversion stuff in the comments above rather than the mismatched holder detection.

@EricCousineau-TRI
Copy link
Collaborator

Sorry for late-follow-up, but it seems like @tadeu may have encountered this in #1215.

Once concern with the current PR is that it may only check Return values but ignore input arguments (and possibly casting).
Could this logic be moved into the holder casters, copyable_holder_caster and move_only_holder_caster, and could the holder type info be stored directly in a type's record?

I'd be happy to make those changes if you'd like, as it'd be a nice step to incorporate bits of #1146.

@oremanj
Copy link
Contributor

oremanj commented Apr 23, 2018

I spent a while today debugging a segfault that would've given a clear error message in the presence of this PR - are there any plans to merge it?

@jagerman
Copy link
Member Author

@wjakob - thoughts on this PR? (As it is now -- there are other comments here that went off on a bit of a tangent). The basic protection here seems useful and relatively cheap—a check when registering classs and function, and yet another container in the internals struct.

@TallJimbo
Copy link

@pschella and I just encountered this, and disallowing mixed holder types is a big problem for our codebase (regardless of whether the failure mode is a segfault or a friendlier exception). We consider it good practice to use unique_ptr when you can and shared_ptr when you need to (on the same class), and having to inform downstream callers which holder was used so they can know when to wrap a smart-pointer-converting lambda instead of the original function is a workaround we'd like to avoid.

Our organization may be able to contribute some effort towards fixing this, and the holder_conversion proposal above sounds quite reasonable - I think it would have to be a runtime-only operation, since code returning a *_ptr<T> doesn't know anything about the py::class_<T...> instantiation that was used. But it sounds like that would also have to involve adding some new weight to possibly performance-critical places, and I'd like to have some agreement that a change in that direction would not be dead-on-arrival before diving in. It'd also be good to know whether to try to build on top of this PR or not.

@rhaschke
Copy link
Contributor

I came across the same issue and suggested another fix in #2052. However, the one here is more elegant as it detects the conflicting holder types during function/class definition. I think this PR should be merged to avoid potential segfaults and establish a warning mechanism to ensure consistent holder types.
For your convenience, I already rebased this PR to master: https://github.com/rhaschke/pybind11/tree/holder-type-checks.

@EricCousineau-TRI's original aim, on the other hand, was to automatically convert between different holder types.
As pointed out in #1161 (comment), it's a common pattern to use unique_ptr were possible (e.g. returning from factory functions) and switch to shared_ptr only when needed. It would be great if a pattern as suggested in #1161 (comment) could be realized in a future PR.

@rhaschke
Copy link
Contributor

Hm. As pointed out in #1161 (comment), this PR doesn't handle mismatching function arguments (only return types).
I added another (failing) test in my copy of this PR, which indicates that this PR should be augmented by corresponding checks for function arguments as well.

@@ -68,6 +68,7 @@ struct internals {
type_map<type_info *> registered_types_cpp; // std::type_index -> pybind11's type information
std::unordered_map<PyTypeObject *, std::vector<type_info *>> registered_types_py; // PyTypeObject* -> base type_info(s)
std::unordered_multimap<const void *, instance*> registered_instances; // void * -> instance*
type_map<const std::type_info *> holders_seen; // type -> seen holder type (to detect holder conflicts)
Copy link
Contributor

Choose a reason for hiding this comment

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

I was puzzled why you use a global map holders_seen here, instead of storing the unique holder type of a registered type in its typeinfo as I did in #2052. However, your approach just considers the first occurrence of a holder type as the gold standard. 👍

@YannickJadoul
Copy link
Collaborator

Adding this to the 2.7 milestone. Not per se to have this included as-is, but to make sure we discuss this old, but probably still relevant PR.

@YannickJadoul YannickJadoul added this to the v2.7 milestone Oct 20, 2020
@YannickJadoul YannickJadoul added the holders Issues and PRs regarding holders label Dec 29, 2020
@henryiii henryiii modified the milestones: v2.7, v2.8 Jul 16, 2021
@henryiii henryiii modified the milestones: v2.8, v2.9 Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
holders Issues and PRs regarding holders
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pybind11 wrapped method segfaults if a function returns unique_ptr<T> when holder is shared_ptr<T>
8 participants