classification
Title: Crash in bytes constructor with mutating list
Type: crash Stage: resolved
Components: Interpreter Core Versions: Python 3.8, Python 3.7, Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: alexandre.vassalotti, miss-islington, serhiy.storchaka, xiang.zhang
Priority: normal Keywords: patch

Created on 2018-10-13 13:37 by serhiy.storchaka, last changed 2018-10-21 13:10 by serhiy.storchaka. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 9841 merged serhiy.storchaka, 2018-10-13 15:18
PR 9842 closed serhiy.storchaka, 2018-10-13 15:28
PR 10026 merged miss-islington, 2018-10-21 12:26
PR 10027 merged miss-islington, 2018-10-21 12:26
Messages (8)
msg327653 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-10-13 13:37
Constructing bytes from mutating list can cause a crash.

class X:
    def __index__(self):
        if len(a) < 1000:
            a.append(self)
        return 0

a = [X()]
bytes(a)

This is not an issue about weird integer-like objects. It is about . The size of the list can be changed in other thread while iterate it in the bytes constructor.

The optimization for the case of the list argument was added in issue6688. The code was refactored several times since, but this flaw was always.
msg327655 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-10-13 15:36
I have two options for solving this result.

1. Add necessary checks for the list case. This will add a bunch of code and will slow down handling all lists because of additional checks.

2. Use the optimization for lists only when there are no other references to it. If no other references, the list can not be changed. This will not add much code, and may even slightly speed up cases for tuples and lists. But the code will be more fragile.

Of course there is an option of removing this optimization at all. But bytes([x]) and bytes((x,)) are common cases.
msg327870 - (view) Author: Xiang Zhang (xiang.zhang) * (Python committer) Date: 2018-10-17 08:25
I carefully read both the two PRs. The first one, easy to understand. The second one, I spend some time to figure out why the test doesn't crash, why we need to have reference count checks in two places and make some experiments to test in different cases, how the reference counts will be. I am afraid I have to repeat this procedure after some time when reading the code again. :-( And while in some cases the second approach increases performance. But in others cases it might hurt. Codes storing the array in a variable will go into the iterator branch.
msg327983 - (view) Author: Alexandre Vassalotti (alexandre.vassalotti) * (Python committer) Date: 2018-10-18 16:27
PR 9841 looks good to me. I wouldn't worry about the performance hit.
msg328207 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-10-21 12:25
Thank you Xiang and Alexandre. I'll merge PR 9841.
msg328208 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-10-21 12:25
New changeset 914f9a078f997e58cfcfabcbb30fafdd1f277bef by Serhiy Storchaka in branch 'master':
bpo-34973: Fix crash in bytes constructor. (GH-9841)
https://github.com/python/cpython/commit/914f9a078f997e58cfcfabcbb30fafdd1f277bef
msg328210 - (view) Author: miss-islington (miss-islington) Date: 2018-10-21 12:55
New changeset 7f34d550231e047c88a1817b58bda03a33817490 by Miss Islington (bot) in branch '3.7':
bpo-34973: Fix crash in bytes constructor. (GH-9841)
https://github.com/python/cpython/commit/7f34d550231e047c88a1817b58bda03a33817490
msg328211 - (view) Author: miss-islington (miss-islington) Date: 2018-10-21 12:56
New changeset 8bb037167cf3204a7d620ba11fbf43d2a9ec36e6 by Miss Islington (bot) in branch '3.6':
bpo-34973: Fix crash in bytes constructor. (GH-9841)
https://github.com/python/cpython/commit/8bb037167cf3204a7d620ba11fbf43d2a9ec36e6
History
Date User Action Args
2018-10-21 13:10:09serhiy.storchakasetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2018-10-21 12:56:14miss-islingtonsetmessages: + msg328211
2018-10-21 12:55:56miss-islingtonsetnosy: + miss-islington
messages: + msg328210
2018-10-21 12:26:20miss-islingtonsetpull_requests: + pull_request9366
2018-10-21 12:26:10miss-islingtonsetpull_requests: + pull_request9365
2018-10-21 12:25:59serhiy.storchakasetmessages: + msg328208
2018-10-21 12:25:15serhiy.storchakasetmessages: + msg328207
2018-10-18 16:27:10alexandre.vassalottisetmessages: + msg327983
2018-10-17 08:25:40xiang.zhangsetnosy: + xiang.zhang
messages: + msg327870
2018-10-13 15:36:50serhiy.storchakasetmessages: + msg327655
2018-10-13 15:28:14serhiy.storchakasetpull_requests: + pull_request9216
2018-10-13 15:18:25serhiy.storchakasetkeywords: + patch
stage: patch review
pull_requests: + pull_request9215
2018-10-13 13:37:47serhiy.storchakacreate