Repository: python-attrs/attrs

Problem statement:
Current default implementation of __getstate__ and __setstate__ could be made safer
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):

```python
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](https://github.com/python-attrs/attrs/blob/c58ffd4e4cba5d5e58356722b985fc362358c48e/src/attr/_make.py#L925) would help.


Hints:
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.
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 :)
I don't think our implementation of pickling is a public API. So if you can make it compatible, that should  be OK.

Patch:
diff --git a/src/attr/_make.py b/src/attr/_make.py
--- a/src/attr/_make.py
+++ b/src/attr/_make.py
@@ -922,7 +922,7 @@ def slots_getstate(self):
             """
             Automatically created by attrs.
             """
-            return tuple(getattr(self, name) for name in state_attr_names)
+            return {name: getattr(self, name) for name in state_attr_names}
 
         hash_caching_enabled = self._cache_hash
 
@@ -931,8 +931,9 @@ def slots_setstate(self, state):
             Automatically created by attrs.
             """
             __bound_setattr = _obj_setattr.__get__(self)
-            for name, value in zip(state_attr_names, state):
-                __bound_setattr(name, value)
+            for name in state_attr_names:
+                if name in state:
+                    __bound_setattr(name, state[name])
 
             # The hash code cache is not included when the object is
             # serialized, but it still needs to be initialized to None to


Test patch:
diff --git a/tests/test_slots.py b/tests/test_slots.py
--- a/tests/test_slots.py
+++ b/tests/test_slots.py
@@ -9,6 +9,8 @@
 import types
 import weakref
 
+from unittest import mock
+
 import pytest
 
 import attr
@@ -743,3 +745,58 @@ def f(self):
 
     assert B(11).f == 121
     assert B(17).f == 289
+
+
+@attr.s(slots=True)
+class A:
+    x = attr.ib()
+    b = attr.ib()
+    c = attr.ib()
+
+
+@pytest.mark.parametrize("cls", [A])
+def test_slots_unpickle_after_attr_removed(cls):
+    """
+    We don't assign attributes we don't have anymore if the class has
+    removed it.
+    """
+    a = cls(1, 2, 3)
+    a_pickled = pickle.dumps(a)
+    a_unpickled = pickle.loads(a_pickled)
+    assert a_unpickled == a
+
+    @attr.s(slots=True)
+    class NEW_A:
+        x = attr.ib()
+        c = attr.ib()
+
+    with mock.patch(f"{__name__}.A", NEW_A):
+        new_a = pickle.loads(a_pickled)
+        assert new_a.x == 1
+        assert new_a.c == 3
+        assert not hasattr(new_a, "b")
+
+
+@pytest.mark.parametrize("cls", [A])
+def test_slots_unpickle_after_attr_added(cls):
+    """
+    We don't assign attribute we haven't had before if the class has one added.
+    """
+    a = cls(1, 2, 3)
+    a_pickled = pickle.dumps(a)
+    a_unpickled = pickle.loads(a_pickled)
+    assert a_unpickled == a
+
+    @attr.s(slots=True)
+    class NEW_A:
+        x = attr.ib()
+        b = attr.ib()
+        d = attr.ib()
+        c = attr.ib()
+
+    with mock.patch(f"{__name__}.A", NEW_A):
+        new_a = pickle.loads(a_pickled)
+        assert new_a.x == 1
+        assert new_a.b == 2
+        assert new_a.c == 3
+        assert not hasattr(new_a, "d")