Issue34973
Created on 2018-10-13 13:37 by serhiy.storchaka, last changed 2022-04-11 14:59 by admin. 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) * | 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) * | 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) * | 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) * | 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) * | Date: 2018-10-21 12:25 | |
Thank you Xiang and Alexandre. I'll merge PR 9841. |
|||
| msg328208 - (view) | Author: Serhiy Storchaka (serhiy.storchaka) * | 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 |
| 2022-04-11 14:59:07 | admin | set | github: 79154 |
| 2018-10-21 13:10:09 | serhiy.storchaka | set | status: open -> closed resolution: fixed stage: patch review -> resolved |
| 2018-10-21 12:56:14 | miss-islington | set | messages: + msg328211 |
| 2018-10-21 12:55:56 | miss-islington | set | nosy:
+ miss-islington messages: + msg328210 |
| 2018-10-21 12:26:20 | miss-islington | set | pull_requests: + pull_request9366 |
| 2018-10-21 12:26:10 | miss-islington | set | pull_requests: + pull_request9365 |
| 2018-10-21 12:25:59 | serhiy.storchaka | set | messages: + msg328208 |
| 2018-10-21 12:25:15 | serhiy.storchaka | set | messages: + msg328207 |
| 2018-10-18 16:27:10 | alexandre.vassalotti | set | messages: + msg327983 |
| 2018-10-17 08:25:40 | xiang.zhang | set | nosy:
+ xiang.zhang messages: + msg327870 |
| 2018-10-13 15:36:50 | serhiy.storchaka | set | messages: + msg327655 |
| 2018-10-13 15:28:14 | serhiy.storchaka | set | pull_requests: + pull_request9216 |
| 2018-10-13 15:18:25 | serhiy.storchaka | set | keywords:
+ patch stage: patch review pull_requests: + pull_request9215 |
| 2018-10-13 13:37:47 | serhiy.storchaka | create | |