Skip to content

gh-106287: do not write objects after an unmarshalling error #132715

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

gh-106287: do not write objects after an unmarshalling error #132715

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
407a473
gh-106287: do not write objects after encountering an unmarshalling e…
duaneg Apr 19, 2025
b77cd2d
Add blurb
duaneg Apr 19, 2025
dc52961
Tweaks from review feedback (thanks!)
duaneg Apr 20, 2025
f410a39
Update Lib/test/test_marshal.py
duaneg May 18, 2025
329c6e2
Drop repeated test case parameters
duaneg May 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions Lib/test/test_marshal.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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", "<string>", "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

Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Library/2025-04-19-17-34-11.gh-issue-132715.XXl47F.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Skip writing objects during marshalling once a failure has occurred.
7 changes: 7 additions & 0 deletions Python/marshal.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does. PySys_Audit or w_init_refs may set an error on failure. The docs say that exceptions are due to marshalling itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, those functions set errors as appropriate, apologies for being unclear. Various other error conditions are correctly reported, too. However unmarshallable objects like in this issue are not. AIUI they should raise a ValueError: unmarshallable object (as PyMarshal_WriteObjectToString does), but the error is silently ignored by PyMarshal_WriteObjectToFile.

The problem is the errors are recorded in WFILE::error (e.g. see the end of w_complex_object) and it is only in _PyMarshal_WriteObjectToString that is converted into an exception. This doesn't happen in PyMarshal_WriteObjectToFile.

This can be reproduced with python -c "import _testcapi; _testcapi.pymarshal_write_object_to_file(int, 'test.data', 5)" (with assertions enabled). It asserts there are no exceptions raised after calling the function, but completes successfully. If you replace int with {int} it will abort as expected.

I can reword or just drop the comment, whatever you prefer. Maybe something like, "TODO: this function silently ignores some failures due to unmarshallable objects"? I'd be happy to just fix it, for that matter, but I wasn't sure whether it would be considered worth the churn.

Now I look at that code again, I note the paths that currently do raise exceptions while writing objects are going to have them overwritten by that error handling code anyway. That's a shame, since they give specifics like "bad marshal data (unnormalized long data)" instead of a generic "unmarshallable object" message. Perhaps we shouldn't raise an exception if there's already one raised?

* 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)
{
Expand Down
Loading