From 407a47329d2815a2de0696818bfc5e7ddb1b932d Mon Sep 17 00:00:00 2001 From: Duane Griffin Date: Sat, 19 Apr 2025 16:05:35 +1200 Subject: [PATCH 1/5] gh-106287: do not write objects after encountering an unmarshalling error Writing out an object may involve a slot lookup, which is not safe to do with an exception raised. In debug mode an assertion failure will occur if this happens. To avoid assertion failures, and possibly more subtle problems the assertion is there to prevent, skip attempts to write an object after an error. Note the unmarshal writing code won't raise an exception itself, however the set writing code calls back into the top-level unmarshal function (see gh-78903), which will check for failures and raises an exception. Once that happens it is not safe to continue writing further objects. Note also that some data may still be written even after an error occurred and an exception has been raised: code objects write objects and longs, and the latter will be written unconditionally, even if the former fails. This shouldn't cause a problem as it is documented that "garbage data will also be written to the file" in such cases, and the top-level function will handle and report the error appropriately. Add a test case to cover the various different object types which could trigger such failures. --- Lib/test/test_marshal.py | 19 +++++++++++++++++++ Python/marshal.c | 7 +++++++ 2 files changed, 26 insertions(+) diff --git a/Lib/test/test_marshal.py b/Lib/test/test_marshal.py index 8b1fb0eba1f8b6..c3948957e81fa3 100644 --- a/Lib/test/test_marshal.py +++ b/Lib/test/test_marshal.py @@ -408,6 +408,25 @@ def test_deterministic_sets(self): _, dump_1, _ = assert_python_ok(*args, PYTHONHASHSEED="1") self.assertEqual(dump_0, dump_1) + def test_unmarshallable(self): + # gh-106287 + fset = frozenset([int]) + code = compile("a = 1", "", "exec") + code = code.replace(co_consts=(1, fset, None)) + cases = (('tuple', (fset,)), + ('list', [fset]), + ('set', fset), + ('dict key', {fset: 'x'}), + ('dict value', {'x': fset}), + ('dict key & value', {fset: fset}), + ('dict set key & buffer', {fset: fset}), + ('slice', slice(fset, fset)), + ('code', code)) + for name, arg in cases: + with self.subTest(name, arg=arg): + with self.assertRaisesRegex(ValueError, "unmarshallable object"): + marshal.dumps((arg, memoryview(b''))) + LARGE_SIZE = 2**31 pointer_size = 8 if sys.maxsize > 0xFFFFFFFF else 4 diff --git a/Python/marshal.c b/Python/marshal.c index b39c1a5b1ade50..3c639e0d8aae26 100644 --- a/Python/marshal.c +++ b/Python/marshal.c @@ -431,6 +431,9 @@ w_object(PyObject *v, WFILE *p) { char flag = '\0'; + if (p->error != WFERR_OK) + return; + p->depth++; if (p->depth > MAX_MARSHAL_STACK_DEPTH) { @@ -750,6 +753,10 @@ PyMarshal_WriteLongToFile(long x, FILE *fp, int version) w_flush(&wf); } +/* TODO: Contrary to its documentation, this function does NOT set an error on + * failure. It is not used internally except from test code, where this doesn't + * matter, but this discrepancy might cause problems for any external code + * using it. */ void PyMarshal_WriteObjectToFile(PyObject *x, FILE *fp, int version) { From b77cd2d0e21b11d36de73adb2ccd4e3d947aa840 Mon Sep 17 00:00:00 2001 From: Duane Griffin Date: Sat, 19 Apr 2025 17:34:18 +1200 Subject: [PATCH 2/5] Add blurb --- .../next/Library/2025-04-19-17-34-11.gh-issue-132715.XXl47F.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Library/2025-04-19-17-34-11.gh-issue-132715.XXl47F.rst diff --git a/Misc/NEWS.d/next/Library/2025-04-19-17-34-11.gh-issue-132715.XXl47F.rst b/Misc/NEWS.d/next/Library/2025-04-19-17-34-11.gh-issue-132715.XXl47F.rst new file mode 100644 index 00000000000000..191b4f16e0f400 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2025-04-19-17-34-11.gh-issue-132715.XXl47F.rst @@ -0,0 +1 @@ +Skip writing objects during marshalling once a failure has occurred. From dc529610a4581b83881cef5409226305ef64d43d Mon Sep 17 00:00:00 2001 From: Duane Griffin Date: Mon, 21 Apr 2025 00:47:28 +1200 Subject: [PATCH 3/5] Tweaks from review feedback (thanks!) --- Lib/test/test_marshal.py | 4 +++- Python/marshal.c | 7 ++----- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/Lib/test/test_marshal.py b/Lib/test/test_marshal.py index c3948957e81fa3..bd38b188dd095c 100644 --- a/Lib/test/test_marshal.py +++ b/Lib/test/test_marshal.py @@ -409,7 +409,8 @@ def test_deterministic_sets(self): self.assertEqual(dump_0, dump_1) def test_unmarshallable(self): - # gh-106287 + # Check no crash after encountering unmarshallable objects. + # See https://github.com/python/cpython/issues/106287 fset = frozenset([int]) code = compile("a = 1", "", "exec") code = code.replace(co_consts=(1, fset, None)) @@ -427,6 +428,7 @@ def test_unmarshallable(self): with self.assertRaisesRegex(ValueError, "unmarshallable object"): marshal.dumps((arg, memoryview(b''))) + LARGE_SIZE = 2**31 pointer_size = 8 if sys.maxsize > 0xFFFFFFFF else 4 diff --git a/Python/marshal.c b/Python/marshal.c index 3c639e0d8aae26..75379a79f44cb6 100644 --- a/Python/marshal.c +++ b/Python/marshal.c @@ -431,8 +431,9 @@ w_object(PyObject *v, WFILE *p) { char flag = '\0'; - if (p->error != WFERR_OK) + if (p->error != WFERR_OK) { return; + } p->depth++; @@ -753,10 +754,6 @@ PyMarshal_WriteLongToFile(long x, FILE *fp, int version) w_flush(&wf); } -/* TODO: Contrary to its documentation, this function does NOT set an error on - * failure. It is not used internally except from test code, where this doesn't - * matter, but this discrepancy might cause problems for any external code - * using it. */ void PyMarshal_WriteObjectToFile(PyObject *x, FILE *fp, int version) { From f410a395dc802939df25a9ecbcfddbcad631df56 Mon Sep 17 00:00:00 2001 From: Duane Griffin Date: Sun, 18 May 2025 20:08:39 +1200 Subject: [PATCH 4/5] Update Lib/test/test_marshal.py MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com> --- Lib/test/test_marshal.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/test_marshal.py b/Lib/test/test_marshal.py index bd38b188dd095c..84508eca3d52a6 100644 --- a/Lib/test/test_marshal.py +++ b/Lib/test/test_marshal.py @@ -410,7 +410,7 @@ def test_deterministic_sets(self): def test_unmarshallable(self): # Check no crash after encountering unmarshallable objects. - # See https://github.com/python/cpython/issues/106287 + # See https://github.com/python/cpython/issues/106287. fset = frozenset([int]) code = compile("a = 1", "", "exec") code = code.replace(co_consts=(1, fset, None)) From 329c6e24f60af478364906f884a7b4c18da63d47 Mon Sep 17 00:00:00 2001 From: Duane Griffin Date: Sun, 18 May 2025 20:11:18 +1200 Subject: [PATCH 5/5] Drop repeated test case parameters --- Lib/test/test_marshal.py | 1 - 1 file changed, 1 deletion(-) diff --git a/Lib/test/test_marshal.py b/Lib/test/test_marshal.py index 84508eca3d52a6..570aafc57ad9e7 100644 --- a/Lib/test/test_marshal.py +++ b/Lib/test/test_marshal.py @@ -420,7 +420,6 @@ def test_unmarshallable(self): ('dict key', {fset: 'x'}), ('dict value', {'x': fset}), ('dict key & value', {fset: fset}), - ('dict set key & buffer', {fset: fset}), ('slice', slice(fset, fset)), ('code', code)) for name, arg in cases: