Binding different types with the same name leads to segfaults

[wolfv]

I accidentally used the same name twice which lead to hard-to-debug segfaults that only happened when the class got imported twice or some other python module was loaded.

Therefore I think it might be nice to do a compile-time or runtime check if the name was already registered.

I added this little function

for (auto other_info : internals.registered_types_py) {
     std::string other = ((const detail::type_info *) (other_info.second))->type->tp_name;
     if (strcmp(other.c_str(), full_name.c_str()) == 0) {
          pybind11_fail(full_name + " registered previously!");
      }
 }

to the initialize (type_record *rec) code to do a check on import if there are duplicates already.

[wjakob]

Not sure what the issue is when importing the same type twice (ans I'm not sure I want to make it illegal -- suppose the same type is aliased multiple times in different namespaces; stuff like that is pretty common). Let's rather fix the piece of code that causes the crash/memory corruption.

[wolfv]

I understand. I investigated a bit further and it was not only specifying the name twice, but related to having a specific wrapper type. When another function returned a wrapper-object, the call to cast caused the segfault.

Ie. after fixing the following it worked:

py::class<Halfedge, Halfedge_handle>(m, "Halfedge");
py::class<Vertex, Vertex_handle>(m, "Halfedge");

If I remember correctly, the segfault occured in line 169 of cast.h (wrapper->value = src;)

But I guess it was a very specific problem to my use case so it might not be justified to add something like this. The weird thing was that it worked when I only imported my python library, and it didn't work when I imported some other library (e.g. random).

[wjakob]

What did those two lines look like when it failed? I still don't have enough context to understand what the problem here really is.

My resources for supporting this project are fairly limited -- to do anything, I'll need a self-contained minimal example that reproduces the issue.

[wolfv]

I have tried to make it as minimal as possible (i also added it as ZIP in an attachment).

I wouldn't say it's a real bug as the mistake was entirely on my part. However, it's quite hard to debug as it was not really obvious to me what happened (and I was creating my own handle-type so I wasn't sure about memory stuff anyways).

I think if the entire namespace is compared, there should be no overlap in type names (e.g. example.MyObject1 should only exist once). But maybe I am wrong about that.

Best,

Wolf minimal.zip

#!/usr/bin/env python
from __future__ import print_function
import sys
sys.path.append('.')

# from example import MyObject2
from example import MyObject3

from example import make_myobject2_1
from example import make_myobject2_2
from example import make_myobject3_2

import random 

for o in [make_myobject2_1(), make_myobject2_2(), make_myobject3_2()]:
    print(o)

and cpp

/*
    example/example8.cpp -- binding classes with custom reference counting,
    implicit conversions between types

    Copyright (c) 2015 Wenzel Jakob <wenzel@inf.ethz.ch>

    All rights reserved. Use of this source code is governed by a
    BSD-style license that can be found in the LICENSE file.
*/

#include "example.h"
#include "object.h"

/// Object managed by a std::shared_ptr<>
class MyObject2 {
public:
    MyObject2(int value) : value(value) {
        std::cout << toString() << " constructor" << std::endl;
    }

    std::string toString() const {
        return "MyObject2[" + std::to_string(value) + "]";
    }

    virtual ~MyObject2() {
        std::cout << toString() << " destructor" << std::endl;
    }

private:
    int value;
};

/// Object managed by a std::shared_ptr<>, additionally derives from std::enable_shared_from_this<>
class MyObject3 : public std::enable_shared_from_this<MyObject3> {
public:
    MyObject3(int value) : value(value) {
        std::cout << toString() << " constructor" << std::endl;
    }

    std::string toString() const {
        return "MyObject3[" + std::to_string(value) + "]";
    }

    virtual ~MyObject3() {
        std::cout << toString() << " destructor" << std::endl;
    }

private:
    int value;
};

PYBIND11_DECLARE_HOLDER_TYPE(T, std::shared_ptr<T>);

MyObject2 *make_myobject2_1() { return new MyObject2(6); }
std::shared_ptr<MyObject2> make_myobject2_2() { return std::make_shared<MyObject2>(7); }

MyObject3 *make_myobject3_1() { return new MyObject3(8); }
std::shared_ptr<MyObject3> make_myobject3_2() { return std::make_shared<MyObject3>(9); }

void init_ex8(py::module &m) {
    // here is the mistake
    py::class_<MyObject2, std::shared_ptr<MyObject2>>(m, "MyObject3")
        .def(py::init<int>());
    m.def("make_myobject2_1", &make_myobject2_1);
    m.def("make_myobject2_2", &make_myobject2_2);

    py::class_<MyObject3, std::shared_ptr<MyObject3>>(m, "MyObject3")
        .def(py::init<int>());
    m.def("make_myobject3_1", &make_myobject3_1);
    m.def("make_myobject3_2", &make_myobject3_2);

}

