Infinite Recursion during Unpickling a codecs Object

[ThomasH]

BPO	6395
Nosy	@malemburg, @avassalotti, @serhiy-storchaka
Files	codecs_stream_delegating.patch codecs_stream_delegating_2.patch codecs_stream_forbid_pickling.patch: Forbid pickling and copying

^{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 = None
created_at = <Date 2009-07-01.15:46:26.758>
labels = ['type-bug', 'library']
title = 'Infinite Recursion during Unpickling a codecs Object'
updated_at = <Date 2015-12-06.12:10:19.822>
user = 'https://bugs.python.org/ThomasH'

bugs.python.org fields:

activity = <Date 2015-12-06.12:10:19.822>
actor = 'serhiy.storchaka'
assignee = 'serhiy.storchaka'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2009-07-01.15:46:26.758>
creator = 'ThomasH'
dependencies = []
files = ['41184', '41215', '41217']
hgrepos = []
issue_num = 6395
keywords = ['patch']
message_count = 10.0
messages = ['89985', '90180', '90258', '255540', '255756', '255761', '255763', '255774', '255785', '256011']
nosy_count = 4.0
nosy_names = ['lemburg', 'alexandre.vassalotti', 'ThomasH', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue6395'
versions = ['Python 2.7', 'Python 3.4', 'Python 3.5', 'Python 3.6']

Linked PRs

• gh-50644: Forbid pickling of codecs streams #109180
• [3.12] gh-50644: Forbid pickling of codecs streams (GH-109180) #109231
• [3.11] gh-50644: Forbid pickling of codecs streams (GH-109180) #109232

[ThomasH]

I recently ran into an infinite recursion trying to unpickle a
codecs.StreamWriter object (I presume the issue would be the same for a
StreamReader).

Here is the end of the stack trace:

  File "/sw/lib/python2.5/codecs.py", line 330, in __getattr__   
    return getattr(self.stream, name)
  File "/sw/lib/python2.5/codecs.py", line 330, in __getattr__ 
    return getattr(self.stream, name) 
  File "/sw/lib/python2.5/codecs.py", line 330, in __getattr__ 
    return getattr(self.stream, name) 
RuntimeError: maximum recursion depth exceeded

The issue is the same under Python2.6 but the error output has changed
(see http://bugs.python.org/issue5508).

The problem is that the codecs module tries to delegate member lookup to
the underlying stream. But after unpickling, "self.stream" is not
defined, so the reference to self.stream in __getattr__ itself leads to
an invocation of __getattr__ - hence the recursion loop.

Using tools from the Pickle protocol, like __getstate__/setstate,
could help degrade codecs objects gracefully during pickle/unpickle
cycles. E.g. it might be enough to provide a dummy self.stream through
__setstate__.

[malemburg]

I don't understand the use case for this.

If the StreamWriter/Reader cannot pickle the underlying stream (which is
probably always the case), why should the object itself be pickleable ?

What we could do is implement .__get/setstate__() and have it raise an
exception to inform the user of the problem.

[ThomasH]

Sounds good to me. The main intention of this bug is not to make codecs
objects pickleable by all means (I don't know if this would be sensible
and/or possible), but at least to have them degrade gracefully during
pickling, particularly without infinite recursion. I've changed the bug
title to reflect this.

[serhiy-storchaka]

Implementing only .__get/setstate__() doesn't fix all problem. We have implement also __getnewargs_ex__(). But implemented __getnewargs_ex__() has priority over __getnewargs__() implemented in subclasses.

And may be there are problems with other optional special methods that are incorrectly delegated to the stream in codecs IO classes.

I think more reliable way is to disallow delegating all special (or may be even private) methods. Here is a patch.

[serhiy-storchaka]

If the StreamWriter/Reader cannot pickle the underlying stream (which is
probably always the case), why should the object itself be pickleable ?

io.BytesIO() and io.StringIO() are pickleable.

[malemburg]

On 02.12.2015 20:16, Serhiy Storchaka wrote:

Serhiy Storchaka added the comment:

> If the StreamWriter/Reader cannot pickle the underlying stream (which is
> probably always the case), why should the object itself be pickleable ?

io.BytesIO() and io.StringIO() are pickleable.

Ok, but I still don't see the use case :-)

I think all we need to do is add a .__reduce__()
method to StreamWriter and StreamReader, which then
raises a PickleError.

Example:

>>> import sys, codecs, pickle
>>> r = codecs.getreader('latin-1')
>>> class MyReader(r):
...    def __reduce__(self, *args):
...       raise pickle.PickleError
...
>>> s = MyReader(sys.stdin)
>>> pickle.dumps(s)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 3, in __reduce__
_pickle.PickleError
> <stdin>(3)__reduce__()

[serhiy-storchaka]

Added tests for pickling and deepcopying.

[serhiy-storchaka]

I think all we need to do is add a .__reduce__()
method to StreamWriter and StreamReader, which then
raises a PickleError.

Rather TypeError. Yes, it is the least that we should to do in maintained releases. If codecs_stream_delegating_2.patch is considered too drastic for bugfix. But this can be only a part of problem. May be there are issues with other optional special methods. And adding __reduce_ex__ breaks subclass pickleability if it was implemented with __getstate__ and __getnewargs__.

Here is a patch for this way.

[malemburg]

On 02.12.2015 21:29, Serhiy Storchaka wrote:

Serhiy Storchaka added the comment:

> I think all we need to do is add a .__reduce__()
> method to StreamWriter and StreamReader, which then
> raises a PickleError.

Rather TypeError. Yes, it is the least that we should to do in maintained releases. If codecs_stream_delegating_2.patch is considered too drastic for bugfix. But this can be only a part of problem. May be there are issues with other optional special methods. And adding __reduce_ex__ breaks subclass pickleability if it was implemented with __getstate__ and __getnewargs__.

Here is a patch for this way.

Thanks. I think using __reduce__ instead of __reduce_ex__ is
safer, since subclasses will more likely override __reduce__
than __reduce_ex__.

The other approach will have too many backwards incompatible side
effects, e.g. the repr() over StreamReader instances would change.

[serhiy-storchaka]

There is also a problem with deepcopying:

>>> class MemReader:
...     def __init__(self, data):
...         self.buf = memoryview(data).cast('B')
...     def read(self, size=-1):
...         if size < 0:
...             size = len(self.buf)
...         res = bytes(self.buf[:size])
...         self.buf = self.buf[size:]
...         return res
...     def close():
...         pass
...     def __deepcopy__(self, memo):
...         return MemReader(self.buf)
...     def __reduce__(self):
...         return MemReader, (bytes(self.buf),)
... 
>>> import codecs, copy
>>> s1 = codecs.getreader('utf-8')(MemReader(b'abcd'))
>>> s2 = copy.deepcopy(s1)
>>> s1.read()
'abcd'
>>> s2.read()
b'abcd'
>>> s2
<__main__.MemReader object at 0xb701988c>