Some repr implementations don't check for self-referential structures

[abacabadabacaba]

BPO	25455
Nosy	@rhettinger, @pitrou, @scoder, @benjaminp, @skrah, @berkerpeksag, @serhiy-storchaka
PRs	bpo-25455: Fixed crashes in repr of recursive buffered file-like obje… #514 bpo-25455: Fixed crashes in repr of recursive buffered file-like obje… #722 bpo-25455: Fixed crashes in repr of recursive buffered file-like obje… #727
Files	partial_recursive_repr.patch io_recursive_repr.patch etree_recursive_repr.patch io_recursive_repr2.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 2017-03-20.07:57:32.622>
created_at = <Date 2015-10-21.19:17:18.579>
labels = ['extension-modules', '3.7', 'type-crash']
title = "Some repr implementations don't check for self-referential structures"
updated_at = <Date 2017-03-24.22:08:12.820>
user = 'https://bugs.python.org/abacabadabacaba'

bugs.python.org fields:

activity = <Date 2017-03-24.22:08:12.820>
actor = 'serhiy.storchaka'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2017-03-20.07:57:32.622>
closer = 'serhiy.storchaka'
components = ['Extension Modules']
creation = <Date 2015-10-21.19:17:18.579>
creator = 'abacabadabacaba'
dependencies = []
files = ['40932', '40971', '40972', '43360']
hgrepos = []
issue_num = 25455
keywords = ['patch']
message_count = 26.0
messages = ['253309', '253872', '253965', '253969', '253972', '253973', '254267', '254268', '255095', '255110', '255174', '255182', '267784', '268328', '268335', '268349', '268365', '268367', '268369', '268371', '268372', '272730', '282295', '290137', '290154', '290161']
nosy_count = 10.0
nosy_names = ['rhettinger', 'pitrou', 'scoder', 'benjamin.peterson', 'stutzbach', 'skrah', 'abacabadabacaba', 'python-dev', 'berker.peksag', 'serhiy.storchaka']
pr_nums = ['514', '722', '727']
priority = 'high'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue25455'
versions = ['Python 2.7', 'Python 3.5', 'Python 3.6', 'Python 3.7']

[abacabadabacaba]

Implementations of repr for some of the types in the standard library doesn't check for self-referential structures. As a result, when calling repr() on such objects, Python crashes due to infinite recursion.

Example:
>>> import functools
>>> x = functools.partial(min)
>>> x.__setstate__((x, (), {}, {}))
>>> repr(x)
Segmentation fault

Another example:
>>> import xml.etree.ElementTree
>>> x = xml.etree.ElementTree.Element('')
>>> x.tag = x
>>> repr(x)
Segmentation fault

One more example:
>>> import io
>>> class X(io.TextIOWrapper): __slots__ = 'name'
>>> x = X(open('/dev/null'))
>>> x.name = x
>>> repr(x)
Segmentation fault

[serhiy-storchaka]

The general solution is to make PyObject_Repr to detect recursive calls (as reprlib.recursive_repr does).

The straightforward way is to use thread local identity set. It can be implemented as a dict that maps id(obj) -> obj (creates an int object for key for every call, requires about 40-80 bytes for recurse level), or as specialized hash table (see Modules/hashtable.c) (faster, requires about 12-24 bytes for recurse level).

The fastest solution would be to set special flag inside proceeded object. For now general object has no place for such flag, but we can add it to GC head. On 64-bit this would increase the size of GC head from 24 to 32 bytes, on 32-bit there is a free place in 16-bytes GC head.

However the performance can be not critical here, because in any case repr() creates new object (resulting string). Using thread local hash table can be enough. In any case the patch will be enough complex to target it 3.6 only.

[rhettinger]

Changing PyObject_Repr is too course; it affects a broad class of objects other than containers, and it risks unknown impacts to larges swaths of third-party code use this venerable API. It is also a break with the long established history of recursion detection being a responsibility of the individual types (i.e. the code in sets, lists, dicts, etc.)

The three cases listed here should be fixed individually.

[serhiy-storchaka]

Yet one example:

>>> import io
>>> class BR(io.BufferedReader):
...     @property
...     def name(self):
...         return self
... 
>>> repr(BR(io.BytesIO()))
Segmentation fault

The same is for other file-like objects.

[serhiy-storchaka]

Recursive partial objects are legitimate. Here is a patch that makes partial's repr to support recursive partial objects. Also added a test for pickling.

Cases for Element and file-like objects are questionable. Recursive Element.tag and TextIOWrapper.name don't make a sense, and I don't think we should special support (and encourage) these cases. To avoid stack overflow we can add a restriction for tag to be str or None, but file's name attribute can be dynamic. We can omit name from repr if it is not None, str, bytes or int.

[serhiy-storchaka]

Added ElementTree and io modules experts to the nosy list.

[serhiy-storchaka]

Here are patches for io classes and for ElementTree.

[serhiy-storchaka]

There is also a crash with Python implementation of TextIOWrapper.

>>> import _pyio as io
>>> t = io.TextIOWrapper(io.BytesIO())
>>> t.mode = t
>>> t
Fatal Python error: Cannot recover from stack overflow.

Current thread 0xb74a9700 (most recent call first):
File "/home/serhiy/py/cpython/Lib/pyio.py", line 1982 in __repr_
File "/home/serhiy/py/cpython/Lib/pyio.py", line 1992 in __repr_
[...]
File "/home/serhiy/py/cpython/Lib/pyio.py", line 1992 in __repr_
File "/home/serhiy/py/cpython/Lib/pyio.py", line 1992 in __repr_
...
Aborted (core dumped)

[serhiy-storchaka]

Raymond, could you please make a review of the first patch?

[rhettinger]

Raymond, could you please make a review of the first patch?
Will do.

Also, we should ask Antoine Pitrou to look at the TextIO patch and ask Stephan Krah to look at the ElementTree patch.

[skrah]

I think you may have meant Eli Bendersky -- I'm not an elementree
expert (Eli, I'm adding you back just to clear this up).

[elibendersky]

As I've mentioned elsewhere, I'll have to temporarily take myself off these issues as I don't have the time to work on them (even review patches). I think Raymond may have gotten his Stefans mixed up and meant Stefan Behnel, who's also been looking at etree patches.