[wolfv]

And to add the GDB backtrace:

#0  0x00007ffff619ebde in pybind11::detail::type_caster_generic::cast (_src=0xabb920, policy=pybind11::automatic, parent=..., type_info=0x7ffff648df18 <typeinfo for MyObject2>, 
    copy_constructor=0x7ffff62046a0 <pybind11::detail::type_caster<MyObject2, void>::copy_constructor<MyObject2, 0>(void const*)>, existing_holder=0x0) at /home/wolfv/Programs/pybind11/include/pybind11/cast.h:169
169         wrapper->value = src;

where wrapper is unfortunately pointing to null.

[wjakob]

You aren't allowed to do this (see the docs):

MyObject2 *make_myobject2_1() { return new MyObject2(6); }

When using shared_ptr as a holder type, you can only pass values of type std::shared_ptr<MyObject2>. If you absolutely need to pass raw pointers, then the class needs to inherit from std::enable_shared_from_this, which you did not do in your example (for MyObject2 at least).

[wjakob]

So here the issue is that you have two types that are defined in an incompatible way, plus you are also not following the conventions for passing values. (either of these is enough to lead to segfaults, and there is little that pybind11 can do in such a case..). I am closing the issue.

[wolfv]

Hey,

hmm, I was fairly sure I copied the stuff almost one-to-one from your example8.cpp. MyObject_2 is defined exactly the same in the example if I am not mistaken, and the function returning a raw pointer, too.

The issue in my code was that the name "MyObject_3" was given twice and that is what caused the SEGFAULT for me, otherwise it works fine.

Anyways, I was considering this more to be an enhancement proposal (one might want to emit an error or pybind fail if a nullptr is discovered in the cast with some error information).

If you want a contribution in this direction I can try to come up with a pull request.

But I can also understand your position on this. Great library,

Wolf

[wolfv]

Link to example returning raw pointer: https://github.com/pybind/pybind11/blob/master/example/example8.cpp#L82

[wjakob]

Ah, good point :) -- that example is indeed returning a raw pointer. It will work in this specific case, since the returned value is not referenced from anywhere else.

I'm curious -- does your example still crash when MyObject2 inherits from std::enable_shared_from_this<MyObject2> ?

[wolfv]

No, it's also segfaulting.

It works perfectly fine when using two different names, though (with or without enable_shared_from_this :)

Cheers,

Wolf

[wjakob]

What happens if you completely remove MyObject2 and bind MyObject3 to the same name twice?

[wolfv]

Works fine!

    py::class_<MyObject3, std::shared_ptr<MyObject3>>(m, "MyObject3")
        .def(py::init<int>());
    m.def("make_myobject3_1", &make_myobject3_1);
    m.def("make_myobject3_2", &make_myobject3_2);

    py::class_<MyObject3, std::shared_ptr<MyObject3>>(m, "MyObject3")
        .def(py::init<int>());
    m.def("make_myobject3_1", &make_myobject3_1);
    m.def("make_myobject3_2", &make_myobject3_2);

That's the cpp code i used

and python

from example import MyObject3
from example import make_myobject3_2
from example import make_myobject3_1

for o in [ make_myobject3_2(), make_myobject3_1()]:
    print(o)
print(MyObject3(123))

[wjakob]

Ok -- I guess the real issue then is that two different types were bound to the same name (which is a rather strange use case). The sort of mistake that I suppose can happens is what you just did -- that the same type is registered multiple times, but that should always work as expected.

Given that, I don't see issue with the current handling of all of this in pybind11.

[wolfv]

Me neither, the way pybind handles it is perfectly well.

But it took me a long time to find that error (even though obvious in hindsight), as I also had to use a different kind of smart pointer class I was always suspecting an issue in my smart pointer wrapper code. IMO it could be a nice addition to pybind to check for names bound to different types and give the developer a hint (as when calling a function with wrong arguments etc.) but it's not a problem for me anymore :)

Cheers, Wolf

[abergmeier]

As mentioned by @wolfv this really is hard to debug once your binding code gets huge. So I would at least like to have this case fail the module loading if a type is registered in a module twice (there is really no reason to do this IMO). Added a test cast in https://github.com/pybind/pybind11/pull/218

[wjakob]

ok, good point -- I added a check.

[abergmeier]

Great. Should e.g. adding a property twice not result in the same problem? It works for me but seems fishy!?

[wjakob]

It will replace the previous property in that case. It's similar for functions -- you could define the same overload ten times in a row, and only the first one matters. Pybind11 will never isolate the user from all sorts of foolishness, and it's expensive (in terms of generated code) to check for all of this stuff.