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

[BUG] pickleable base C++ classes can incur object slicing if derived typeid is not registered with pybind11 #3062

Open
EricCousineau-TRI opened this issue Jun 25, 2021 · 3 comments
Labels

Comments

@EricCousineau-TRI
Copy link
Collaborator

EricCousineau-TRI commented Jun 25, 2021

Issue description

Context: #2972 (comment)

If you have C++ base class that is exposed to pybind11, but then you have a derived class that is not exposed to pybind11, then you will encounter object slicing (wikipedia) via a pickle -> unpickle round trip.
Slicing may mean that the unpickled object has the wrong vtable, data, etc.

Reproducible example code

master...EricCousineau-TRI:issue-3062 (derived from fixes in #2972)
commit: 7ce1ad0

Upon running the test:

$ make pytest
...
    def test_roundtrip_simple_cpp_derived():
        p = m.make_SimpleCppDerivedAsBase()
        p.num = 404
        assert m.get_value_via_vtable_cpp(p) == 10
...
        data = pickle.dumps(p, pickle.HIGHEST_PROTOCOL)
        p2 = pickle.loads(data)
        assert isinstance(p2, m.SimpleBase)
        assert p2.num == 404
>       assert m.get_value_via_vtable_cpp(p2) == 10
E       assert 1 == 10  # Egad! Error!

\cc @rwgk @YannickJadoul @elkhrt

@EricCousineau-TRI
Copy link
Collaborator Author

EricCousineau-TRI commented Jun 25, 2021

Suggested (but invasive) solution:

Fail fast if we attempt to pickle an object whose typeid is not registered.
EDIT: Ah, but ensure that we add an option for users to allow it if they have some virtual methods of their own to add in pickle-based serialization+deserialization.

Less invasive solution:

Document this sharp edge, and tell people to be careful / provide an example of how to do this properly.

rwgk pushed a commit to rwgk/pybind11 that referenced this issue Jun 26, 2021
@rwgk
Copy link
Collaborator

rwgk commented Jun 26, 2021

Oh, I saw your get_value_via_vtable_cpp addition here only after I added the dynamic_cast test under #2972, but it looks like the same basic idea.

In a way the __dict__ issue @elkhrt ran into is a warning flag that we ignored. Edward, what's your take on the "implicit upcasting" going through a pickling/unpickling cycle?

How could we detect the situation when pickling? (I'm currently drawing a blank.)

@elkhrt
Copy link
Contributor

elkhrt commented Jun 30, 2021

Agree this is an issue. It doesn't affect the open spiel use case because we effectively have our own type registration - the unpickling code will call a factory method, which might be native C++, or a Python callable, which ensures we get the right derived type. (This wasn't deliberate, but just a consequence of the general design).

rwgk pushed a commit that referenced this issue Jun 30, 2021
* pickle setstate: setattr __dict__ only if not empty, to not force use of py::dynamic_attr() unnecessarily.

* Adding unit test.

* Clang 3.6 & 3.7 compatibility.

* PyPy compatibility.

* Minor iwyu fix, additional comment.

* Addressing reviewer requests.

* Applying clang-tidy suggested fixes.

* Adding check_dynamic_cast_SimpleCppDerived, related to issue #3062.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants