-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
[BUG]: __new__
does not initialize STL containers and resulting in undefined behavior
#4549
Comments
@Skylion007 Thanks for the example. This works for me: PyTreeSpecTypeObject
.def_static(
"__new__",
[](const py::type& cls) {
throw py::type_error(cls.attr("__module__").cast<std::string>() + "." +
cls.attr("__name__").cast<std::string>() +
" cannot be instantiated");
},
py::arg("cls"))
.def(py::init<>()) // no-op Now it works as expected (raise a In [1]: !pip3 install -e .
...: from optree import PyTreeSpec
In [2]: PyTreeSpec
Out[2]: <class 'optree.PyTreeSpec'>
In [3]: PyTreeSpec.mro()
Out[3]: [<class 'optree.PyTreeSpec'>, <class 'pybind11_builtins.pybind11_object'>, <class 'object'>]
In [4]: PyTreeSpec() # an error raised as expected
TypeError: optree.PyTreeSpec cannot be instantiated
In [5]: PyTreeSpec.__new__(PyTreeSpec) # an error raised as expected
TypeError: optree.PyTreeSpec cannot be instantiated
In [6]: spec = tree_structure({'a': 1, 'b': (2, 3)})
In [7]: spec
Out[7]: PyTreeSpec({'a': *, 'b': (*, *)})
In [8]: spec.__new__(PyTreeSpec) # an error raised as expected
TypeError: optree.PyTreeSpec cannot be instantiated
In [9]: spec.__init__() # noop Minor bug found in the test case: Lines 87 to 90 in 442261d
Should be: -.def(py::init([](const NoConstructorNew &self) { return self; })) // Need a NOOP __init__
+.def(py::init<>()) // Need a NOOP __init__ otherwise, you will get: >>> print(NoConstructorNew.__init__.__doc__)
__init__(self: NoConstructorNew, arg0: NoConstructorNew) -> None |
Reopened for found another issue. If I raise In [1]: from optree import PyTreeSpec, tree_structure
In [2]: spec = tree_structure({'a': 1, 'b': (2, 3)})
In [3]: import pickle as pkl
In [4]: pkl.dumps(spec)
Out[4]: b'\x80\x04\x95w\x00\x00\x00\x00\x00\x00\x00\x8c\x06optree\x94\x8c\nPyTreeSpec\x94\x93\x94)\x81\x94((K\x01K\x00NNNK\x01K\x01t\x94(K\x01K\x00NNNK\x01K\x01t\x94(K\x01K\x00NNNK\x01K\x01t\x94(K\x03K\x02NNNK\x02K\x03t\x94(K\x05K\x02]\x94(\x8c\x01a\x94\x8c\x01b\x94eNNK\x03K\x05t\x94t\x94\x89\x8c\x00\x94\x87\x94b.'
In [5]: pkl.loads(pkl.dumps(spec))
TypeError: optree.PyTreeSpec cannot be instantiated Ref:
In the Pybind11 pickling example, a new instance is returned in Python inst = cls.__new__()
inst.__setstate__(state) The return value for |
@XuehaiPan That's an oddly specific usecase. you have full access to the object state and you could pass it in the lambda you are using for the pybind11/include/pybind11/detail/init.h Line 375 in 4768a6f
|
Finding
I investigate Pybind11's pybind11/include/pybind11/detail/init.h Lines 117 to 142 in 442261d
But I have no idea why it has no effect on the Python side PyTreeSpecTypeObject
.def_static(
"__new__",
[](const py::type& cls) {
return std::make_unique<PyTreeSpec>();
},
py::arg("cls"))
.def(py::init<>()) // no-op
.def(py::pickle([](const PyTreeSpec& t) { return t.ToPicklable(); },
// Return a new instance, `setstate` will update `value_and_holder.value_ptr()`
[](const py::object& o) { return PyTreeSpec::FromPicklable(o); }) obj.__setstate__(other.__getstate__())
obj # unchanged I found:
I tried manually calling auto PyTreeSpecTypeObject =
py::class_<PyTreeSpec>(mod, "PyTreeSpec", "Representing the structure of the pytree.");
auto PyTreeSpec_Type = *(reinterpret_cast<PyTypeObject*>(PyTreeSpecTypeObject.ptr()));
auto pybind11_object_new = PyTreeSpec_Type.tp_new;
auto new_func = [&pybind11_object_new](
PyTypeObject* type, PyObject* args, PyObject* kwargs) -> PyObject* {
auto* self = pybind11_object_new(type, args, kwargs);
auto* inst = reinterpret_cast<py::detail::instance*>(self);
auto v_h = inst->get_value_and_holder(py::detail::get_type_info(typeid(PyTreeSpec)));
v_h.type->init_instance(inst, nullptr);
return self;
};
PyTreeSpec_Type.tp_new = reinterpret_cast<decltype(pybind11_object_new)>(&new_func); Still:
Overall, I cannot let both "always initialize instance in |
@XuehaiPan would this PR solve most of your issue? If so, we can try to revive it and get something similar integrated into pybind11: #4116 |
In a #4621 comment @Skylion007 asked:
Not sure. At first glance "probably not", but I could be wrong. I'd need a significant block of time to really understand. We (@wangxf123456 and myself) hit on something related - I think - the other day. Ultimately I think it has to do with this: pybind11/include/pybind11/detail/smart_holder_type_casters.h Lines 658 to 668 in d930de0
That's smart_holder code. Pointing that out here because that is meant to clearly identify and cleanly handle the "not initialized" case. I'd have
Sorry, nothing concrete/actionable right now. |
I just finished minimizing another example of this issue while debugging what originally looked like something completely different. Once I got it minimized enough to convince me it was pybind11 or pytest I started searching issue trackers and found this issue which allowed me to minimize it even further: https://github.com/davisp/tiledb-pybind11-bug I may be able to get by with just throwing an exception from I'm no expert on Python internals, but I'm curious why nothing else caught that this is what was happening. Once I figured out what was going on, I immediately started to wonder why the allocated memory isn't zeroed out and then checked against nullptr before invoking methods on it. I assume that's just a performance trade off since most folks aren't doing separate Either way, I'll report back if throwing from |
Apparently I skimmed the discussion on |
Looking at this it crossed my mind: does #4762 make a difference for the original issue reported here? IIUC @davisp used the newest pybind11 master when debugging his issue. But if that not the same as this issue we have two questions, possibly with two root causes. What would really help: Reproducers in pybind11 PRs. |
@rwgk I only used what was newest on PyPI. However, that reproducer should be fairly easily reproducible as a standalone test. I can give that a whirl if you think its worth the time. However, I'm not entirely certain on what the expected behavior should be here. My current understanding is that this is similar to placement new in that we've allocated the space to hold the wrapped C++ object, then failed to successfully instantiate the object, which means if we attempt to apply a method on the un-instantiated random bit of memory we can easily segfault the interpreter. Which is to say, I can at least get things into the shape of a test, but I'm not entirely certain what the behavior is expected to be in order to know what specifically to assert and what not. |
I did not mean to suggest looking for (or constructing) a problem. Offering a couple more thoughts for completeness:
Is that acceptable? — I'm not sure. It doesn't sound ideal, but then again, there might be arguments against runtime overhead that comes with a guard. A related question: Is the behavior safer when using |
This is precisely the question I don't know how to answer. On the one hand it feels like its an odd edge case that most folks don't encounter so tacking on some non-zero overhead for everyone feels a bit heavy handed. And on the other hand, knowing there's a basic inheritance issue that can lead to segfaults seems not awesome either. If I find some time, I'll try that smart_holder branch and report back what I find. |
Fail into this again while testing pickling support with How
Now my A small snippet to demonstrate: class OwnsPythonObjects {
public:
std::vector<py::object> sequence;
};
{
py::class_<OwnsPythonObjects> cls(
mod, "OwnsPythonObjects", py::custom_type_setup([](PyHeapTypeObject *heap_type) {
auto *type = &heap_type->ht_type;
type->tp_flags |= Py_TPFLAGS_HAVE_GC;
type->tp_traverse = [](PyObject *self_base, visitproc visit, void *arg) {
if (PY_VERSION_HEX >= 0x03090000) [[likely]] { // Python 3.9
Py_VISIT(Py_TYPE(self_base));
}
auto &self = py::cast<OwnsPythonObjects &>(py::handle(self_base));
for (auto &item : self.sequence) { // <<< segmentation fault due to uninitialized vector
Py_VISIT(item.ptr());
}
return 0;
};
}));
cls.def(py::pickle([](const OwnsPythonObjects &t) { return py::cast<py::tuple>(t.sequence); },
[](const py::object &o) { raise py::value_error("malformed state"); }));
// immutable type does not need tp_clear
reinterpret_cast<PyTypeObject *>(cls.ptr())->tp_flags |= Py_TPFLAGS_IMMUTABLETYPE;
} |
I believe that's a different problem. To me it looks like the way the I think this alternative pickle mechanism will support your use case: https://github.com/google/pybind11k/pull/30094/files Note that the production code change is tiny: the delta is just -4 lines, +13 lines in pybind11.h. I think with that, you could just add a default constructor ( It should be really easy for you to try out, just patch pybind11.h locally. Please let me know if that works or not. |
@rwgk Unfortunately,
I resolve this by adding a guard check in my py::custom_type_setup([](PyHeapTypeObject *heap_type) {
auto *type = &heap_type->ht_type;
type->tp_flags |= Py_TPFLAGS_HAVE_GC;
type->tp_traverse = [](PyObject *self_base, visitproc visit, void *arg) {
if (PY_VERSION_HEX >= 0x03090000) [[likely]] { // Python 3.9
Py_VISIT(Py_TYPE(self_base));
}
+ auto* instance = reinterpret_cast<py::detail::instance*>(self_base);
+ if (!instance->get_value_and_holder().holder_constructed()) [[unlikely]] {
+ // The holder is not constructed yet. Skip the traversal to avoid segfault.
+ return 0;
+ }
auto &self = py::cast<OwnsPythonObjects &>(py::handle(self_base));
for (auto &item : self.sequence) {
Py_VISIT(item.ptr());
}
return 0;
};
}) But this relies on the implementation details for pybind11 objects. I think the ultimate solution is to initialize the allocated memory in As an opt-in feature, I think we can add a new option (e.g., |
Cool! Everything around it does anyway. Just be happy! |
Required prerequisites
What version (or hash if on master) of pybind11 are you using?
2.10.3
Problem description
I'm writing a C++ class with some STL-like containers as its members. Then I bind it with
pybind11
and expose it to Python. However, the bind C++ class cannot behave like a normal Python class as expected.All bind types created by
py::class_<CppClass>
are inherit frompybind11_builtins.pybind11_object
. As the comment says:pybind11/include/pybind11/detail/class.h
Lines 346 to 370 in 3cc7e42
pybind11_object_new
only allocates space for the C++ object, but doesn't call the constructor. That means if someone callsBoundCppClass.__new__(BoundCppClass)
, they will get undefined results since the C++ object is not initialized at all. Also, default values in the C++ class definition are not used even if there is a default constructor.Reproducible example code
git clone https://github.com/pybind/cmake_example.git cd cmake_example
src/main.cpp
:ipython
:Is this a regression? Put the last known working version here if it is.
Not a regression
The text was updated successfully, but these errors were encountered: