Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 33 additions & 3 deletions Lib/test/test_struct.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import unittest
import struct
import sys
import warnings
import weakref

from test import support
Expand Down Expand Up @@ -575,11 +576,15 @@ def test_Struct_reinitialization(self):
# Struct instance. This test can be used to detect the leak
# when running with regrtest -L.
s = struct.Struct('i')
s.__init__('ii')
with self.assertWarns(DeprecationWarning):
s.__init__('ii')
with warnings.catch_warnings():
warnings.simplefilter("error", DeprecationWarning)
self.assertRaises(DeprecationWarning, s.__init__, 'ii')

def check_sizeof(self, format_str, number_of_codes):
# The size of 'PyStructObject'
totalsize = support.calcobjsize('2n3P')
totalsize = support.calcobjsize('2n3PB')
# The size taken up by the 'formatcode' dynamic array
totalsize += struct.calcsize('P3n0P') * (number_of_codes + 1)
support.check_sizeof(self, struct.Struct(format_str), totalsize)
Expand Down Expand Up @@ -783,7 +788,8 @@ class MyStruct(struct.Struct):
def __init__(self):
super().__init__('>h')

my_struct = MyStruct()
with self.assertWarns(DeprecationWarning):
my_struct = MyStruct()
self.assertEqual(my_struct.pack(12345), b'\x30\x39')

def test_repr(self):
Expand Down Expand Up @@ -816,6 +822,30 @@ def test_endian_table_init_subinterpreters(self):
results = executor.map(exec, [code] * 5)
self.assertListEqual(list(results), [None] * 5)

def test_Struct_object_mutation_via_dunders(self):
S = struct.Struct('?I')
buf = array.array('b', b' '*100)

class Evil():
def __bool__(self):
# This rebuilds format codes during S.pack().
S.__init__('I')
return True

with self.assertWarns(DeprecationWarning):
self.assertRaises(RuntimeError, S.pack, Evil(), 1)
self.assertRaises(RuntimeError, S.pack_into, buf, 0, Evil(), 1)

def test_operations_on_half_initialized_Struct(self):
with self.assertWarns(DeprecationWarning):
S = struct.Struct.__new__(struct.Struct)

spam = array.array('b', b' ')
for attr in ['iter_unpack', 'pack', 'pack_into',
'unpack', 'unpack_from']:
meth = getattr(S, attr)
self.assertRaises(RuntimeError, meth, spam)
Comment thread
skirpichev marked this conversation as resolved.
Outdated


