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

PyStructSequence_NewType is broken; makes GC type without setting Py_TPFLAGS_HEAPTYPE #72895

Closed
MojoVampire mannequin opened this issue Nov 16, 2016 · 4 comments
Closed
Assignees
Labels
3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@MojoVampire
Copy link
Mannequin

MojoVampire mannequin commented Nov 16, 2016

BPO 28709
Nosy @encukou, @serhiy-storchaka, @MojoVampire
Files
  • testnewtype.c: C source for test module
  • setup.py
  • 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-11-14.10:01:50.335>
    created_at = <Date 2016-11-16.00:00:14.437>
    labels = ['interpreter-core', '3.7', 'type-crash']
    title = 'PyStructSequence_NewType is broken; makes GC type without setting Py_TPFLAGS_HEAPTYPE'
    updated_at = <Date 2018-11-14.10:01:50.335>
    user = 'https://github.com/MojoVampire'

    bugs.python.org fields:

    activity = <Date 2018-11-14.10:01:50.335>
    actor = 'petr.viktorin'
    assignee = 'serhiy.storchaka'
    closed = True
    closed_date = <Date 2018-11-14.10:01:50.335>
    closer = 'petr.viktorin'
    components = ['Interpreter Core']
    creation = <Date 2016-11-16.00:00:14.437>
    creator = 'josh.r'
    dependencies = []
    files = ['45495', '45496']
    hgrepos = []
    issue_num = 28709
    keywords = []
    message_count = 4.0
    messages = ['280904', '280905', '280908', '329895']
    nosy_count = 3.0
    nosy_names = ['petr.viktorin', 'serhiy.storchaka', 'josh.r']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'crash'
    url = 'https://bugs.python.org/issue28709'
    versions = ['Python 3.4', 'Python 3.5', 'Python 3.6', 'Python 3.7']

    @MojoVampire
    Copy link
    Mannequin Author

    MojoVampire mannequin commented Nov 16, 2016

    I could be missing something, but it looks like PyStructSequence_NewType is guaranteed broken. Specifically, it allocates the memory with PyType_GenericAlloc (use PyType_Type as the base), and PyType declares itself to have garbage collected instances, so the memory is allocated with _PyObject_GC_Malloc and added to GC tracking just before PyType_GenericAlloc returns.

    Problem is, PyStructSequence_Init2 copies from a template struct which sets Py_TPFLAGS_DEFAULT. So even though the new struct sequence is GC allocated and managed, it doesn't set Py_TPFLAGS_HEAPTYPE, which means when GC tries to traverse it, type's type_traverse errors out with:

    Fatal Python error: type_traverse() called for non-heap type 'NameOfStructSequence'

    It's possible I'm missing something here, so I've attached simple test code for others to confirm (it omits most error checking for simplicity/readability).

    Just compile the extension module, then run (with the module in the working directory):

    python -c "import testnewtype; Foo = testnewtype.makeseq('Foo', ['x', 'y'])"
    

    There is a commented out line in the test code that explicitly sets the HEAPTYPE flag after type construction (no idea if that's supposed to be supported), and uncommenting it seems to fix the crash (though again, if retroactively flagging as HEAPTYPE is unsupported, something else may break here).

    I can't find any actual use of PyStructSequence_NewType in the CPython code base, which probably explains why this hasn't been seen; odds are, most extensions using struct sequences are using Init, not NewType, and I only ran into this because I was experimenting with a struct sequence based replacement for collections.namedtuple (to end the start up time objections to using namedtuple in the built-in modules, e.g. bpo-28638).

    @MojoVampire MojoVampire mannequin added 3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Nov 16, 2016
    @MojoVampire MojoVampire mannequin changed the title PyStructSequence_NewType is broken PyStructSequence_NewType is broken; makes GC type without setting Py_TPFLAGS_HEAPTYPE Nov 16, 2016
    @MojoVampire
    Copy link
    Mannequin Author

    MojoVampire mannequin commented Nov 16, 2016

    Note: Uncommenting the line that forces Py_TPFLAGS_HEAPTYPE isn't enough, since it looks like the PyHeapTypeObject fields aren't initialized properly, causing seg faults if you access, for example, __name__/qualname (or print the type's repr, which implicitly accesses same):

    python -c "import testnewtype; Foo = testnewtype.makeseq('Foo', ['x', 'y']); print(Foo.__name__)"
    

    The type behaves properly otherwise (you can make instances, access values on them), but crashing on repr is probably poor form. :-)

    @MojoVampire
    Copy link
    Mannequin Author

    MojoVampire mannequin commented Nov 16, 2016

    On further checking, looks like there is a lot of work that should be done to initialize heap types (see PyType_FromSpecWithBases) that PyStructSequeuence_Init2 doesn't do (because it thinks it's working on a static type). I think the solution here is decouple PyStructSequeuence_NewType from PyStructSequeuence_Init2 (or to minimize code duplication, make both of them call a third internal function that accepts additional flags, e.g. to make the type a HEAPTYPE, BASETYPE, or both, and performs the additional work required for those flags if given); Init2 clearly expects a static type, and definitionally, NewType is producing a heap/dynamic type.

    @MojoVampire MojoVampire mannequin added the type-crash A hard crash of the interpreter, possibly with a core dump label Nov 16, 2016
    @serhiy-storchaka serhiy-storchaka self-assigned this Jun 16, 2017
    @encukou
    Copy link
    Member

    encukou commented Nov 14, 2018

    Should be fixed in PR9665 (bpo-34784), thanks to Eddie Elizondo.

    @encukou encukou closed this as completed Nov 14, 2018
    @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
    Labels
    3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants