Multiple use after frees in obj2ast_* methods

[pkt]

BPO	24098
Nosy	@tiran, @benjaminp, @berkerpeksag, @serhiy-storchaka
PRs	[Do Not Merge] Convert Misc/NEWS so that it is managed by towncrier #552
Files	poc_obj2mod.py issue24098.patch: patch issue24098-check-size.patch issue24098-iterate-tuple.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-10-07.20:37:29.733>
created_at = <Date 2015-05-01.14:10:29.629>
labels = ['extension-modules', '3.7', 'type-crash']
title = 'Multiple use after frees in obj2ast_* methods'
updated_at = <Date 2017-03-31.16:36:27.216>
user = 'https://bugs.python.org/pkt'

bugs.python.org fields:

activity = <Date 2017-03-31.16:36:27.216>
actor = 'dstufft'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2016-10-07.20:37:29.733>
closer = 'serhiy.storchaka'
components = ['Extension Modules']
creation = <Date 2015-05-01.14:10:29.629>
creator = 'pkt'
dependencies = []
files = ['39249', '44830', '44838', '44839']
hgrepos = []
issue_num = 24098
keywords = ['patch']
message_count = 9.0
messages = ['242315', '242750', '242980', '246066', '246147', '277419', '277469', '277504', '278261']
nosy_count = 7.0
nosy_names = ['christian.heimes', 'benjamin.peterson', 'Arfrever', 'python-dev', 'berker.peksag', 'serhiy.storchaka', 'pkt']
pr_nums = ['552']
priority = 'high'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue24098'
versions = ['Python 2.7', 'Python 3.5', 'Python 3.6', 'Python 3.7']

[pkt]

\bpo-3617 for (i = 0; i < len; i++) {
# (gdb) print *(PyListObject*)tmp
# $1 = {ob_base = {ob_base = {ob_next = 0x4056f8f4, _ob_prev = 0x4057329c, ob_refcnt = 2, ob_type = 0x830e1c0 <PyList_Type>},
# ob_size = 1337}, ob_item = 0x8491ae0, allocated = 1432}
# (gdb) n
\bpo-3619 res = obj2ast_stmt(PyList_GET_ITEM(tmp, i), &value, arena);
# (gdb) n
\bpo-3620 if (res != 0) goto failed;
# (gdb) print *(PyListObject*)tmp
# $2 = {ob_base = {ob_base = {ob_next = 0x4056f8f4, _ob_prev = 0x4057329c, ob_refcnt = 2, ob_type = 0x830e1c0 <PyList_Type>},
# ob_size = 1}, ob_item = 0x8491ae0, allocated = 4}
# (gdb) c
# Continuing.
#
# Program received signal SIGSEGV, Segmentation fault.
# 0x080f2c17 in PyObject_GetAttr (v=<unknown at remote 0x405733b4>, name='lineno') at Objects/object.c:872
# 872 if (tp->tp_getattro != NULL)
#
# Objects freed in __getattr are used later in the loop above. There are two
# bugs actually. One is the use-after-free and the second is using a stale size
# variable "len" to control the for(...) loop. "body" can be mutated inside
# obj2ast_stmt.

This construct:

            for (i = 0; i < len; i++) {
                stmt_ty value;
                res = obj2ast_stmt(PyList_GET_ITEM(tmp, i), &value, arena);
                if (res != 0) goto failed;
                asdl_seq_SET(body, i, value);
            }

is repeated multiple times in multiple obj2ast_ methods. It contains two bugs:

1. tmp[i] isn't protected from deletion inside python code (refcnt is not increased by GET_ITEM),
2. tmp's length can drop below "len" resulting in an OOB read, because the loop counter is static.

[pkt]

ping

[pkt]

ping

[pkt]

ping

[pkt]

ping

[pkt]

Fix by replacing static 'len' in loops with a macro, so that mutations of size of the containter do not casue OOB reads.

[berkerpeksag]

Please note that Python/Python-ast.c is automatically generated by Parser/asdl_c.py.

[serhiy-storchaka]

Bad things happen not only when a list shrinks, but also when it grows during iteration.

The one solution is to check if the size is changed on every iteration. The other solution is to convert a list to a tuple for iterating.

[python-dev]

New changeset 47d5bf5a846f by Serhiy Storchaka in branch '2.7':
Issue bpo-24098: Fixed possible crash when AST is changed in process of
https://hg.python.org/cpython/rev/47d5bf5a846f

New changeset f575710b5f56 by Serhiy Storchaka in branch '3.5':
Issue bpo-24098: Fixed possible crash when AST is changed in process of
https://hg.python.org/cpython/rev/f575710b5f56

New changeset 7528154cadaa by Serhiy Storchaka in branch '3.6':
Issue bpo-24098: Fixed possible crash when AST is changed in process of
https://hg.python.org/cpython/rev/7528154cadaa

New changeset def217aaad2f by Serhiy Storchaka in branch 'default':
Issue bpo-24098: Fixed possible crash when AST is changed in process of
https://hg.python.org/cpython/rev/def217aaad2f