Restrict default pickleability

[serhiy-storchaka]

BPO	22995
Nosy	@warsaw, @pitrou, @scoder, @avassalotti, @serhiy-storchaka, @MojoVampire
Files	pickle_restrictions.diff pickle_restrictions_2.patch pickle_restrictions_3.patch pickle_restrictions_4.patch pickle_restrictions_5.patch pickle_restrictions_warning-3.5.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2016-01-12.14:25:27.091>
created_at = <Date 2014-12-04.13:16:43.440>
labels = ['interpreter-core', 'type-bug']
title = 'Restrict default pickleability'
updated_at = <Date 2016-01-16.12:44:45.844>
user = 'https://github.com/serhiy-storchaka'

bugs.python.org fields:

activity = <Date 2016-01-16.12:44:45.844>
actor = 'scoder'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2016-01-12.14:25:27.091>
closer = 'barry'
components = ['Interpreter Core']
creation = <Date 2014-12-04.13:16:43.440>
creator = 'serhiy.storchaka'
dependencies = []
files = ['37494', '37739', '40373', '41024', '41363', '41582']
hgrepos = []
issue_num = 22995
keywords = ['patch']
message_count = 40.0
messages = ['232135', '232160', '232887', '234125', '234131', '234175', '242962', '249901', '251723', '254525', '254526', '254528', '254562', '256733', '257006', '257234', '257235', '257646', '257650', '257667', '257686', '257706', '257719', '257720', '257724', '257725', '257979', '257993', '257998', '257999', '258000', '258002', '258003', '258004', '258099', '258101', '258105', '258377', '258385', '258386']
nosy_count = 10.0
nosy_names = ['barry', 'pitrou', 'scoder', 'alexandre.vassalotti', 'nadeem.vawda', 'Arfrever', 'kitterma', 'python-dev', 'serhiy.storchaka', 'josh.r']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue22995'
versions = ['Python 2.7', 'Python 3.5']

[serhiy-storchaka]

When no efforts were spent to support pickling of instances of Python class, in most cases the class will be pickleable. Default implementation just saves all attributes and this works if all attributes are pickleable. Exceptional special cases:

1. Classes with __slot__. In this case an attempt to pickle will raise an exception with explaining message.

2. Classes with __new__ wish mandatory parameters. In this case the pickling will be silently "successful", but actually data will be not saved and unpickling will fail. See for example bpo-21975, bpo-22955.

But when the class is implemented in C and doesn't expose its attribute in __dict__, in most cases it is variant (2) -- silent incorrect pickling.

I think we should prevent incorrect pickling by default. It is better to fail loudly. Default reduce could inspect the __new__ method and raise an exception if it has non-optional parameters and the class doesn't implement __getnewargs_ex__ or __getnewargs__ methods. And fail on all classes implemented in C if they don't implement any pickle-related methods.

[MojoVampire]

Minor note: Python classes with __slots__ pickle and unpickle just fine under protocol 2 and higher. Only protocols 0 and 1 have this problem, and they are no longer used by default on Py3, and not necessary to communicate with Py2, where protocol 2 is supported and generally more efficient.

I understand that you're not suggesting any changes to address issue #1, just wanted to mention this.

[serhiy-storchaka]

Here is sample patch which adds two restrictions. Default reduce method for protocol >= 2 will forbid pickling objects:

1. When tp_new == NULL.

2. Builtins without any of pickle-related methods: __getnewargs_ex__, __getnewargs__ or __getstate__.

Are there any other ideas?

[pitrou]

How many cases does the patch catch?

[serhiy-storchaka]

All public classes not designed for pickling explicitly. I tested only operator.methodcaller, mmap.mmap, sqlite3 classes (Connect, Cursor, Row), _socket.socket, select.epoll, _csv.Dialect, but should be more. Instances of these classes can be "pickled", but unpickling either raise an exception (usually TypeError), or returns default or underinitialized object.

For other classes the patch changes raised exception type. E.g. pickling zlib.compressobj() raised

_pickle.PicklingError: Can't pickle <class 'zlib.Compress'>: attribute lookup Compress on zlib failed

and with the patch it raises

TypeError: can't pickle zlib.Compress objects

[serhiy-storchaka]

It is hard to write good tests, because every example is temporary, after adding support of pickling it will be not valid.

Here is extended patch. It now handles Python subclasses of builtin classes (except list and dict which are pickleable via iterators) such as socket.socket. Removed several guarding __getstate__() methods. Added few new tests.

[serhiy-storchaka]

Ping.

[serhiy-storchaka]

See also bpo-14350 and bpo-24900. Both will be solved by disabling pickling when tp_new is NULL (a part of proposed patch). So that I think the patch should be applied to all releases.

[serhiy-storchaka]

Could anyone please make a review?

[python-dev]

New changeset c8841db9433d by Serhiy Storchaka in branch '3.4':
Issue bpo-22995: Default implementation of __reduce__ and __reduce_ex__ now
https://hg.python.org/cpython/rev/c8841db9433d

New changeset 4c05e7c195ac by Serhiy Storchaka in branch '3.5':
Issue bpo-22995: Default implementation of __reduce__ and __reduce_ex__ now
https://hg.python.org/cpython/rev/4c05e7c195ac

New changeset a2f574896f49 by Serhiy Storchaka in branch 'default':
Issue bpo-22995: Default implementation of __reduce__ and __reduce_ex__ now
https://hg.python.org/cpython/rev/a2f574896f49