Make struct.Struct() really immutable

[skirpichev]

Bug report

Bug description:

The Struct constructor permits creation of half-initialized Struct's, e.g.:

>>> from _struct import Struct
>>> s = Struct.__new__(Struct)
>>> s.unpack_from(b'boo!')  # this might be a crash!
Traceback (most recent call last):
  File "<python-input-2>", line 1, in <module>
    s.unpack_from(b'boo!')
    ~~~~~~~~~~~~~^^^^^^^^^
SystemError: Objects/tupleobject.c:40: bad argument to internal function
>>> s = Struct.__new__(Struct, 1, 2, 3)  # anything is accepted
>>> s.unpack_from(b'boo!')
Traceback (most recent call last):
  File "<python-input-6>", line 1, in <module>
    s.unpack_from(b'boo!')
    ~~~~~~~~~~~~~^^^^^^^^^
SystemError: Objects/tupleobject.c:40: bad argument to internal function

c.f.:

>>> int.__new__(int, 1, 2, 3)
Traceback (most recent call last):
  File "<python-input-7>", line 1, in <module>
    int.__new__(int, 1, 2,3 )
    ~~~~~~~~~~~^^^^^^^^^^^^^^
TypeError: int() takes at most 2 arguments (3 given)

The Struct.__new__() dunder handles only memory allocation, the rest goes to the Struct.__init__(). That doesn't make sense for immutable type (which Struct() pretend to be in fact) and introduce a number of issues, e.g.:

• _struct.Struct: calling functions without calling __init__ results in SystemError #78724
• assertion failures and a crash when using an uninitialized struct.Struct object #75960
• Use-after-free in s_pack_internal via re-entrant __bool__ #143379

The proper way to fix all this, probably, is moving all initialization logic to the Struct.__new__() dunder:

It is more convenient to initialize the Struct instance in __new__ than in __init__, and it makes sense, since Struct instances are cached and therefore can be considered immutable like ints or tuples. But the possibility of creating subclasses and the existence of subclasses in the wild makes this a breaking change.

Originally posted by @serhiy-storchaka in #112358

From docs:

A good rule of thumb is that for immutable types, all initialization should take place in tp_new, while for mutable types, most initialization should be deferred to tp_init.

This was done in #94532, which then was reverted due to introduced breackage (#112358).

I propose:

1. deprecate repeated calls of the Struct.__init__() on initialized Struct (will be a no-op eventually)
2. move all initialization logic to Struct.__new__(), make self.__init__() a no-op if __new__() got one argument
3. deprecate calls of Struct.__new__() without required argument.

The Struct.__init__() dunder method will be removed in the CPython 3.20. I suggest to close all opened referenced above issues as duplicates of this one.

CPython versions tested on:

CPython main branch

Operating systems tested on:

No response

Linked PRs

• gh-143715: deprecate incomplete initialization of struct.Struct() #143659
• gh-143715: Deprecate incomplete initialization of struct.Struct() #145580

[skirpichev]

CC @serhiy-storchaka

[serhiy-storchaka]

To arguments about which Struct should be considered immutable:

• Currently, only __init__() mutates a Struct object
• There is a global cache for Struct. Mutating the cached Struct object breaks the cache.

Initializing the Struct object in __init__() caused #78724. The most probable way to encounter that bug not when the Struct object was created by calling Struct.__new__(), but when super().__init__() was omitted in a Struct subclass.

The previous attempt to fix this issue failed because people write the code like:

class MyStruct(struct.Struct):
    def __init__(self):
        super().__init__('>h')

If the constructor would be implemented in __new__(), they would write it differently:

class MyStruct(struct.Struct):
    def __new__(cls):
        self = super().__new__(cls, '>h')
        return self

This is not compatible with the current implementation, and the current way is not compatible with initialization made in __new__(). If we are going to move the initialization code from __init__() to __new__(), we have to design the transition way which would cause less pain to users.

• The current idiom (calling super().__init__() in the subclass's __init__()) should rather fail than silently produce incorrect result in future Python versions. And if it fails, there should be a deprecation warning in the transition period.
• It should be possible to write a user code compatible with future Python versions which works without warnings during transitional period.
• It should be possible to write a user code compatible with future Python versions and with old Python versions which works without warnings during transitional period.
• Preferably, such compatible code should not add much overhead in comparison with the code written only for future Python versions.

This is not easy.

There is other way to make Struct practically immutable -- forbid repeated calls of Struct.__init__(). Between the calls of Struct.__new__() and Struct.__init__(), the object is in a "not initialized" state and should not be used. After Struct.__init__() was called, it becomes immutable, and Struct.__init__() cannot be called repeatedly. This will solve the problem that Struct can be mutated.

[serhiy-storchaka]

I am planning to also deprecate calls of __init__() method on initialized Picler and Unpickler objects in _pickle. They are not immutable, strictly speaking, but reinitialization during the call of their method can have bad consequences.

[skirpichev]

The current idiom (calling super().__init__() in the subclass's __init()__) should rather fail than silently produce incorrect result in future Python versions. And if it fails, there should be a deprecation warning in the transition period.

This emits a warning in my pr:

>>> import struct
... # Old idiom:
... class MyStruct(struct.Struct):
...     def __init__(self):
...         super().__init__('>h')
... # New idiom:
... class MyStruct2(struct.Struct):
...     def __new__(cls):
...         self = super().__new__(cls, '>h')
...         return self
...         
>>> MyStruct()
<python-input-2>:1: DeprecationWarning: Struct.__new__() has one positional argument
MyStruct('>h')

There is other way to make Struct practically immutable -- forbid repeated calls of Struct.__init__().

This is more easy to achieve (and done in my pr):

>>> s = struct.Struct('i')
>>> s.__init__('ii')
<python-input-7>:1: DeprecationWarning: Explicit call of __init__() on initialized Struct() is deprecated
>>> s.pack(1, 2)
b'\x01\x00\x00\x00\x02\x00\x00\x00'

It should be possible to write a user code compatible with future Python versions which works without warnings during transitional period.
It should be possible to write a user code compatible with future Python versions and with old Python versions which works without warnings during transitional period.

I think both goals are reachable, but not without hacks. I did format argument optional in the __init__(). If it's not provided and the object is not initialized in the __new__() - the TypeError raised, as before. In this way - the second idoom should work after merging PR without warnings:

>>> MyStruct2()
MyStruct2('>h')

[serhiy-storchaka]

The following code also should work without warnings:

class MyStruct(struct.Struct):
    def __new__(cls, arg):
        self = super().__new__(cls, '>h')
        return self

MyStruct(5)

Both Struct.__new__ and Struct.__init__ can be called explicitly (in the overwritten __new__ or __init__) or implicitly (when __new__ or __init__ are not defined in a Struct subclass). We can check tp_new and tp_init of the current class. If they are the same as in the base class, they were called implicitly. If Struct.__init__ is called implicitly, it should be virtually no-op. We can only check that Struct.__init__ was not called multiple times.