Skip to content

Commit a1c2603

Browse files
committed
gh-143379: fix UAF in struct.Struct.pack() when object modified by dunder methods
1 parent b538c28 commit a1c2603

File tree

3 files changed

+41
-8
lines changed

3 files changed

+41
-8
lines changed

Lib/test/test_struct.py

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -817,6 +817,17 @@ def test_endian_table_init_subinterpreters(self):
817817
results = executor.map(exec, [code] * 5)
818818
self.assertListEqual(list(results), [None] * 5)
819819

820+
def test_Struct_object_mutation_via_dunders(self):
821+
S = struct.Struct('?I')
822+
823+
class Evil():
824+
def __bool__(self):
825+
# This rebuilds format codes during S.pack().
826+
S.__init__('I')
827+
return True
828+
829+
self.assertEqual(S.pack(Evil(), 1), struct.Struct('?I').pack(True, 1))
830+
820831

821832
class UnpackIteratorTest(unittest.TestCase):
822833
"""
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
Fix use-after-free in :meth:`struct.Struct.pack` when the
2+
:class:`struct.Struct` object is mutated by dunder methods (like
3+
:meth:`object.__bool__`) during packing of arguments. Patch by Sergey B
4+
Kirpichev.

Modules/_struct.c

Lines changed: 26 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2139,21 +2139,31 @@ Struct_iter_unpack_impl(PyStructObject *self, PyObject *buffer)
21392139
* argument for where to start processing the arguments for packing, and a
21402140
* character buffer for writing the packed string. The caller must insure
21412141
* that the buffer may contain the required length for packing the arguments.
2142-
* 0 is returned on success, 1 is returned if there is an error.
2142+
* 0 is returned on success, -1 is returned if there is an error.
21432143
*
21442144
*/
21452145
static int
21462146
s_pack_internal(PyStructObject *soself, PyObject *const *args, int offset,
21472147
char* buf, _structmodulestate *state)
21482148
{
2149-
formatcode *code;
2149+
formatcode *code, *frozen_codes;
21502150
/* XXX(nnorwitz): why does i need to be a local? can we use
21512151
the offset parameter or do we need the wider width? */
2152-
Py_ssize_t i;
2152+
Py_ssize_t i, frozen_size = 1;
21532153

2154+
/* Take copy of soself->s_codes, as dunder methods of arguments (e.g.
2155+
__bool__ of __float__) could modify the struct object during packing. */
2156+
for (code = soself->s_codes; code->fmtdef != NULL; code++) {
2157+
frozen_size++;
2158+
}
2159+
frozen_codes = PyMem_Malloc(frozen_size * sizeof(formatcode));
2160+
if (!frozen_codes) {
2161+
goto err;
2162+
}
2163+
memcpy(frozen_codes, soself->s_codes, frozen_size * sizeof(formatcode));
21542164
memset(buf, '\0', soself->s_size);
21552165
i = offset;
2156-
for (code = soself->s_codes; code->fmtdef != NULL; code++) {
2166+
for (code = frozen_codes; code->fmtdef != NULL; code++) {
21572167
const formatdef *e = code->fmtdef;
21582168
char *res = buf + code->offset;
21592169
Py_ssize_t j = code->repeat;
@@ -2167,7 +2177,7 @@ s_pack_internal(PyStructObject *soself, PyObject *const *args, int offset,
21672177
if (!isstring && !PyByteArray_Check(v)) {
21682178
PyErr_SetString(state->StructError,
21692179
"argument for 's' must be a bytes object");
2170-
return -1;
2180+
goto err;
21712181
}
21722182
if (isstring) {
21732183
n = PyBytes_GET_SIZE(v);
@@ -2189,7 +2199,7 @@ s_pack_internal(PyStructObject *soself, PyObject *const *args, int offset,
21892199
if (!isstring && !PyByteArray_Check(v)) {
21902200
PyErr_SetString(state->StructError,
21912201
"argument for 'p' must be a bytes object");
2192-
return -1;
2202+
goto err;
21932203
}
21942204
if (isstring) {
21952205
n = PyBytes_GET_SIZE(v);
@@ -2215,15 +2225,20 @@ s_pack_internal(PyStructObject *soself, PyObject *const *args, int offset,
22152225
if (PyLong_Check(v) && PyErr_ExceptionMatches(PyExc_OverflowError))
22162226
PyErr_SetString(state->StructError,
22172227
"int too large to convert");
2218-
return -1;
2228+
goto err;
22192229
}
22202230
}
22212231
res += code->size;
22222232
}
22232233
}
22242234

22252235
/* Success */
2236+
PyMem_Free(frozen_codes);
22262237
return 0;
2238+
err:
2239+
/* Failure */
2240+
PyMem_Free(frozen_codes);
2241+
return -1;
22272242
}
22282243

22292244

@@ -2257,14 +2272,17 @@ s_pack(PyObject *self, PyObject *const *args, Py_ssize_t nargs)
22572272
return NULL;
22582273
}
22592274
char *buf = PyBytesWriter_GetData(writer);
2275+
/* Take copy of soself->s_size, as dunder methods of arguments (e.g.
2276+
__bool__ of __float__) could modify the struct object during packing. */
2277+
Py_ssize_t frozen_size = soself->s_size;
22602278

22612279
/* Call the guts */
22622280
if ( s_pack_internal(soself, args, 0, buf, state) != 0 ) {
22632281
PyBytesWriter_Discard(writer);
22642282
return NULL;
22652283
}
22662284

2267-
return PyBytesWriter_FinishWithSize(writer, soself->s_size);
2285+
return PyBytesWriter_FinishWithSize(writer, frozen_size);
22682286
}
22692287

22702288
PyDoc_STRVAR(s_pack_into__doc__,

0 commit comments

Comments
 (0)