Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Crash in bytes constructor with mutating list #79154

Closed
serhiy-storchaka opened this issue Oct 13, 2018 · 8 comments
Closed

Crash in bytes constructor with mutating list #79154

serhiy-storchaka opened this issue Oct 13, 2018 · 8 comments
Assignees
@serhiy-storchaka
Labels
3.7 (EOL) end of life 3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@serhiy-storchaka
Copy link
Member

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
    Copy link
    Member Author

    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 serhiy-storchaka added 3.7 (EOL) end of life 3.8 only security fixes labels Oct 13, 2018
    @serhiy-storchaka serhiy-storchaka self-assigned this Oct 13, 2018
    @serhiy-storchaka serhiy-storchaka added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump labels Oct 13, 2018
    @serhiy-storchaka
    Copy link
    Member Author

    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
    Copy link
    Member

    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
    Copy link
    Member

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

    @serhiy-storchaka
    Copy link
    Member Author

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

    @serhiy-storchaka
    Copy link
    Member Author

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

    @miss-islington
    Copy link
    Contributor

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

    @miss-islington
    Copy link
    Contributor

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

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees

    @serhiy-storchaka serhiy-storchaka

    Labels
    3.7 (EOL) end of life 3.8 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    4 participants
    @avassalotti @serhiy-storchaka @zhangyangyu @miss-islington