wjakob / nanobind

nanobind: tiny and efficient C++/Python bindings
BSD 3-Clause "New" or "Revised" License
2.29k stars 191 forks source link

Avoid unsafe cast in nb::ref<T> #603

Closed oremanj closed 4 months ago

oremanj commented 4 months ago

If you manage to form a nb::ref<T> where T does not inherit from nb::intrusive_base, then nb::ref will reinterpret the first two words of a T object as if they were an intrusive_base, due to the widespread use of C-style pointer casts in the implementation of nb::ref. This seems like an unnecessary hazard to me.

I'm not entirely sure what the purpose of the casts is. This PR assumes that the C-style cast is used to access an intrusive_base subobject even if it is inherited privately; it keeps the cast if std::is_base_of<intrusive_base, T> is true, and eschews it otherwise (in which case the user had better have provided their own inc_ref and dec_ref ADL functions). Alternatively, if the intention was that nb::ref<T> only works if T inherits nb::intrusive_base, we could add a static assert to that effect instead.

wjakob commented 4 months ago

These harsh C-style casts exist so that everything works with forward declarations. I am extensively using ref<> in a codebase where I want to be able to write:

/// Forward declaration
class A;

struct B {
    ref<A> a;
};

which is something that one can do with raw pointers. To turn those into reference-counted pointers, we can't assume to actually know anything about the type, or the full definition of A would need to be included, which contributes to header bloat.

oremanj commented 4 months ago

All right, I'll leave well enough alone then :-) Since there's nowhere that's required to have a complete T (even the delete can indirect through the virtual dtor in the base class), there's nowhere that one could validate the base relationship.