From f39262d51ba271f6b686c5d4bea826f6e19bedb2 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Fri, 4 Sep 2026 17:48:45 +0300 Subject: [PATCH] gh-156942: Raise the exception where the marshalling error is detected Previously the marshal writer recorded an error code and converted it into an exception at the end, replacing the exception which was already raised with ValueError("unmarshallable object"). Error messages now name the type of the unsupported object and the required version. Co-Authored-By: Claude Opus 5 (1M context) --- Lib/test/test_marshal.py | 89 ++++++++++---- ...-09-04-16-12-30.gh-issue-156942.Qk7Rw2.rst | 4 + Python/marshal.c | 114 ++++++++---------- 3 files changed, 117 insertions(+), 90 deletions(-) create mode 100644 Misc/NEWS.d/next/Core_and_Builtins/2026-09-04-16-12-30.gh-issue-156942.Qk7Rw2.rst diff --git a/Lib/test/test_marshal.py b/Lib/test/test_marshal.py index c595e8cf14f1e15..f8c957fef1d40b8 100644 --- a/Lib/test/test_marshal.py +++ b/Lib/test/test_marshal.py @@ -154,14 +154,16 @@ def test_no_allow_code(self): data = {'a': [({co, 0},)]} dump = marshal.dumps(data, allow_code=True) self.assertEqual(marshal.loads(dump, allow_code=True), data) - with self.assertRaises(ValueError): + with self.assertRaisesRegex(ValueError, + 'marshalling code objects is disallowed'): marshal.dumps(data, allow_code=False) with self.assertRaises(ValueError): marshal.loads(dump, allow_code=False) marshal.dump(data, io.BytesIO(), allow_code=True) self.assertEqual(marshal.load(io.BytesIO(dump), allow_code=True), data) - with self.assertRaises(ValueError): + with self.assertRaisesRegex(ValueError, + 'marshalling code objects is disallowed'): marshal.dump(data, io.BytesIO(), allow_code=False) with self.assertRaises(ValueError): marshal.load(io.BytesIO(dump), allow_code=False) @@ -339,16 +341,29 @@ def test_reference_loop_dict(self): self.assertIsInstance(b, dict) self.assertIs(b[None], b) + def check_reference_loop(self, a, typename, minversion, + oldmsg='object too deeply nested to marshal'): + # Only versions supporting references to the type detect the loop; + # older versions fail for a different reason. + for v in range(minversion): + with self.subTest(version=v): + with self.assertRaisesRegex(ValueError, oldmsg): + marshal.dumps(a, v) + for v in range(minversion, marshal.version + 1): + with self.subTest(version=v): + with self.assertRaisesRegex( + ValueError, + f'cannot marshal recursion {typename} objects'): + marshal.dumps(a, v) + def test_reference_loop_tuple(self): a = ([],) a[0].append(a) - for v in range(marshal.version + 1): - self.assertRaises(ValueError, marshal.dumps, a, v) + self.check_reference_loop(a, 'tuple', 3) a = ({},) a[0][None] = a - for v in range(marshal.version + 1): - self.assertRaises(ValueError, marshal.dumps, a, v) + self.check_reference_loop(a, 'tuple', 3) def test_shared_reference_tuple(self): # A tuple referenced more than once still round-trips with the @@ -373,30 +388,28 @@ def f(): # so we need to break the loop manually. See gh-148722. self.addCleanup(a.clear) a.append(code) - for v in range(marshal.version + 1): - self.assertRaises(ValueError, marshal.dumps, code, v) + self.check_reference_loop(code, 'code', 3) def test_reference_loop_slice(self): + oldmsg = 'marshalling slice objects requires version 5 or higher' a = slice([], None) a.start.append(a) - for v in range(marshal.version + 1): - self.assertRaises(ValueError, marshal.dumps, a, v) + self.check_reference_loop(a, 'slice', 5, oldmsg) a = slice(None, []) a.stop.append(a) - for v in range(marshal.version + 1): - self.assertRaises(ValueError, marshal.dumps, a, v) + self.check_reference_loop(a, 'slice', 5, oldmsg) a = slice(None, None, []) a.step.append(a) - for v in range(marshal.version + 1): - self.assertRaises(ValueError, marshal.dumps, a, v) + self.check_reference_loop(a, 'slice', 5, oldmsg) def test_reference_loop_frozendict(self): a = frozendict({None: []}) a[None].append(a) - for v in range(marshal.version + 1): - self.assertRaises(ValueError, marshal.dumps, a, v) + self.check_reference_loop( + a, 'frozendict', 6, + 'marshalling frozendict objects requires version 6 or higher') def test_shared_reference_frozendict(self): # A frozendict referenced more than once must round-trip with the @@ -467,7 +480,9 @@ def test_exact_type_match(self): # Note: str subclasses are not tested because they get handled # by marshal's routines for objects supporting the buffer API. subtyp = type('subtyp', (typ,), {}) - self.assertRaises(ValueError, marshal.dumps, subtyp()) + with self.assertRaisesRegex(ValueError, + r'cannot marshal \S*subtyp objects'): + marshal.dumps(subtyp()) # Issue #1792 introduced a change in how marshal increases the size of its # internal buffer; this test ensures that the new code is exercised. @@ -570,9 +585,25 @@ def test_unmarshallable(self): ('code', code)) for name, arg in cases: with self.subTest(name, arg=arg): - with self.assertRaisesRegex(ValueError, "unmarshallable object"): + with self.assertRaisesRegex(ValueError, + "cannot marshal type objects"): marshal.dumps((arg, memoryview(b''))) + def test_error_in_set_item(self): + # Set items are sorted by their marshalled representation, and NaNs + # are only distinguished by identity, so they are compared as + # complex numbers. + nan = float('nan') + with self.assertRaisesRegex(TypeError, "'<' not supported"): + marshal.dumps({complex(nan, 0), complex(nan, 0)}) + + def test_error_in_buffer(self): + # The BufferError raised for a non-contiguous buffer is not replaced + # with a generic error. + step2 = slice(None, None, 2) + with self.assertRaises(BufferError): + marshal.dumps(memoryview(bytearray(b'abcdef'))[step2]) + LARGE_SIZE = 2**31 pointer_size = 8 if sys.maxsize > 0xFFFFFFFF else 4 @@ -583,8 +614,14 @@ def write(self, s): @unittest.skipIf(LARGE_SIZE > sys.maxsize, "test cannot run on 32-bit systems") class LargeValuesTestCase(unittest.TestCase): - def check_unmarshallable(self, data): - self.assertRaises(ValueError, marshal.dump, data, NullWriter()) + def check_unmarshallable(self, data, msg='object too large to marshal'): + with self.assertRaisesRegex(ValueError, msg): + marshal.dump(data, NullWriter()) + + @support.bigmemtest(size=LARGE_SIZE, memuse=4, dry_run=False) + def test_int(self, size): + # An int with more than SIZE32_MAX 15-bit digits. + self.check_unmarshallable(1 << (15 * size), 'int too large to marshal') @support.bigmemtest(size=LARGE_SIZE, memuse=2, dry_run=False) def test_bytes(self, size): @@ -717,7 +754,10 @@ def testFrozenDict(self): self.helper(dictobj) for version in range(6): - with self.assertRaises(ValueError): + with self.assertRaisesRegex( + ValueError, + 'marshalling frozendict objects requires ' + 'version 6 or higher'): marshal.dumps(dictobj, version) def testModule(self): @@ -786,7 +826,10 @@ def test_slice(self): self.helper(obj) for version in range(5): - with self.assertRaises(ValueError): + with self.assertRaisesRegex( + ValueError, + 'marshalling slice objects requires ' + 'version 5 or higher'): marshal.dumps(obj, version) @support.cpython_only @@ -818,7 +861,7 @@ def test_write_to_file_error(self): def test_write_unmarshallable_to_file(self): self.addCleanup(os_helper.unlink, os_helper.TESTFN) - with self.assertRaisesRegex(ValueError, 'unmarshallable object'): + with self.assertRaisesRegex(ValueError, 'cannot marshal object objects'): _testcapi.pymarshal_write_object_to_file(object(), os_helper.TESTFN, marshal.version) diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2026-09-04-16-12-30.gh-issue-156942.Qk7Rw2.rst b/Misc/NEWS.d/next/Core_and_Builtins/2026-09-04-16-12-30.gh-issue-156942.Qk7Rw2.rst new file mode 100644 index 000000000000000..24bbeef0461efb7 --- /dev/null +++ b/Misc/NEWS.d/next/Core_and_Builtins/2026-09-04-16-12-30.gh-issue-156942.Qk7Rw2.rst @@ -0,0 +1,4 @@ +:mod:`marshal` no longer replaces the exception raised while marshalling the +value with a generic ``ValueError("unmarshallable object")``. Error messages +now name the type of the unsupported object and, for the types supported only +by newer data formats, the required version. diff --git a/Python/marshal.c b/Python/marshal.c index 1897d700c055bd3..53e84d8efbfc6db 100644 --- a/Python/marshal.c +++ b/Python/marshal.c @@ -102,11 +102,7 @@ module marshal // Error codes: #define WFERR_OK 0 -#define WFERR_UNMARSHALLABLE 1 -#define WFERR_NESTEDTOODEEP 2 -#define WFERR_NOMEMORY 3 -#define WFERR_CODE_NOT_ALLOWED 4 -#define WFERR_EXCEPTION_SET 5 /* An exception has already been raised. */ +#define WFERR_EXCEPTION_SET 1 /* An exception has been raised. */ typedef struct { FILE *fp; @@ -174,12 +170,14 @@ w_reserve(WFILE *p, Py_ssize_t needed) delta = size + 1024; delta = Py_MAX(delta, needed); if (delta > PY_SSIZE_T_MAX - size) { - p->error = WFERR_NOMEMORY; + PyErr_NoMemory(); + p->error = WFERR_EXCEPTION_SET; return 0; } size += delta; if (_PyBytes_Resize(&p->str, size) != 0) { p->end = p->ptr = p->buf = NULL; + p->error = WFERR_EXCEPTION_SET; return 0; } else { @@ -236,13 +234,15 @@ w_long(long x, WFILE *p) #define SIZE32_MAX 0x7FFFFFFF #if SIZEOF_SIZE_T > 4 -# define W_SIZE(n, p) do { \ - if ((n) > SIZE32_MAX) { \ - (p)->depth--; \ - (p)->error = WFERR_UNMARSHALLABLE; \ - return; \ - } \ - w_long((long)(n), p); \ +# define W_SIZE(n, p) do { \ + if ((n) > SIZE32_MAX) { \ + (p)->depth--; \ + PyErr_SetString(PyExc_ValueError, \ + "object too large to marshal"); \ + (p)->error = WFERR_EXCEPTION_SET; \ + return; \ + } \ + w_long((long)(n), p); \ } while(0) #else # define W_SIZE w_long @@ -295,7 +295,8 @@ _r_digits##bitsize(const uint ## bitsize ## _t *digits, Py_ssize_t n, \ } while (d != 0); \ if (l > SIZE32_MAX) { \ p->depth--; \ - p->error = WFERR_UNMARSHALLABLE; \ + PyErr_SetString(PyExc_ValueError, "int too large to marshal"); \ + p->error = WFERR_EXCEPTION_SET; \ return; \ } \ w_long((long)(negative ? -l : l), p); \ @@ -331,7 +332,7 @@ w_PyLong(const PyLongObject *ob, char flag, WFILE *p) if (PyLong_Export((PyObject *)ob, &long_export) < 0) { p->depth--; - p->error = WFERR_UNMARSHALLABLE; + p->error = WFERR_EXCEPTION_SET; return; } if (!long_export.digits) { @@ -384,7 +385,7 @@ w_float_bin(double v, WFILE *p) { char buf[8]; if (PyFloat_Pack8(v, buf, 1) < 0) { - p->error = WFERR_UNMARSHALLABLE; + p->error = WFERR_EXCEPTION_SET; return; } w_string(buf, 8, p); @@ -395,7 +396,7 @@ w_float_str(double v, WFILE *p) { char *buf = PyOS_double_to_string(v, 'g', 17, 0, NULL); if (!buf) { - p->error = WFERR_NOMEMORY; + p->error = WFERR_EXCEPTION_SET; return; } w_short_pstring(buf, strlen(buf), p); @@ -449,13 +450,14 @@ w_ref(PyObject *v, char *flag, WFILE *p) if (_Py_hashtable_set(p->hashtable, Py_NewRef(v), (void *)(uintptr_t)w) < 0) { Py_DECREF(v); + PyErr_NoMemory(); goto err; } *flag |= FLAG_REF; return 0; } err: - p->error = WFERR_UNMARSHALLABLE; + p->error = WFERR_EXCEPTION_SET; return 1; } @@ -495,7 +497,9 @@ w_object(PyObject *v, WFILE *p) p->depth++; if (p->depth > MAX_MARSHAL_STACK_DEPTH) { - p->error = WFERR_NESTEDTOODEEP; + PyErr_SetString(PyExc_ValueError, + "object too deeply nested to marshal"); + p->error = WFERR_EXCEPTION_SET; } else if (v == NULL) { w_byte(TYPE_NULL, p); @@ -598,7 +602,7 @@ w_complex_object(PyObject *v, char flag, WFILE *p) utf8 = PyUnicode_AsEncodedString(v, "utf8", "surrogatepass"); if (utf8 == NULL) { p->depth--; - p->error = WFERR_UNMARSHALLABLE; + p->error = WFERR_EXCEPTION_SET; return; } if (p->version >= 3 && PyUnicode_CHECK_INTERNED(v)) @@ -638,7 +642,10 @@ w_complex_object(PyObject *v, char flag, WFILE *p) if (PyFrozenDict_CheckExact(v)) { if (p->version < 6) { w_byte(TYPE_UNKNOWN, p); - p->error = WFERR_UNMARSHALLABLE; + PyErr_Format(PyExc_ValueError, + "marshalling %T objects requires version 6 " + "or higher", v); + p->error = WFERR_EXCEPTION_SET; return; } @@ -675,7 +682,7 @@ w_complex_object(PyObject *v, char flag, WFILE *p) // use an order equivalent to sorted(v, key=marshal.dumps): PyObject *pairs = PyList_New(n); if (pairs == NULL) { - p->error = WFERR_NOMEMORY; + p->error = WFERR_EXCEPTION_SET; return; } Py_ssize_t i = 0; @@ -684,25 +691,25 @@ w_complex_object(PyObject *v, char flag, WFILE *p) PyObject *dump = _PyMarshal_WriteObjectToString(value, p->version, p->allow_code); if (dump == NULL) { - p->error = WFERR_UNMARSHALLABLE; + p->error = WFERR_EXCEPTION_SET; Py_DECREF(value); break; } PyObject *pair = _PyTuple_FromPairSteal(dump, value); if (pair == NULL) { - p->error = WFERR_NOMEMORY; + p->error = WFERR_EXCEPTION_SET; break; } PyList_SET_ITEM(pairs, i++, pair); } Py_END_CRITICAL_SECTION(); - if (p->error == WFERR_UNMARSHALLABLE || p->error == WFERR_NOMEMORY) { + if (p->error != WFERR_OK) { Py_DECREF(pairs); return; } assert(i == n); if (PyList_Sort(pairs)) { - p->error = WFERR_NOMEMORY; + p->error = WFERR_EXCEPTION_SET; Py_DECREF(pairs); return; } @@ -715,13 +722,15 @@ w_complex_object(PyObject *v, char flag, WFILE *p) } else if (PyCode_Check(v)) { if (!p->allow_code) { - p->error = WFERR_CODE_NOT_ALLOWED; + PyErr_SetString(PyExc_ValueError, + "marshalling code objects is disallowed"); + p->error = WFERR_EXCEPTION_SET; return; } PyCodeObject *co = (PyCodeObject *)v; PyObject *co_code = _PyCode_GetCode(co); if (co_code == NULL) { - p->error = WFERR_NOMEMORY; + p->error = WFERR_EXCEPTION_SET; return; } W_TYPE(TYPE_CODE, p); @@ -750,7 +759,7 @@ w_complex_object(PyObject *v, char flag, WFILE *p) if (PyObject_GetBuffer(v, &view, PyBUF_SIMPLE) != 0) { w_byte(TYPE_UNKNOWN, p); p->depth--; - p->error = WFERR_UNMARSHALLABLE; + p->error = WFERR_EXCEPTION_SET; return; } W_TYPE(TYPE_STRING, p); @@ -760,7 +769,10 @@ w_complex_object(PyObject *v, char flag, WFILE *p) else if (PySlice_Check(v)) { if (p->version < 5) { w_byte(TYPE_UNKNOWN, p); - p->error = WFERR_UNMARSHALLABLE; + PyErr_Format(PyExc_ValueError, + "marshalling %T objects requires version 5 " + "or higher", v); + p->error = WFERR_EXCEPTION_SET; return; } PySliceObject *slice = (PySliceObject *)v; @@ -772,7 +784,8 @@ w_complex_object(PyObject *v, char flag, WFILE *p) } else { W_TYPE(TYPE_UNKNOWN, p); - p->error = WFERR_UNMARSHALLABLE; + PyErr_Format(PyExc_ValueError, "cannot marshal %T objects", v); + p->error = WFERR_EXCEPTION_SET; } } @@ -806,35 +819,6 @@ w_clear_refs(WFILE *wf) } } -/* Set the exception indicator according to the recorded error. */ -static void -w_set_exception(WFILE *p) -{ - assert(p->error != WFERR_OK); - switch (p->error) { - case WFERR_NOMEMORY: - PyErr_NoMemory(); - break; - case WFERR_NESTEDTOODEEP: - PyErr_SetString(PyExc_ValueError, - "object too deeply nested to marshal"); - break; - case WFERR_CODE_NOT_ALLOWED: - PyErr_SetString(PyExc_ValueError, - "marshalling code objects is disallowed"); - break; - case WFERR_EXCEPTION_SET: - /* An exception has already been raised. */ - assert(PyErr_Occurred()); - break; - default: - case WFERR_UNMARSHALLABLE: - PyErr_SetString(PyExc_ValueError, - "unmarshallable object"); - break; - } -} - /* version currently has no effect for writing ints. */ void PyMarshal_WriteLongToFile(long x, FILE *fp, int version) @@ -849,9 +833,7 @@ PyMarshal_WriteLongToFile(long x, FILE *fp, int version) wf.version = version; w_long(x, &wf); w_flush(&wf); - if (wf.error != WFERR_OK) { - w_set_exception(&wf); - } + assert(wf.error == WFERR_OK || PyErr_Occurred()); } void @@ -875,9 +857,7 @@ PyMarshal_WriteObjectToFile(PyObject *x, FILE *fp, int version) w_object(x, &wf); w_clear_refs(&wf); w_flush(&wf); - if (wf.error != WFERR_OK) { - w_set_exception(&wf); - } + assert(wf.error == WFERR_OK || PyErr_Occurred()); } typedef struct { @@ -2024,8 +2004,8 @@ _PyMarshal_WriteObjectToString(PyObject *x, int version, int allow_code) return NULL; } if (wf.error != WFERR_OK) { + assert(PyErr_Occurred()); Py_XDECREF(wf.str); - w_set_exception(&wf); return NULL; } return wf.str;