This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Multiple use after frees in obj2ast_* methods
Type: crash Stage: resolved
Components: Extension Modules Versions: Python 3.7, Python 3.6, Python 3.5, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: Arfrever, benjamin.peterson, berker.peksag, christian.heimes, pkt, python-dev, serhiy.storchaka
Priority: high Keywords: patch

Created on 2015-05-01 14:10 by pkt, last changed 2022-04-11 14:58 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
poc_obj2mod.py pkt, 2015-05-01 14:10
issue24098.patch pkt, 2016-09-26 13:40 patch
issue24098-check-size.patch serhiy.storchaka, 2016-09-27 08:19 review
issue24098-iterate-tuple.patch serhiy.storchaka, 2016-09-27 08:19 review
Pull Requests
URL Status Linked Edit
PR 552 closed dstufft, 2017-03-31 16:36
Messages (9)
msg242315 - (view) Author: paul (pkt) Date: 2015-05-01 14:10
# 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
# 3619                    res = obj2ast_stmt(PyList_GET_ITEM(tmp, i), &value, arena);
# (gdb) n
# 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.
msg242750 - (view) Author: paul (pkt) Date: 2015-05-08 08:14
ping
msg242980 - (view) Author: paul (pkt) Date: 2015-05-12 15:42
ping
msg246066 - (view) Author: paul (pkt) Date: 2015-07-02 10:26
ping
msg246147 - (view) Author: paul (pkt) Date: 2015-07-03 07:46
ping
msg277419 - (view) Author: paul (pkt) Date: 2016-09-26 13:40
Fix by replacing static 'len' in loops with a macro, so that mutations of   size of the containter do not casue OOB reads.
msg277469 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2016-09-26 20:53
Please note that Python/Python-ast.c is automatically generated by Parser/asdl_c.py.
msg277504 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-09-27 08:19
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.
msg278261 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-10-07 18:57
New changeset 47d5bf5a846f by Serhiy Storchaka in branch '2.7':
Issue #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 #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 #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 #24098: Fixed possible crash when AST is changed in process of
https://hg.python.org/cpython/rev/def217aaad2f
History
Date User Action Args
2022-04-11 14:58:16adminsetgithub: 68286
2017-03-31 16:36:27dstufftsetpull_requests: + pull_request1008
2016-10-07 20:37:29serhiy.storchakasetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2016-10-07 18:57:47python-devsetnosy: + python-dev
messages: + msg278261
2016-10-05 08:05:05serhiy.storchakasetassignee: serhiy.storchaka
2016-09-27 08:19:21serhiy.storchakasetfiles: + issue24098-iterate-tuple.patch
2016-09-27 08:19:02serhiy.storchakasetfiles: + issue24098-check-size.patch

messages: + msg277504
versions: + Python 2.7, - Python 3.4
2016-09-26 20:53:22berker.peksagsetnosy: + berker.peksag, benjamin.peterson
messages: + msg277469
2016-09-26 13:44:04christian.heimessetpriority: normal -> high
stage: needs patch -> patch review
versions: + Python 3.6, Python 3.7
2016-09-26 13:40:22pktsetfiles: + issue24098.patch
keywords: + patch
messages: + msg277419
2015-07-03 07:46:07pktsetmessages: + msg246147
2015-07-02 10:26:20pktsetmessages: + msg246066
2015-05-12 15:42:17pktsetmessages: + msg242980
2015-05-08 08:14:42pktsetmessages: + msg242750
2015-05-03 06:48:04Arfreversetnosy: + Arfrever
2015-05-02 04:49:57serhiy.storchakasetnosy: + serhiy.storchaka
2015-05-01 14:13:35christian.heimessetnosy: + christian.heimes
2015-05-01 14:13:06christian.heimessetstage: needs patch
components: + Extension Modules
versions: + Python 3.5
2015-05-01 14:10:29pktcreate