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.

[serhiy-storchaka]

Ping.

[scoder]

Etree patch looks straight forward to me, feel free to apply it.

[python-dev]

New changeset e44bd1259bda by Serhiy Storchaka in branch '3.5':
Issue bpo-25455: Fixed a crash in repr of ElementTree.Element with recursive tag.
https://hg.python.org/cpython/rev/e44bd1259bda

New changeset e3671a684ea0 by Serhiy Storchaka in branch 'default':
Issue bpo-25455: Fixed a crash in repr of ElementTree.Element with recursive tag.
https://hg.python.org/cpython/rev/e3671a684ea0

[python-dev]

New changeset c071da010053 by Serhiy Storchaka in branch '2.7':
Issue bpo-25455: Fixed a crash in repr of cElementTree.Element with recursive tag.
https://hg.python.org/cpython/rev/c071da010053

New changeset 17e78918f608 by Serhiy Storchaka in branch '3.5':
Issue bpo-25455: Fixed a crash in repr of recursive functools.partial objects.
https://hg.python.org/cpython/rev/17e78918f608

New changeset 86959c696ab7 by Serhiy Storchaka in branch 'default':
Issue bpo-25455: Fixed a crash in repr of recursive functools.partial objects.
https://hg.python.org/cpython/rev/86959c696ab7

[berkerpeksag]

I'm not sure if 17e78918f608 is relevant but test_gc is failing on several buildbots:

• http://buildbot.python.org/all/builders/PPC64%20Fedora%203.5/builds/716
  http://buildbot.python.org/all/builders/PPC64%20Fedora%203.5/builds/716/steps/test/logs/stdio

• http://buildbot.python.org/all/builders/s390x%20RHEL%203.5/builds/775
  http://buildbot.python.org/all/builders/s390x%20RHEL%203.5/builds/775/steps/test/logs/stdio

[serhiy-storchaka]

The patch for io classes needed an update.

[python-dev]

New changeset 7859742826b2 by Serhiy Storchaka in branch '2.7':
Issue bpo-25455: Backported tests for pickling recursive functools.partial objects.
https://hg.python.org/cpython/rev/7859742826b2

[serhiy-storchaka]

Indeed, tests for recursive partial objects create reference loops and don't clean them. Thank you Berker. I'll fix this.

[python-dev]

New changeset 0323b33894f2 by Serhiy Storchaka in branch '3.5':
Issue bpo-25455: Clean up reference loops created in tests for recursive
https://hg.python.org/cpython/rev/0323b33894f2

New changeset 688edc946ab9 by Serhiy Storchaka in branch '2.7':
Issue bpo-25455: Clean up reference loops created in tests for recursive
https://hg.python.org/cpython/rev/688edc946ab9

New changeset 818a10534e44 by Serhiy Storchaka in branch 'default':
Issue bpo-25455: Clean up reference loops created in tests for recursive
https://hg.python.org/cpython/rev/818a10534e44

[serhiy-storchaka]

Antoine, are you fine with io_recursive_repr2.patch?

[python-dev]

New changeset ea1edf1bf362 by Benjamin Peterson in branch '2.7':
when you enter repr, you must leave, too (bpo-25455)
https://hg.python.org/cpython/rev/ea1edf1bf362

[serhiy-storchaka]

New changeset 08612ed by Serhiy Storchaka in branch '3.5':
bpo-25455: Fixed crashes in repr of recursive buffered file-like objects. (#514) (#727)
08612ed

[serhiy-storchaka]

New changeset fca705d by Serhiy Storchaka in branch '3.6':
bpo-25455: Fixed crashes in repr of recursive buffered file-like objects. (#514) (#722)
fca705d

[serhiy-storchaka]

New changeset a5af6e1 by Serhiy Storchaka in branch 'master':
bpo-25455: Fixed crashes in repr of recursive buffered file-like objects. (#514)
a5af6e1