From b3dea86b74fba41bdf21da08c68cecc55d66f21b Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Sun, 11 Oct 2026 20:04:20 +0000 Subject: [PATCH] gh-159184: Make the marshal order of set elements with equal dumps stable Set elements with the same dump (NaNs which are distinct objects) were written in set iteration order, which depends on their addresses. When one of them is also referenced elsewhere in the dump, the position of the reference among them changed from run to run. Elements with equal dumps are now ordered by when they were first written in this dump; floats and complex numbers not written yet are interchangeable, and which one is which is decided when one of them is referenced again. This relies on the writer knowing exactly which objects it has written (gh-129724). The elements are sorted with qsort() on the bytes of their dumps, in an array instead of a list of tuples. --- Lib/test/test_marshal.py | 60 +++- ...-10-11-08-42-00.gh-issue-159184.Tq2nSe.rst | 3 + Python/marshal.c | 340 ++++++++++++++---- 3 files changed, 324 insertions(+), 79 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2026-10-11-08-42-00.gh-issue-159184.Tq2nSe.rst diff --git a/Lib/test/test_marshal.py b/Lib/test/test_marshal.py index a9b25c6082b7bf..cc8c19366ce241 100644 --- a/Lib/test/test_marshal.py +++ b/Lib/test/test_marshal.py @@ -617,7 +617,6 @@ def test_unorderable_set_items(self): self.assertEqual(sorted(map(repr, new)), sorted(map(repr, sample))) - def test_unorderable_set_items_in_code(self): # A constant set containing NaNs which are distinct objects. # Run in a subprocess: constants of code objects are immortal @@ -662,7 +661,6 @@ def shrink(sample): "set changed size"): marshal.dumps(sample, version) - def test_error_in_buffer(self): # The BufferError raised for a non-contiguous buffer is not replaced # with a generic error. @@ -1238,7 +1236,8 @@ def test_nan_in_set(self): nan1 = float('nan') nan2 = float('nan') nan3 = float('nan') - s = kind([nan1, 1.5, nan2, nan3]) + s = kind([nan1, 1.5, nan2, nan3, + complex('nan'), complex('nan')]) for sample in (s, [s], [nan1, s], [s, nan2], [nan3, s, nan1], [nan1, nan2, nan3, s], [s, s], (nan2, [s, nan2], s)): @@ -1247,6 +1246,59 @@ def test_nan_in_set(self): new = marshal.loads(data) self.check_same_structure(new, sample, version >= 3) + @support.cpython_only + def test_nan_in_set_order(self): + # The output does not depend on the order of the elements of a set + # which have the same marshalled representation, even if some of + # them are also referenced elsewhere. + version = self.VERSION + def flagged(data): + data = bytearray(data) + data[0] |= self.FLAG_REF + return bytes(data) + def ref(index): + return b'r' + struct.pack(''): + head = typecode + b'\4\0\0\0' + data15 + for i in range(20): + # Different objects at different addresses every time. + keep.append(bytes(i * 8)) + nans = [make_nan() for j in range(3)] + keep.append(nans) + s = kind([1.5, *nans]) + a, b, c = nans[i % 3:] + nans[:i % 3] + for sample, expected in [ + (s, head + data * 3), + ([s, a], + b'[\2\0\0\0' + head + fdata + data * 2 + ref(0)), + ([a, s], + b'[\2\0\0\0' + fdata + head + ref(0) + data * 2), + ([s, a, b, a], + b'[\4\0\0\0' + head + fdata * 2 + data + + ref(0) + ref(1) + ref(0)), + ([a, b, s, c], + b'[\4\0\0\0' + fdata * 2 + head + ref(0) + ref(1) + + flagged(data) + ref(2)), + ([b, s, c, a, kind([a, b, c])], + b'[\5\0\0\0' + fdata + head + ref(0) + fdata * 2 + + ref(1) + ref(2) + typecode + b'\3\0\0\0' + + ref(0) + ref(1) + ref(2)), + ([s, kind([b, a]), kind([c, make_nan()]), b, a, c], + b'[\6\0\0\0' + head + fdata * 3 + + typecode + b'\2\0\0\0' + ref(0) + ref(1) + + typecode + b'\2\0\0\0' + ref(2) + data + + ref(0) + ref(1) + ref(2)), + ]: + self.assertEqual(marshal.dumps(sample, version), + expected) + new = marshal.loads(expected) + self.check_same_structure(new, sample, True) + def check_same_structure(self, new, sample, shared): # Both object graphs have the same values and, if shared is true, # share the same objects. Elements of sets are matched in any @@ -1261,7 +1313,7 @@ def known(x): if shared: return seen.get(id(x), -1) def visit(x): - if isinstance(x, float): + if isinstance(x, (float, complex)): result.append((repr(x), number(x))) elif isinstance(x, (set, frozenset)): # Elements are only numbered if they occur elsewhere. diff --git a/Misc/NEWS.d/next/Library/2026-10-11-08-42-00.gh-issue-159184.Tq2nSe.rst b/Misc/NEWS.d/next/Library/2026-10-11-08-42-00.gh-issue-159184.Tq2nSe.rst new file mode 100644 index 00000000000000..240738671ffaaf --- /dev/null +++ b/Misc/NEWS.d/next/Library/2026-10-11-08-42-00.gh-issue-159184.Tq2nSe.rst @@ -0,0 +1,3 @@ +The output of :func:`marshal.dumps` no longer depends on the order of the +elements of a set which have the same marshalled form, such as NaNs which +are distinct objects. diff --git a/Python/marshal.c b/Python/marshal.c index 7109e5c56f4036..737246b4193222 100644 --- a/Python/marshal.c +++ b/Python/marshal.c @@ -14,7 +14,6 @@ #include "pycore_object.h" // _PyObject_IsUniquelyReferenced #include "pycore_pystate.h" // _PyInterpreterState_GET() #include "pycore_setobject.h" // _PySet_NextEntryRef() -#include "pycore_tuple.h" // _PyTuple_FromPairSteal #include "pycore_unicodeobject.h" // _PyUnicode_InternImmortal() #include "marshal.h" // Py_MARSHAL_VERSION @@ -107,14 +106,18 @@ module marshal by W_ENTRY_SHIFT bits, plus the flags: - W_ENTRY_ACTIVE: the object is being written (used to detect recursion, see w_complete()); - - W_ENTRY_SHARED: the object has been written more than once. + - W_ENTRY_SHARED: the object has been written more than once; + - W_ENTRY_TIED: the object is a set element which cannot be told from + other elements of the set yet. The value is then an index in + WFILE.ties instead, see w_tie_set_items(). The item of WFILE.refs is the offset of the type byte of the object in the output, shifted left by one bit, plus W_REF_SHARED if the object has been written more than once. */ #define W_ENTRY_ACTIVE ((uintptr_t)1) #define W_ENTRY_SHARED ((uintptr_t)2) -#define W_ENTRY_SHIFT 2 +#define W_ENTRY_TIED ((uintptr_t)4) +#define W_ENTRY_SHIFT 3 #define W_REF_SHARED ((size_t)1) /* Maximum number of items in WFILE.refs. TYPE_REF has a signed 32-bit @@ -139,6 +142,9 @@ typedef struct { the current run, or 0. */ size_t refpos_end; /* Offset of the end of the last TYPE_REF in the output. */ + size_t *ties; /* See w_tie_set_items(). */ + size_t nties; + size_t ties_allocated; int version; int allow_code; } WFILE; @@ -521,6 +527,12 @@ w_ref(PyObject *v, WFILE *p) PyErr_Format(PyExc_ValueError, "cannot marshal recursion %T objects", v); goto err; } + if (w & W_ENTRY_TIED) { + /* Take the first object which is still available in the group + of interchangeable set elements, see w_tie_set_items(). */ + size_t i = p->ties[w >> W_ENTRY_SHIFT]++; + w = (uintptr_t)i << W_ENTRY_SHIFT; + } if (!(w & W_ENTRY_SHARED)) { entry->value = (void *)(w | W_ENTRY_SHARED); p->refs[w >> W_ENTRY_SHIFT] |= W_REF_SHARED; @@ -653,6 +665,254 @@ w_complete(PyObject *v, WFILE *p) static void w_complex_object(PyObject *v, WFILE *p); +static void +w_object(PyObject *v, WFILE *p); + +/* An element of a set which is being written. */ +typedef struct { + PyObject *value; + PyObject *dump; /* The element marshalled on its own. */ + size_t order; /* Order among the elements with the same dump. */ +} w_set_item; + +/* Minimal order of an element which has not been written yet. */ +#define W_SET_ITEM_NEW ((size_t)1 << (8 * SIZEOF_SIZE_T - 1)) + +static int +w_set_item_compare(const void *left, const void *right) +{ + const w_set_item *a = (const w_set_item *)left; + const w_set_item *b = (const w_set_item *)right; + Py_ssize_t asize = PyBytes_GET_SIZE(a->dump); + Py_ssize_t bsize = PyBytes_GET_SIZE(b->dump); + int c = memcmp(PyBytes_AS_STRING(a->dump), PyBytes_AS_STRING(b->dump), + Py_MIN(asize, bsize)); + if (c != 0) { + return c; + } + if (asize != bsize) { + return asize < bsize ? -1 : 1; + } + if (a->order != b->order) { + return a->order < b->order ? -1 : 1; + } + return 0; +} + +/* The count elements of a set starting at items are floats or complex + numbers which have the same dump and have just been written, as the + objects number first to first + count - 1 in p->refs or as references to + them. Since the same bytes would have been written for these elements + in any order, it is not decided yet which element is which of these + objects. The elements which are written again later take these objects + one by one, in the order in which that happens (see w_ref()), so that the + output does not depend on the order of the elements in the set. + + The entries of these elements in p->hashtable get W_ENTRY_TIED and the + index of a new item of p->ties, which is the number of the first of + these objects that has not been taken yet. */ +static void +w_tie_set_items(w_set_item *items, Py_ssize_t count, size_t first, WFILE *p) +{ + size_t *ties = w_array_grow(p->ties, p->nties, &p->ties_allocated, + sizeof(*ties)); + if (ties == NULL) { + PyErr_NoMemory(); + p->error = true; + return; + } + p->ties = ties; + ties[p->nties] = first; + uintptr_t value = ((uintptr_t)p->nties << W_ENTRY_SHIFT) | W_ENTRY_TIED; + p->nties++; + for (Py_ssize_t i = 0; i < count; i++) { + _Py_hashtable_entry_t *entry = _Py_hashtable_get_entry( + p->hashtable, items[i].value); + assert(entry != NULL); + assert(((uintptr_t)entry->value >> W_ENTRY_SHIFT) - first + < (size_t)count); + assert(!((uintptr_t)entry->value & (W_ENTRY_ACTIVE | W_ENTRY_TIED))); + entry->value = (void *)value; + } +} + +/* Return true if two elements of a set have the same dump. */ +static bool +w_set_item_same_dump(const w_set_item *a, const w_set_item *b) +{ + Py_ssize_t size = PyBytes_GET_SIZE(a->dump); + return (PyBytes_GET_SIZE(b->dump) == size + && memcmp(PyBytes_AS_STRING(a->dump), + PyBytes_AS_STRING(b->dump), size) == 0); +} + +/* Write the elements of a set which have the same dump: the count + elements starting at items. + + Distinct elements have the same dump if they contain NaNs which are + distinct objects. The order of these elements matters if they are also + written elsewhere in the output, since references to them or to the + objects they contain are then written. That order is made independent + of the order of the elements in the set (which depends on the addresses + of the objects) as follows: + + - the elements which have already been written before come first, in + the order in which they were written; + - floats and complex numbers which cannot be told from each other by + what has been written so far are kept interchangeable: which of them + are written again later does not change the output, see + w_tie_set_items(); + - the other ones (containers, for example tuples or frozensets of NaNs) + are written in the order of the set. This is the only case in which + the output still depends on that order, and only if these elements or + the objects they contain are also written elsewhere in the output: + the element which is written first gets the lowest reference indexes + for the objects it contains, or the first objects of the group if + these objects are interchangeable. + */ +static void +w_same_set_items(w_set_item *items, Py_ssize_t count, WFILE *p) +{ + if (p->version < 3 || p->hashtable == NULL) { + /* Not writing object references. */ + for (Py_ssize_t i = 0; i < count; i++) { + w_object(items[i].value, p); + } + return; + } + + for (Py_ssize_t i = 0; i < count; i++) { + _Py_hashtable_entry_t *entry = _Py_hashtable_get_entry( + p->hashtable, items[i].value); + if (entry == NULL) { + items[i].order = W_SET_ITEM_NEW + (size_t)i; + continue; + } + uintptr_t w = (uintptr_t)entry->value; + items[i].order = w >> W_ENTRY_SHIFT; + if (w & W_ENTRY_TIED) { + /* The first object that the element can take. It is the + same for all the elements it is interchangeable with. */ + items[i].order = p->ties[items[i].order]; + } + } + qsort(items, count, sizeof(w_set_item), w_set_item_compare); + + Py_ssize_t start = 0; + while (start < count && !p->error) { + /* Write together the interchangeable elements: floats and complex + numbers which either have not been written yet, or are already + interchangeable (they are the only elements already written + which have the same order). */ + w_set_item *item = &items[start]; + Py_ssize_t stop = start + 1; + /* The first object written or referenced below. */ + size_t first = item->order; + if (first >= W_SET_ITEM_NEW) { + first = p->hashtable->nentries; + if (PyFloat_CheckExact(item->value) + || PyComplex_CheckExact(item->value)) + { + stop = count; + } + } + else { + while (stop < count && items[stop].order == item->order) { + stop++; + } + } + for (Py_ssize_t i = start; i < stop; i++) { + w_object(items[i].value, p); + } + if (stop - start > 1 && !p->error) { + w_tie_set_items(item, stop - start, first, p); + } + start = stop; + } +} + +/* Write the n elements of a set or a frozenset. + + bpo-37596: To support reproducible builds, the elements need to be + written in a consistent order, even when they have been scrambled by hash + randomization. They are written in the order of sorted(v, + key=marshal.dumps). The elements themselves are never compared: they + may not support it. See w_same_set_items() for the elements which have + the same dump. + + Marshalling an element can run arbitrary code (an audit hook, or the + __buffer__() method of the element), which can add elements to the set + or remove some: n is not trusted to still be the size of the set. + + This function is not inlined, to keep its code and its local variables + out of w_complex_object(), which is recursive and mostly used for other + types. + */ +static Py_NO_INLINE void +w_set_items(PyObject *v, Py_ssize_t n, WFILE *p) +{ + w_set_item *items = PyMem_New(w_set_item, n ? n : 1); + if (items == NULL) { + PyErr_NoMemory(); + p->error = true; + return; + } + PyObject *value; + Py_ssize_t pos = 0; + Py_hash_t hash; + Py_ssize_t i = 0; + bool changed = false; + Py_BEGIN_CRITICAL_SECTION(v); + while (_PySet_NextEntryRef(v, &pos, &value, &hash)) { + if (i == n) { + changed = true; + Py_DECREF(value); + break; + } + PyObject *dump = _PyMarshal_WriteObjectToString(value, + p->version, p->allow_code); + if (dump == NULL) { + p->error = true; + Py_DECREF(value); + break; + } + items[i].value = value; + items[i].dump = dump; + items[i].order = (size_t)i; + i++; + } + Py_END_CRITICAL_SECTION(); + if (!p->error && (changed || i != n)) { + PyErr_SetString(PyExc_RuntimeError, + "set changed size during iteration"); + p->error = true; + } + if (!p->error) { + qsort(items, n, sizeof(w_set_item), w_set_item_compare); + Py_ssize_t start = 0; + while (start < n && !p->error) { + Py_ssize_t stop = start + 1; + while (stop < n && w_set_item_same_dump(&items[start], + &items[stop])) + { + stop++; + } + if (stop - start == 1) { + w_object(items[start].value, p); + } + else { + w_same_set_items(items + start, stop - start, p); + } + start = stop; + } + } + while (--i >= 0) { + Py_DECREF(items[i].value); + Py_DECREF(items[i].dump); + } + PyMem_Free(items); +} + static void w_object(PyObject *v, WFILE *p) { @@ -832,84 +1092,13 @@ w_complex_object(PyObject *v, WFILE *p) } } else if (PyAnySet_CheckExact(v)) { - PyObject *value; - Py_ssize_t pos = 0; - Py_hash_t hash; - if (PyFrozenSet_CheckExact(v)) W_TYPE(TYPE_FROZENSET, p); else W_TYPE(TYPE_SET, p); n = PySet_GET_SIZE(v); W_SIZE(n, p); - // bpo-37596: To support reproducible builds, sets and frozensets need - // to have their elements serialized in a consistent order (even when - // they have been scrambled by hash randomization). To ensure this, we - // use an order equivalent to sorted(v, key=marshal.dumps). - // The list has a (dump, index, element) tuple for each element. - // The indexes are all different, so that sorting never compares - // two elements: elements with the same dump (NaNs which are - // distinct objects) may not be orderable. - PyObject *items = PyList_New(n); - if (items == NULL) { - p->error = true; - return; - } - // Marshalling an element can run arbitrary code (an audit hook, or - // the __buffer__() method of the element), which can add elements - // to the set or remove some: n is not trusted to still be the size - // of the set. - Py_ssize_t i = 0; - bool changed = false; - Py_BEGIN_CRITICAL_SECTION(v); - while (_PySet_NextEntryRef(v, &pos, &value, &hash)) { - if (i == n) { - changed = true; - Py_DECREF(value); - break; - } - PyObject *dump = _PyMarshal_WriteObjectToString(value, - p->version, p->allow_code); - if (dump == NULL) { - p->error = true; - Py_DECREF(value); - break; - } - PyObject *index = PyLong_FromSsize_t(i); - PyObject *item = NULL; - if (index != NULL) { - item = PyTuple_Pack(3, dump, index, value); - Py_DECREF(index); - } - Py_DECREF(dump); - Py_DECREF(value); - if (item == NULL) { - p->error = true; - break; - } - PyList_SET_ITEM(items, i++, item); - } - Py_END_CRITICAL_SECTION(); - if (!p->error && (changed || i != n)) { - PyErr_SetString(PyExc_RuntimeError, - "set changed size during iteration"); - p->error = true; - } - if (p->error) { - Py_DECREF(items); - return; - } - if (PyList_Sort(items)) { - p->error = true; - Py_DECREF(items); - return; - } - for (Py_ssize_t i = 0; i < n; i++) { - PyObject *item = PyList_GET_ITEM(items, i); - value = PyTuple_GET_ITEM(item, 2); - w_object(value, p); - } - Py_DECREF(items); + w_set_items(v, n, p); } else if (PyCode_Check(v)) { if (!p->allow_code) { @@ -1010,6 +1199,7 @@ w_clear_refs(WFILE *wf) } PyMem_Free(wf->refs); PyMem_Free(wf->refpos); + PyMem_Free(wf->ties); } /* Write a buffer to a file. Set an exception on failure. */