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

[dekrain]

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]

>>> 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__

[stevendaprano]

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]

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]

_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]

(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]

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]

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]

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

Should we change the exception to RuntimeError?