class UnpackIteratorTest(unittest.TestCase):
"""
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Fix use-after-free in :meth:`struct.Struct.pack` when the
:class:`struct.Struct` object is mutated by dunder methods (like
:meth:`object.__bool__`) during packing of arguments. Now this trigger
:exc:`RuntimeError`. Patch by Sergey B Kirpichev.
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
Prevent crashes due to using half-initialized :class:`~struct.Struct`
objects. For example, created by ``Struct.__new__(Struct)``. Calling the
``Struct.__new__()`` dunder without required argument now is deprecated.
Calling :meth:`~object.__init__` dunder method of :class:`~struct.Struct`
object on initialized object now also is deprecated. Patch by Sergey B
Kirpichev.
60 changes: 54 additions & 6 deletions Modules/_struct.c
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@ typedef struct {
formatcode *s_codes;
PyObject *s_format;
PyObject *weakreflist; /* List of weak references */
PyMutex mutex; /* to prevent mutation during packing */
bool ready;
Comment thread
skirpichev marked this conversation as resolved.
Outdated
} PyStructObject;

#define PyStructObject_CAST(op) ((PyStructObject *)(op))
Expand Down Expand Up @@ -1762,6 +1764,12 @@ s_new(PyTypeObject *type, PyObject *args, PyObject *kwds)
{
PyObject *self;

if (PyTuple_GET_SIZE(args) != 1
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not see how can this be useful.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to trigger deprecation warning in a following scenario:

>>> import struct
>>> class MyStruct(struct.Struct):
...     def __init__(self):
...         super().__init__('>h')
...         
>>> my_struct = MyStruct()
<python-input-2>:1: DeprecationWarning: Struct().__new__() has one required argument

PS: deprecation part moved to #143659.

&& PyErr_WarnEx(PyExc_DeprecationWarning,
"Struct().__new__() has one required argument", 1))
{
return NULL;
}
assert(type != NULL);
allocfunc alloc_func = PyType_GetSlot(type, Py_tp_alloc);
assert(alloc_func != NULL);
Expand All @@ -1773,6 +1781,8 @@ s_new(PyTypeObject *type, PyObject *args, PyObject *kwds)
s->s_codes = NULL;
s->s_size = -1;
s->s_len = -1;
s->mutex = (PyMutex){0};
s->ready = false;
}
return self;
}
Expand All @@ -1794,8 +1804,13 @@ static int
Struct___init___impl(PyStructObject *self, PyObject *format)
/*[clinic end generated code: output=b8e80862444e92d0 input=192a4575a3dde802]*/
{
int ret = 0;

Comment thread
skirpichev marked this conversation as resolved.
if (self->ready
&& PyErr_WarnEx(PyExc_DeprecationWarning,
("Explicit call of __init__() on "
"initialized Struct()"), 1))
Comment thread
skirpichev marked this conversation as resolved.
Outdated
{
return -1;
}
if (PyUnicode_Check(format)) {
format = PyUnicode_AsASCIIString(format);
if (format == NULL)
Expand All @@ -1816,8 +1831,16 @@ Struct___init___impl(PyStructObject *self, PyObject *format)

Py_SETREF(self->s_format, format);

ret = prepare_s(self);
return ret;
if (PyMutex_IsLocked(&self->mutex)) {
PyErr_SetString(PyExc_RuntimeError,
"Call Struct.__init__() in struct.pack()");
return -1;
}
if (prepare_s(self)) {
return -1;
}
self->ready = true;
return 0;
}

static int
Expand Down Expand Up @@ -1917,6 +1940,10 @@ Struct_unpack_impl(PyStructObject *self, Py_buffer *buffer)
/*[clinic end generated code: output=873a24faf02e848a input=3113f8e7038b2f6c]*/
{
_structmodulestate *state = get_struct_state_structinst(self);
if (!self->ready) {
PyErr_SetString(PyExc_RuntimeError, "Call unpack on non-initialized Struct()");
return NULL;
}
assert(self->s_codes != NULL);
Comment thread
skirpichev marked this conversation as resolved.
Outdated
if (buffer->len != self->s_size) {
PyErr_Format(state->StructError,
Expand Down Expand Up @@ -1949,6 +1976,10 @@ Struct_unpack_from_impl(PyStructObject *self, Py_buffer *buffer,
/*[clinic end generated code: output=57fac875e0977316 input=cafd4851d473c894]*/
{
_structmodulestate *state = get_struct_state_structinst(self);
if (!self->ready) {
Comment thread
skirpichev marked this conversation as resolved.
Outdated
PyErr_SetString(PyExc_RuntimeError, "Call unpack_from on non-initialized Struct()");
return NULL;
}
assert(self->s_codes != NULL);

if (offset < 0) {
Expand Down Expand Up @@ -2101,6 +2132,10 @@ Struct_iter_unpack_impl(PyStructObject *self, PyObject *buffer)
{
_structmodulestate *state = get_struct_state_structinst(self);
unpackiterobject *iter;
if (!self->ready) {
PyErr_SetString(PyExc_RuntimeError, "Call iter_unpack on non-initialized Struct()");
return NULL;
}

assert(self->s_codes != NULL);

Expand Down Expand Up @@ -2139,7 +2174,7 @@ Struct_iter_unpack_impl(PyStructObject *self, PyObject *buffer)
* argument for where to start processing the arguments for packing, and a
* character buffer for writing the packed string. The caller must insure
* that the buffer may contain the required length for packing the arguments.
* 0 is returned on success, 1 is returned if there is an error.
* 0 is returned on success, -1 is returned if there is an error.
*
*/
static int
Expand Down Expand Up @@ -2242,6 +2277,10 @@ s_pack(PyObject *self, PyObject *const *args, Py_ssize_t nargs)

/* Validate arguments. */
soself = PyStructObject_CAST(self);
if (!soself->ready) {
PyErr_SetString(PyExc_RuntimeError, "Call pack on non-initialized Struct()");
return NULL;
}
assert(PyStruct_Check(self, state));
assert(soself->s_codes != NULL);
if (nargs != soself->s_len)
Expand All @@ -2259,10 +2298,13 @@ s_pack(PyObject *self, PyObject *const *args, Py_ssize_t nargs)
char *buf = PyBytesWriter_GetData(writer);

/* Call the guts */
PyMutex_Lock(&soself->mutex);
if ( s_pack_internal(soself, args, 0, buf, state) != 0 ) {
PyMutex_Unlock(&soself->mutex);
PyBytesWriter_Discard(writer);
return NULL;
}
PyMutex_Unlock(&soself->mutex);

return PyBytesWriter_FinishWithSize(writer, soself->s_size);
}
Expand All @@ -2285,6 +2327,10 @@ s_pack_into(PyObject *self, PyObject *const *args, Py_ssize_t nargs)

/* Validate arguments. +1 is for the first arg as buffer. */
soself = PyStructObject_CAST(self);
if (!soself->ready) {
PyErr_SetString(PyExc_RuntimeError, "Call pack_into on non-initialized Struct()");
return NULL;
}
assert(PyStruct_Check(self, state));
assert(soself->s_codes != NULL);
if (nargs != (soself->s_len + 2))
Expand Down Expand Up @@ -2360,11 +2406,13 @@ s_pack_into(PyObject *self, PyObject *const *args, Py_ssize_t nargs)
}

/* Call the guts */
PyMutex_Lock(&soself->mutex);
if (s_pack_internal(soself, args, 2, (char*)buffer.buf + offset, state) != 0) {
PyMutex_Unlock(&soself->mutex);
PyBuffer_Release(&buffer);
return NULL;
}

PyMutex_Unlock(&soself->mutex);
PyBuffer_Release(&buffer);
Py_RETURN_NONE;
}
Expand Down
Loading