Current default implementation of __getstate__ and __setstate__ could be made safer

[djipko]

This is a known "sharp edge" of pickle, but attrs could make this a bit safer by slightly modifying the default implementation of __getstate__ and __setstate__. The problem is that due to returning a tuple in the default implementation of __getstate__, removing a member, and then unpickling from a previous version can be very unsafe. This is not a hypothetical situation - it commonly happens when an object is pickled, and then stored in a data-store of some sort, and unpickled some time later by a changed version of the code.

Here's a simple reproducer that demonstrates it (python 3.10, attrs 22.1.0):

import pickle

import attr

@attr.s(slots=True, hash=False, auto_attribs=True)
class Test:
    count: int
    enable_minor_feature: bool
    should_launch_missiles: bool

t = Test(count=1, should_launch_missiles=False, enable_minor_feature=True)
print(t)  # Test(count=1, enable_minor_feature=True, should_launch_missiles=False)
tp = pickle.dumps(t)

@attr.s(slots=True, hash=False, auto_attribs=True)
class Test:
    count: int
    should_launch_missiles: bool

tl = pickle.loads(tp)
print(tl)  # Test(count=1, should_launch_missiles=True)  <== an attribute is assigned a dangerously wrong value

While there's certainly an argument to be made that pickle should not be used for such things (and I'd agree) - the default implementation could, I believe, be made safer. Raise if things don't match, or even ignore unknown attributes in setstate, by potentially returning a dictionary instead of a tuple here would help.

[hynek]

Have you tried replacing the tuple by the dict and see how much it breaks? Performance difference should be benign, since we save ourselves zipping.

[djipko]

I wanted to get some thoughts on the feasibility of making the change at all first. Let me put together a patch and see how much it breaks, and we can take it from there. Thanks :)

[hynek]

I don't think our implementation of pickling is a public API. So if you can make it compatible, that should be OK.

[djipko]

I think this should now have the right coverage, and all tests seem to pass so \o/

Let me know what you think :)

Thanks!