From a1c26039dce3faf7a73db97ac15d060cdc5666c2 Mon Sep 17 00:00:00 2001 From: Sergey B Kirpichev Date: Sat, 3 Jan 2026 10:23:37 +0300 Subject: [PATCH 1/4] gh-143379: fix UAF in struct.Struct.pack() when object modified by dunder methods --- Lib/test/test_struct.py | 11 ++++++ ...-01-03-10-15-32.gh-issue-143379.iz-hU7.rst | 4 +++ Modules/_struct.c | 34 ++++++++++++++----- 3 files changed, 41 insertions(+), 8 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2026-01-03-10-15-32.gh-issue-143379.iz-hU7.rst diff --git a/Lib/test/test_struct.py b/Lib/test/test_struct.py index cceecdd526c006..cc7201e5431265 100644 --- a/Lib/test/test_struct.py +++ b/Lib/test/test_struct.py @@ -817,6 +817,17 @@ 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') + + class Evil(): + def __bool__(self): + # This rebuilds format codes during S.pack(). + S.__init__('I') + return True + + self.assertEqual(S.pack(Evil(), 1), struct.Struct('?I').pack(True, 1)) + class UnpackIteratorTest(unittest.TestCase): """ diff --git a/Misc/NEWS.d/next/Library/2026-01-03-10-15-32.gh-issue-143379.iz-hU7.rst b/Misc/NEWS.d/next/Library/2026-01-03-10-15-32.gh-issue-143379.iz-hU7.rst new file mode 100644 index 00000000000000..17eed480ebf4cc --- /dev/null +++ b/Misc/NEWS.d/next/Library/2026-01-03-10-15-32.gh-issue-143379.iz-hU7.rst @@ -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. Patch by Sergey B +Kirpichev. diff --git a/Modules/_struct.c b/Modules/_struct.c index 2acb3df3a30395..ba55de453f62ea 100644 --- a/Modules/_struct.c +++ b/Modules/_struct.c @@ -2139,21 +2139,31 @@ 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 s_pack_internal(PyStructObject *soself, PyObject *const *args, int offset, char* buf, _structmodulestate *state) { - formatcode *code; + formatcode *code, *frozen_codes; /* XXX(nnorwitz): why does i need to be a local? can we use the offset parameter or do we need the wider width? */ - Py_ssize_t i; + Py_ssize_t i, frozen_size = 1; + /* Take copy of soself->s_codes, as dunder methods of arguments (e.g. + __bool__ of __float__) could modify the struct object during packing. */ + for (code = soself->s_codes; code->fmtdef != NULL; code++) { + frozen_size++; + } + frozen_codes = PyMem_Malloc(frozen_size * sizeof(formatcode)); + if (!frozen_codes) { + goto err; + } + memcpy(frozen_codes, soself->s_codes, frozen_size * sizeof(formatcode)); memset(buf, '\0', soself->s_size); i = offset; - for (code = soself->s_codes; code->fmtdef != NULL; code++) { + for (code = frozen_codes; code->fmtdef != NULL; code++) { const formatdef *e = code->fmtdef; char *res = buf + code->offset; Py_ssize_t j = code->repeat; @@ -2167,7 +2177,7 @@ s_pack_internal(PyStructObject *soself, PyObject *const *args, int offset, if (!isstring && !PyByteArray_Check(v)) { PyErr_SetString(state->StructError, "argument for 's' must be a bytes object"); - return -1; + goto err; } if (isstring) { n = PyBytes_GET_SIZE(v); @@ -2189,7 +2199,7 @@ s_pack_internal(PyStructObject *soself, PyObject *const *args, int offset, if (!isstring && !PyByteArray_Check(v)) { PyErr_SetString(state->StructError, "argument for 'p' must be a bytes object"); - return -1; + goto err; } if (isstring) { n = PyBytes_GET_SIZE(v); @@ -2215,7 +2225,7 @@ s_pack_internal(PyStructObject *soself, PyObject *const *args, int offset, if (PyLong_Check(v) && PyErr_ExceptionMatches(PyExc_OverflowError)) PyErr_SetString(state->StructError, "int too large to convert"); - return -1; + goto err; } } res += code->size; @@ -2223,7 +2233,12 @@ s_pack_internal(PyStructObject *soself, PyObject *const *args, int offset, } /* Success */ + PyMem_Free(frozen_codes); return 0; +err: + /* Failure */ + PyMem_Free(frozen_codes); + return -1; } @@ -2257,6 +2272,9 @@ s_pack(PyObject *self, PyObject *const *args, Py_ssize_t nargs) return NULL; } char *buf = PyBytesWriter_GetData(writer); + /* Take copy of soself->s_size, as dunder methods of arguments (e.g. + __bool__ of __float__) could modify the struct object during packing. */ + Py_ssize_t frozen_size = soself->s_size; /* Call the guts */ if ( s_pack_internal(soself, args, 0, buf, state) != 0 ) { @@ -2264,7 +2282,7 @@ s_pack(PyObject *self, PyObject *const *args, Py_ssize_t nargs) return NULL; } - return PyBytesWriter_FinishWithSize(writer, soself->s_size); + return PyBytesWriter_FinishWithSize(writer, frozen_size); } PyDoc_STRVAR(s_pack_into__doc__, From e611953812a84c985ee25bbd8b4d871430f60462 Mon Sep 17 00:00:00 2001 From: Sergey B Kirpichev Date: Sat, 10 Jan 2026 05:08:48 +0300 Subject: [PATCH 2/4] address review: implement a different solution with a mutex flag Now modification of the Struct() while packing trigger a RuntimeError --- Lib/test/test_struct.py | 6 ++- ...-01-03-10-15-32.gh-issue-143379.iz-hU7.rst | 4 +- Modules/_struct.c | 51 +++++++++---------- 3 files changed, 31 insertions(+), 30 deletions(-) diff --git a/Lib/test/test_struct.py b/Lib/test/test_struct.py index c2a9792dc2cb82..8811388f4ea143 100644 --- a/Lib/test/test_struct.py +++ b/Lib/test/test_struct.py @@ -579,7 +579,7 @@ def test_Struct_reinitialization(self): 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) @@ -818,6 +818,7 @@ def test_endian_table_init_subinterpreters(self): def test_Struct_object_mutation_via_dunders(self): S = struct.Struct('?I') + buf = array.array('b', b' '*100) class Evil(): def __bool__(self): @@ -825,7 +826,8 @@ def __bool__(self): S.__init__('I') return True - self.assertEqual(S.pack(Evil(), 1), struct.Struct('?I').pack(True, 1)) + self.assertRaises(RuntimeError, S.pack, Evil(), 1) + self.assertRaises(RuntimeError, S.pack_into, buf, 0, Evil(), 1) class UnpackIteratorTest(unittest.TestCase): diff --git a/Misc/NEWS.d/next/Library/2026-01-03-10-15-32.gh-issue-143379.iz-hU7.rst b/Misc/NEWS.d/next/Library/2026-01-03-10-15-32.gh-issue-143379.iz-hU7.rst index 17eed480ebf4cc..c2612ef27a558c 100644 --- a/Misc/NEWS.d/next/Library/2026-01-03-10-15-32.gh-issue-143379.iz-hU7.rst +++ b/Misc/NEWS.d/next/Library/2026-01-03-10-15-32.gh-issue-143379.iz-hU7.rst @@ -1,4 +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. Patch by Sergey B -Kirpichev. +:meth:`object.__bool__`) during packing of arguments. Now this trigger +:exc:`RuntimeError`. Patch by Sergey B Kirpichev. diff --git a/Modules/_struct.c b/Modules/_struct.c index ba55de453f62ea..d97f6d4a85835f 100644 --- a/Modules/_struct.c +++ b/Modules/_struct.c @@ -70,6 +70,7 @@ typedef struct { formatcode *s_codes; PyObject *s_format; PyObject *weakreflist; /* List of weak references */ + PyMutex mutex; /* to prevent mutation during packing */ } PyStructObject; #define PyStructObject_CAST(op) ((PyStructObject *)(op)) @@ -1773,6 +1774,7 @@ s_new(PyTypeObject *type, PyObject *args, PyObject *kwds) s->s_codes = NULL; s->s_size = -1; s->s_len = -1; + s->mutex = (PyMutex){0}; } return self; } @@ -1816,6 +1818,11 @@ Struct___init___impl(PyStructObject *self, PyObject *format) Py_SETREF(self->s_format, format); + if (PyMutex_IsLocked(&self->mutex)) { + PyErr_SetString(PyExc_RuntimeError, + "Call Struct.__init__() in struct.pack()"); + return -1; + } ret = prepare_s(self); return ret; } @@ -2146,24 +2153,14 @@ static int s_pack_internal(PyStructObject *soself, PyObject *const *args, int offset, char* buf, _structmodulestate *state) { - formatcode *code, *frozen_codes; + formatcode *code; /* XXX(nnorwitz): why does i need to be a local? can we use the offset parameter or do we need the wider width? */ - Py_ssize_t i, frozen_size = 1; + Py_ssize_t i; - /* Take copy of soself->s_codes, as dunder methods of arguments (e.g. - __bool__ of __float__) could modify the struct object during packing. */ - for (code = soself->s_codes; code->fmtdef != NULL; code++) { - frozen_size++; - } - frozen_codes = PyMem_Malloc(frozen_size * sizeof(formatcode)); - if (!frozen_codes) { - goto err; - } - memcpy(frozen_codes, soself->s_codes, frozen_size * sizeof(formatcode)); memset(buf, '\0', soself->s_size); i = offset; - for (code = frozen_codes; code->fmtdef != NULL; code++) { + for (code = soself->s_codes; code->fmtdef != NULL; code++) { const formatdef *e = code->fmtdef; char *res = buf + code->offset; Py_ssize_t j = code->repeat; @@ -2177,7 +2174,7 @@ s_pack_internal(PyStructObject *soself, PyObject *const *args, int offset, if (!isstring && !PyByteArray_Check(v)) { PyErr_SetString(state->StructError, "argument for 's' must be a bytes object"); - goto err; + return -1; } if (isstring) { n = PyBytes_GET_SIZE(v); @@ -2199,7 +2196,7 @@ s_pack_internal(PyStructObject *soself, PyObject *const *args, int offset, if (!isstring && !PyByteArray_Check(v)) { PyErr_SetString(state->StructError, "argument for 'p' must be a bytes object"); - goto err; + return -1; } if (isstring) { n = PyBytes_GET_SIZE(v); @@ -2225,7 +2222,7 @@ s_pack_internal(PyStructObject *soself, PyObject *const *args, int offset, if (PyLong_Check(v) && PyErr_ExceptionMatches(PyExc_OverflowError)) PyErr_SetString(state->StructError, "int too large to convert"); - goto err; + return -1; } } res += code->size; @@ -2233,12 +2230,7 @@ s_pack_internal(PyStructObject *soself, PyObject *const *args, int offset, } /* Success */ - PyMem_Free(frozen_codes); return 0; -err: - /* Failure */ - PyMem_Free(frozen_codes); - return -1; } @@ -2272,17 +2264,22 @@ s_pack(PyObject *self, PyObject *const *args, Py_ssize_t nargs) return NULL; } char *buf = PyBytesWriter_GetData(writer); - /* Take copy of soself->s_size, as dunder methods of arguments (e.g. - __bool__ of __float__) could modify the struct object during packing. */ - Py_ssize_t frozen_size = soself->s_size; /* Call the guts */ + if (PyMutex_IsLocked(&soself->mutex)) { + PyErr_SetString(PyExc_RuntimeError, "XXX"); + PyBytesWriter_Discard(writer); + return NULL; + } + 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, frozen_size); + return PyBytesWriter_FinishWithSize(writer, soself->s_size); } PyDoc_STRVAR(s_pack_into__doc__, @@ -2378,11 +2375,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; } From ab6216773a1e530ae1a770101519690855b23008 Mon Sep 17 00:00:00 2001 From: Sergey B Kirpichev Date: Sat, 10 Jan 2026 05:45:08 +0300 Subject: [PATCH 3/4] + clean the mess --- Modules/_struct.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/Modules/_struct.c b/Modules/_struct.c index d97f6d4a85835f..eeee5fd68d87a9 100644 --- a/Modules/_struct.c +++ b/Modules/_struct.c @@ -2266,11 +2266,6 @@ s_pack(PyObject *self, PyObject *const *args, Py_ssize_t nargs) char *buf = PyBytesWriter_GetData(writer); /* Call the guts */ - if (PyMutex_IsLocked(&soself->mutex)) { - PyErr_SetString(PyExc_RuntimeError, "XXX"); - PyBytesWriter_Discard(writer); - return NULL; - } PyMutex_Lock(&soself->mutex); if ( s_pack_internal(soself, args, 0, buf, state) != 0 ) { PyMutex_Unlock(&soself->mutex); From 60ab4b728ef8796321cd13325a75c4fde5780c19 Mon Sep 17 00:00:00 2001 From: Sergey B Kirpichev Date: Sun, 11 Jan 2026 03:27:56 +0300 Subject: [PATCH 4/4] address review: use counter to allow concurent packing --- .../internal/pycore_pyatomic_ft_wrappers.h | 3 +++ Lib/test/test_struct.py | 2 +- Modules/_struct.c | 23 +++++++++++-------- 3 files changed, 18 insertions(+), 10 deletions(-) diff --git a/Include/internal/pycore_pyatomic_ft_wrappers.h b/Include/internal/pycore_pyatomic_ft_wrappers.h index 70a32db663b293..1def72fff3a010 100644 --- a/Include/internal/pycore_pyatomic_ft_wrappers.h +++ b/Include/internal/pycore_pyatomic_ft_wrappers.h @@ -107,6 +107,8 @@ extern "C" { _Py_atomic_store_ulong_relaxed(&value, new_value) #define FT_ATOMIC_STORE_SSIZE_RELAXED(value, new_value) \ _Py_atomic_store_ssize_relaxed(&value, new_value) +#define FT_ATOMIC_STORE_SSIZE(value, new_value) \ + _Py_atomic_store_ssize(&value, new_value) #define FT_ATOMIC_STORE_FLOAT_RELAXED(value, new_value) \ _Py_atomic_store_float_relaxed(&value, new_value) #define FT_ATOMIC_LOAD_FLOAT_RELAXED(value) \ @@ -151,6 +153,7 @@ extern "C" { #define FT_ATOMIC_STORE_INT8_RELAXED(value, new_value) value = new_value #define FT_ATOMIC_STORE_INT8_RELEASE(value, new_value) value = new_value #define FT_ATOMIC_STORE_SSIZE_RELAXED(value, new_value) value = new_value +#define FT_ATOMIC_STORE_SSIZE(value, new_value) value = new_value #define FT_ATOMIC_STORE_SSIZE_RELEASE(value, new_value) value = new_value #define FT_ATOMIC_STORE_UINT8_RELAXED(value, new_value) value = new_value #define FT_ATOMIC_STORE_UINT16_RELAXED(value, new_value) value = new_value diff --git a/Lib/test/test_struct.py b/Lib/test/test_struct.py index 8811388f4ea143..a73174993dc6f8 100644 --- a/Lib/test/test_struct.py +++ b/Lib/test/test_struct.py @@ -579,7 +579,7 @@ def test_Struct_reinitialization(self): def check_sizeof(self, format_str, number_of_codes): # The size of 'PyStructObject' - totalsize = support.calcobjsize('2n3PB') + totalsize = support.calcobjsize('2n3P1n') # 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) diff --git a/Modules/_struct.c b/Modules/_struct.c index eeee5fd68d87a9..19208e9a542a4c 100644 --- a/Modules/_struct.c +++ b/Modules/_struct.c @@ -70,7 +70,7 @@ typedef struct { formatcode *s_codes; PyObject *s_format; PyObject *weakreflist; /* List of weak references */ - PyMutex mutex; /* to prevent mutation during packing */ + Py_ssize_t mutex_cnt; /* to prevent mutation during packing */ } PyStructObject; #define PyStructObject_CAST(op) ((PyStructObject *)(op)) @@ -1774,7 +1774,7 @@ 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->mutex_cnt = 0; } return self; } @@ -1818,7 +1818,7 @@ Struct___init___impl(PyStructObject *self, PyObject *format) Py_SETREF(self->s_format, format); - if (PyMutex_IsLocked(&self->mutex)) { + if (FT_ATOMIC_LOAD_SSIZE(self->mutex_cnt)) { PyErr_SetString(PyExc_RuntimeError, "Call Struct.__init__() in struct.pack()"); return -1; @@ -2266,13 +2266,15 @@ s_pack(PyObject *self, PyObject *const *args, Py_ssize_t nargs) char *buf = PyBytesWriter_GetData(writer); /* Call the guts */ - PyMutex_Lock(&soself->mutex); + Py_ssize_t prev_cnt = FT_ATOMIC_LOAD_SSIZE(soself->mutex_cnt); + + FT_ATOMIC_ADD_SSIZE(soself->mutex_cnt, 1); if ( s_pack_internal(soself, args, 0, buf, state) != 0 ) { - PyMutex_Unlock(&soself->mutex); + FT_ATOMIC_STORE_SSIZE(soself->mutex_cnt, prev_cnt); PyBytesWriter_Discard(writer); return NULL; } - PyMutex_Unlock(&soself->mutex); + FT_ATOMIC_STORE_SSIZE(soself->mutex_cnt, prev_cnt); return PyBytesWriter_FinishWithSize(writer, soself->s_size); } @@ -2370,13 +2372,16 @@ s_pack_into(PyObject *self, PyObject *const *args, Py_ssize_t nargs) } /* Call the guts */ - PyMutex_Lock(&soself->mutex); + Py_ssize_t prev_cnt = FT_ATOMIC_LOAD_SSIZE(soself->mutex_cnt); + + FT_ATOMIC_ADD_SSIZE(soself->mutex_cnt, 1); if (s_pack_internal(soself, args, 2, (char*)buffer.buf + offset, state) != 0) { - PyMutex_Unlock(&soself->mutex); + FT_ATOMIC_STORE_SSIZE(soself->mutex_cnt, prev_cnt); PyBuffer_Release(&buffer); return NULL; } - PyMutex_Unlock(&soself->mutex); + FT_ATOMIC_STORE_SSIZE(soself->mutex_cnt, prev_cnt); + PyBuffer_Release(&buffer); Py_RETURN_NONE; }