Crash in bytes constructor with mutating list

[serhiy-storchaka]

BPO	34973
Nosy	@avassalotti, @serhiy-storchaka, @zhangyangyu, @miss-islington
PRs	bpo-34973: Fix crash in bytes constructor. (variant 1) #9841 bpo-34973: Fix crash in bytes constructor. (variant 2) #9842 [3.7] bpo-34973: Fix crash in bytes constructor. (GH-9841) #10026 [3.6] bpo-34973: Fix crash in bytes constructor. (GH-9841) #10027

^{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 2018-10-21.13:10:09.843>
created_at = <Date 2018-10-13.13:37:47.798>
labels = ['interpreter-core', '3.7', '3.8', 'type-crash']
title = 'Crash in bytes constructor with mutating list'
updated_at = <Date 2018-10-21.13:10:09.842>
user = 'https://github.com/serhiy-storchaka'

bugs.python.org fields:

activity = <Date 2018-10-21.13:10:09.842>
actor = 'serhiy.storchaka'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2018-10-21.13:10:09.843>
closer = 'serhiy.storchaka'
components = ['Interpreter Core']
creation = <Date 2018-10-13.13:37:47.798>
creator = 'serhiy.storchaka'
dependencies = []
files = []
hgrepos = []
issue_num = 34973
keywords = ['patch']
message_count = 8.0
messages = ['327653', '327655', '327870', '327983', '328207', '328208', '328210', '328211']
nosy_count = 4.0
nosy_names = ['alexandre.vassalotti', 'serhiy.storchaka', 'xiang.zhang', 'miss-islington']
pr_nums = ['9841', '9842', '10026', '10027']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue34973'
versions = ['Python 3.6', 'Python 3.7', 'Python 3.8']

[serhiy-storchaka]

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 bpo-6688. The code was refactored several times since, but this flaw was always.

[serhiy-storchaka]

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.

[zhangyangyu]

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.

[avassalotti]

PR 9841 looks good to me. I wouldn't worry about the performance hit.

[serhiy-storchaka]

Thank you Xiang and Alexandre. I'll merge PR 9841.

[serhiy-storchaka]

New changeset 914f9a0 by Serhiy Storchaka in branch 'master':
bpo-34973: Fix crash in bytes constructor. (GH-9841)
914f9a0

[miss-islington]

New changeset 7f34d55 by Miss Islington (bot) in branch '3.7':
bpo-34973: Fix crash in bytes constructor. (GH-9841)
7f34d55

[miss-islington]

New changeset 8bb0371 by Miss Islington (bot) in branch '3.6':
bpo-34973: Fix crash in bytes constructor. (GH-9841)
8bb0371