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.

Sign up for GitHub

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

sys.flags.__new__ crashes #57413

Closed
Trundle mannequin opened this issue Oct 17, 2011 · 12 comments
Closed

sys.flags.__new__ crashes #57413

Trundle mannequin opened this issue Oct 17, 2011 · 12 comments
Labels
type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@Trundle
Copy link
Mannequin

Trundle mannequin commented Oct 17, 2011

BPO 13204
Nosy @amauryfa, @pitrou, @vstinner, @ezio-melotti, @merwok, @Trundle
Files
  • sys_flags__new__crash.patch
  • sys_flags__new__crash_2.7.patch
  • issue13204.patch
    		•

    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 = None
closed_at = <Date 2014-04-28.11:09:58.458>
created_at = <Date 2011-10-17.22:14:25.187>
labels = ['type-crash']
title = 'sys.flags.__new__ crashes'
updated_at = <Date 2014-04-28.11:09:58.457>
user = 'https://github.com/Trundle'

    bugs.python.org fields:

    activity = <Date 2014-04-28.11:09:58.457>
actor = 'pitrou'
assignee = 'none'
closed = True
closed_date = <Date 2014-04-28.11:09:58.458>
closer = 'pitrou'
components = []
creation = <Date 2011-10-17.22:14:25.187>
creator = 'Trundle'
dependencies = []
files = ['23431', '23432', '35068']
hgrepos = []
issue_num = 13204
keywords = ['patch', 'needs review']
message_count = 12.0
messages = ['145763', '147089', '147092', '147100', '147103', '147106', '147490', '147563', '217338', '217359', '217360', '217362']
nosy_count = 8.0
nosy_names = ['amaury.forgeotdarc', 'pitrou', 'vstinner', 'ezio.melotti', 'eric.araujo', 'Trundle', 'jesstess', 'python-dev']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue13204'
versions = ['Python 2.7', 'Python 3.4', 'Python 3.5']
    @Trundle
    Copy link
    Mannequin Author

    Trundle mannequin commented Oct 17, 2011

    It's not possible (by intention) to instantiate a new instance of sys.flags. This is achieved by setting the "tp_new" slot to NULL (in _PySys_Init()), after PyType_Ready() is called, which means that a slot wrapper is added to the type dict for the "tp_new" slot (because the slot != NULL at that time). The problem is now that if one calls sys.flags.__new__ directly, a null pointer dereference occurs in tp_new_wrapper().

    Attached is a patch that fixes the crash and adds a test.

    @Trundle Trundle mannequin added the type-crash A hard crash of the interpreter, possibly with a core dump label Oct 17, 2011
    @merwok
    Copy link
    Member

    merwok commented Nov 5, 2011

    I’m not sure it is useful to fix this bug.

    @ezio-melotti
    Copy link
    Member

    Why not?

    @merwok
    Copy link
    Member

    merwok commented Nov 5, 2011

    Because the class of sys.flags is an implementation detail. Most people won’t try to instantiate it, IMO.

    @ezio-melotti
    Copy link
    Member

    That's probably true, but IMHO it's not a valid reason to keep the buggy behavior.

    @amauryfa
    Copy link
    Member

    amauryfa commented Nov 5, 2011

    Why would we want to prevent users from creating new instances of FlagsType?

    @merwok
    Copy link
    Member

    merwok commented Nov 12, 2011

    You are right. Even if it’s an undocumented internal type, there is no reason not to fix it. There are plenty of similar crash fixes committed in the repo.

    @pitrou
    Copy link
    Member

    pitrou commented Nov 13, 2011

    Thanks for the patch. You should add the same tests for sys.version_info and sys.getwindowsversion.

    @jesstess
    Copy link
    Member

    Thanks for reporting this and providing a patch, Trundle.

    The Python 3 patch didn't apply cleanly anymore, so I regenerated it and added tests for sys.version_info and sys.getwindowsversion.

    • The patch passes make patchcheck
    • The full test suite passes with this patch.
    • Testing manually, without this patch each of the following segfaults, and with the patch they raise errors instead:
    import sys; sys.flags.__new__(type(sys.flags))
import sys; sys.version_info.__new__(type(sys.version_info))
import sys; sys.getwindowsversion().__new__(type(sys.getwindowsversion()))

    One important caveat is that while I confirmed the sys.getwindowsversion segfault on Windows, I don't have Visual Studio set up so I couldn't build and test the new test for sys.getwindowsversion (I ran the full test suite on OSX, where this test is skipped).

    @pitrou
    Copy link
    Member

    pitrou commented Apr 28, 2014

    Thanks for the patch, Jessica. It seems to work under Windows here.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Apr 28, 2014

    New changeset 7052fdd90a11 by Antoine Pitrou in branch '3.4':
    Issue bpo-13204: Calling sys.flags.__new__ would crash the interpreter, now it raises a TypeError.
    http://hg.python.org/cpython/rev/7052fdd90a11

    New changeset a14012352f65 by Antoine Pitrou in branch 'default':
    Issue bpo-13204: Calling sys.flags.__new__ would crash the interpreter, now it raises a TypeError.
    http://hg.python.org/cpython/rev/a14012352f65

    @pitrou
    Copy link
    Member

    pitrou commented Apr 28, 2014

    I've committed the patch to 3.4 and 3.5. I'm closing the issue, I don't think fixing 2.7 is important at this point.

    @pitrou pitrou closed this as completed Apr 28, 2014
    @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
    No one assigned
    Labels
    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
    5 participants
    @jesstess @merwok @amauryfa @pitrou @ezio-melotti