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

_struct.Struct: calling functions without calling __init__ results in SystemError #78724

Closed
dekrain mannequin opened this issue Aug 29, 2018 · 12 comments · Fixed by #94532
Closed

_struct.Struct: calling functions without calling __init__ results in SystemError #78724

dekrain mannequin opened this issue Aug 29, 2018 · 12 comments · Fixed by #94532
Assignees
Labels
3.10 3.11 3.12 extension-modules C modules in the Modules dir type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@dekrain
Copy link
Mannequin

dekrain mannequin commented Aug 29, 2018

BPO 34543
Nosy @ronaldoussoren, @stevendaprano, @ZackerySpytz, @dekrain, @iritkatriel
PRs
  • bpo-34543: Fix SystemErrors and segfaults with uninitialized Structs #14777
  • 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 = None
    created_at = <Date 2018-08-29.16:18:52.469>
    labels = ['extension-modules', '3.10', '3.9', 'type-crash', '3.11']
    title = '_struct.Struct: calling functions without calling __init__ results in SystemError'
    updated_at = <Date 2021-10-19.11:59:55.712>
    user = 'https://github.com/dekrain'

    bugs.python.org fields:

    activity = <Date 2021-10-19.11:59:55.712>
    actor = 'iritkatriel'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Extension Modules']
    creation = <Date 2018-08-29.16:18:52.469>
    creator = 'DeKrain'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 34543
    keywords = ['patch']
    message_count = 12.0
    messages = ['324330', '324331', '324335', '324338', '324341', '324484', '324498', '324504', '324505', '324507', '324509', '404291']
    nosy_count = 5.0
    nosy_names = ['ronaldoussoren', 'steven.daprano', 'ZackerySpytz', 'DeKrain', 'iritkatriel']
    pr_nums = ['14777']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'crash'
    url = 'https://bugs.python.org/issue34543'
    versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']

    @dekrain
    Copy link
    Mannequin Author

    dekrain mannequin commented Aug 29, 2018

    >>> from _struct import Struct
    >>> s = Struct.__new__(Struct)
    >>> s.unpack_from(b'asdf')
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    SystemError: /Objects/tupleobject.c:84: Bad argument to internal function

    In Modules/_struct.c:

    static PyObject *
    s_unpack_internal(PyStructObject *soself, const char *startfrom) {
    ...
    PyObject *result = PyTuple_New(soself->s_len);
    // soself->s_len is -1, set in Struct.__new__

    @dekrain dekrain mannequin added type-bug An unexpected behavior, bug, or error extension-modules C modules in the Modules dir 3.7 labels Aug 29, 2018
    @stevendaprano
    Copy link
    Member

    stevendaprano commented Aug 29, 2018

    This exception goes back to at least Python 2.6 (if not older) but I'm not convinced it is a bug.

    Calling __new__ alone is not guaranteed to initialise a new instance completely. The public API for creating an instance is to call the class object:

        s = Struct()

    not to call __new__. You bypassed the proper initialisation of the instance, resulting in a broken, half-initialised instance. When you tried to use it, it correctly raised an exception.

    If this caused a crash or a seg fault, then it would be reasonable to report it as a bug, but it looks to me that this is behaving correctly.

    If you disagree, please explain why you think it is a bug.

    (Also, for the record, you shouldn't be importing Struct from the private module _struct, you should import it from the public struct module.)

    @dekrain
    Copy link
    Mannequin Author

    dekrain mannequin commented Aug 29, 2018

    Well, sometimes when i do
    >>> b = bytearray()
    >>> s.pack_into(b)

    application crashes (because it checks arg #1, which is not initialized).
    Also, I imported from _struct, because it's where implementation of Struct really is.

    @stevendaprano
    Copy link
    Member

    stevendaprano commented Aug 29, 2018

    _struct is a private implementation detail. You shouldn't use it. You shouldn't care where the implementation "really is" in your Python code, because it could move without warning. There are no backwards-compatibility guarantees for private modules like _struct.

    But regardless of where you are importing it from, why are you calling Struct.__new__(Struct) in the first place? You should be calling Struct().

    I still don't see any reason to consider this a bug. I can't reproduce your report of a crash:

    py> from _struct import Struct
    py> s = Struct.__new__(Struct)
    py> b = bytearray()
    py> s.pack_into(b)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    SystemError: null argument to internal routine

    I get an exception, which is the correct behaviour. Unless this segfaults, I don't believe this is a bug that needs fixing.

    (By the way, Struct doesn't even have a __new__ method. You are calling the __new__ method inherited from object, which clearly knows nothing about how to initialise a Struct.)

    @dekrain
    Copy link
    Mannequin Author

    dekrain mannequin commented Aug 29, 2018

    (I wrote that I'm importing from _struct just for this issue.)
    I've seen that tp_new of PyStructType is set to s_new in Modules/_struct.c.
    And that crash is most likely caused by access to uninitialized memory, so it is not guaranteed.

    @stevendaprano
    Copy link
    Member

    stevendaprano commented Sep 2, 2018

    I've tried running this code in Python 3.6:

    from _struct import Struct
    for i in range(100000):
        L = [Struct.__new__(Struct) for j in range(1000)]
        for s in L:
            try:
                x = s.pack_into(bytearray())
            except SystemError:
                pass

    I've run it 6 times, for a total of 600 million calls to Struct.__new__
    and pack_into, and I cannot reproduce any crash or segfault. An
    exception (SystemError) is the correct behaviour.

    Is anyone able to try it under Python 3.7?

    Unless somebody is able to demonstrate a segfault or core dump, or
    otherwise demonstrate a problem with the C code, I think this ticket
    ought to be closed.

    @ronaldoussoren
    Copy link
    Contributor

    ronaldoussoren commented Sep 3, 2018

    IMHO SystemError is the wrong exception, that exception is primarily used to signal implementation errors.

    BTW. I can reproduce crashes in a couple of runs of your scriptlet:

    Python 3.7.0 (v3.7.0:1bf9cc5093, Jun 26 2018, 23:26:24) 
    [Clang 6.0 (clang-600.0.57)] on darwin
    Type "help", "copyright", "credits" or "license" for more information.
    >>> from _struct import Struct
    >>> for i in range(100000):
    ...     L = [Struct.__new__(Struct) for j in range(1000)]
    ...     for s in L:
    ...         try:
    ...             x = s.pack_into(bytearray())
    ...         except SystemError:
    ...             pass
    ... 
    Traceback (most recent call last):
      File "<stdin>", line 5, in <module>
    TypeError: 'code' object cannot be interpreted as an integer
    >>>             
    >>> from _struct import Struct
    >>> for i in range(100000):
    ...     L = [Struct.__new__(Struct) for j in range(1000)]
    ...     for s in L:
    ...         try:
    ...             x = s.pack_into(bytearray())
    ...         except SystemError:
    ...             pass
    ... 
    Traceback (most recent call last):
      File "<stdin>", line 5, in <module>
    TypeError: 'traceback' object cannot be interpreted as an integer
    >>> 
    >>> 
    >>> 
    >>> from _struct import Struct
    >>> for i in range(100000):
    ...     L = [Struct.__new__(Struct) for j in range(1000)]
    ...     for s in L:
    ...         try:
    ...             x = s.pack_into(bytearray())
    ...         except SystemError:
    ...             pass
    ... 
    Segmentation fault: 11

    @stevendaprano
    Copy link
    Member

    stevendaprano commented Sep 3, 2018

    Thanks for confirming the seg fault. I've changed this to a crasher.

    Should we change the exception to RuntimeError?

    @stevendaprano stevendaprano added stdlib Python modules in the Lib dir type-crash A hard crash of the interpreter, possibly with a core dump and removed extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error labels Sep 3, 2018
    @ronaldoussoren
    Copy link
    Contributor

    ronaldoussoren commented Sep 3, 2018

    It's not as easy as that, the SystemError in the original report is caused by invalid use of a C-API due to partial initialisation of an _struct.Struct instance.

    The solution is likely two-fold:

    1. Ensure that __new__ fully initialises the fields in de C struct to some value

    2. (Possibly) check that fields in the C structure have a sane value before using them. This part can have a measurable performance cost, and it would be nicer to avoid this by picking smart values in (1).

    The most important bit is the first step, even if that keeps raising SystemError when only calling Struct.__new__ because this avoid crashing the interpreter.

    @dekrain
    Copy link
    Mannequin Author

    dekrain mannequin commented Sep 3, 2018

    I think we should leave 'Extension Modules' in components field, because implementation of struct module is really written in C.

    @ronaldoussoren
    Copy link
    Contributor

    ronaldoussoren commented Sep 3, 2018

    @dekrain: I agree

    @ronaldoussoren ronaldoussoren added extension-modules C modules in the Modules dir and removed stdlib Python modules in the Lib dir labels Sep 3, 2018
    @ZackerySpytz ZackerySpytz mannequin added 3.8 3.9 labels Jul 14, 2019
    @iritkatriel
    Copy link
    Member

    iritkatriel commented Oct 19, 2021

    Reproduced on 3.11:

    >>> from _struct import Struct
    >>> s = Struct.__new__(Struct)
    >>> s.unpack_from(b'asdf')
    Assertion failed: (self->s_codes != NULL), function Struct_unpack_from_impl, file /Users/iritkatriel/src/cpython/Modules/_struct.c, line 1603.
    zsh: abort      ./python.exe

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @kumaraditya303 kumaraditya303 self-assigned this Jul 3, 2022
    @kumaraditya303 kumaraditya303 removed the 3.9 label Jul 3, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.10 3.11 3.12 extension-modules C modules in the Modules dir type-crash A hard crash of the interpreter, possibly with a core dump
    Projects
    None yet
    Development

    Successfully merging a pull request may close this issue.

    4 participants