GH-155728: Untrack tuples while unmarshalling - #156423
Open
sergey-miryanov wants to merge 4 commits into
Open
Conversation
markshannon
reviewed
Aug 26, 2026
| } | ||
| PyTuple_SET_ITEM(v, i, v2); | ||
| } | ||
| _PyTuple_MaybeUntrack(v); |
Member
There was a problem hiding this comment.
Can we allocate this untracked, then conditionally track it after construction. That way we're not exposing partially constructed tuples to the GC.
Also, it would be a bit more efficient to record whether tracking is needed in the loop, so we don't need to loop over the tuple again
Contributor
Author
There was a problem hiding this comment.
Ok, got it.
Contributor
|
I'm wondering if it makes sense to reuse this set of payloads for testing cpython/Lib/test/test_marshal.py Line 411 in 9036982 My hunch is to check for recursion, self-referentials etc. |
efimov-mikhail
approved these changes
Aug 27, 2026
| break; | ||
| } | ||
| PyTuple_SET_ITEM(v, i, v2); | ||
| if (!track_tuple && PyObject_GC_IsTracked(v2)) { |
Member
There was a problem hiding this comment.
Why not _PyObject_GC_MAY_BE_TRACKED, as in _PyObject_GC_TRACK?
Is there a significant difference between these functions?
Contributor
Author
There was a problem hiding this comment.
PyObject_GC_IsTracked wider than _PyObject_GC_MAY_BE_TRACKED, I'm not sure we should narrow our checks only to tuples as _PyObject_GC_MAY_BE_TRACKED do.
| self._not_tracked_instantly((1, 2) * 5) | ||
|
|
||
| self._not_tracked(((1, x), y, (2, 3))) | ||
| self._not_tracked((1, 2, (True, False, ()))) |
Member
There was a problem hiding this comment.
Maybe, something like this?
t = (True, False, ())
self._not_tracked((1, 2, t))
Because for now self._not_tracked_instantly((1, 2, (True, False, ()))) is also correct.
| @@ -1,3 +1,5 @@ | |||
| import gc | |||
Member
There was a problem hiding this comment.
Maybe we should move this import below?
Contributor
|
@sergey-miryanov To follow up on #156423 (comment). This is not a great test itself but demonstrates the issue: diff --git i/Lib/test/test_marshal.py w/Lib/test/test_marshal.py
index 8ce95e75108..61ef8d65d55 100644
--- i/Lib/test/test_marshal.py
+++ w/Lib/test/test_marshal.py
@@ -8,6 +8,7 @@
import marshal
import sys
import unittest
+import weakref
import os
import types
import textwrap
@@ -923,6 +924,55 @@ def testTuple(self):
self._tracked((set(),))
self._tracked((x, y, z))
+ def _check_untracked_invariant(self, obj, seen=None):
+ seen = set() if seen is None else seen
+ if id(obj) in seen:
+ return
+ seen.add(id(obj))
+ if isinstance(obj, tuple):
+ if not gc.is_tracked(obj):
+ for item in obj:
+ self.assertFalse(gc.is_tracked(item), (obj, item))
+ for item in obj:
+ self._check_untracked_invariant(item, seen)
+ elif isinstance(obj, list):
+ for item in obj:
+ self._check_untracked_invariant(item, seen)
+ elif isinstance(obj, dict):
+ for item in obj.values():
+ self._check_untracked_invariant(item, seen)
+
+ def test_reference_loops(self):
+ for data in [
+ b'\xa8\x01\x00\x00\x00[\x01\x00\x00\x00r\x00\x00\x00\x00', # ([<R>],)
+ b'\xa8\x01\x00\x00\x00{Nr\x00\x00\x00\x000', # ({None: <R>},)
+ b'\xa8\x01\x00\x00\x00r\x00\x00\x00\x00', # (<R>,)
+ b'\xa8\x01\x00\x00\x00(\x01\x00\x00\x00r\x00\x00\x00\x00', # ((<R>,),)
+ b'\xa8\x02\x00\x00\x00(\x01\x00\x00\x00r\x00\x00\x00\x00[\x00\x00\x00\x00', # ((<R>,), [])
+ b'\xa8\x02\x00\x00\x00[\x01\x00\x00\x00r\x00\x00\x00\x00(\x01\x00\x00\x00r\x00\x00\x00\x00', # ([<R>], (<R>,))
+ b'\xa8\x01\x00\x00\x00{N(\x01\x00\x00\x00r\x00\x00\x00\x000', # ({None: (<R>,)},)
+ ]:
+ with self.subTest(data=data):
+ a = marshal.loads(data)
+ self._check_untracked_invariant(a)
+
+ def test_reference_loop_is_collectable(self):
+ data = (b'\xa8\x01\x00\x00\x00{'
+ b'N(\x01\x00\x00\x00r\x00\x00\x00\x00'
+ b'i\x01\x00\x00\x00<\x00\x00\x00\x00'
+ b'0')
+ a = marshal.loads(data)
+ self.assertIs(a[0][None][0], a)
+ ref = weakref.ref(a[0][1])
+ del a
+ support.gc_collect()
+ self.assertIsNone(ref())
+
if __name__ == "__main__":
unittest.main()results in: maurycy@gimel cpython (bug/155728-untrack-tuples-from-marshal 97526e5*) % ./python.exe -m test test_marshal -m test_reference_loops -m test_reference_loop_is_collectable -v
== CPython 3.16.0a0 (codex/pr-156423:97526e51b15, Aug 28 2026, 16:33:23) [Clang 21.0.0 (clang-2100.1.1.101)]
== macOS-26.6.2-arm64-arm-64bit-Mach-O little-endian
== Python build: release with_assert
== cwd: /Users/maurycy/src/github.com/maurycy/cpython/build/test_python_worker_88006æ
== CPU count: 10
== encodings: locale=UTF-8 FS=utf-8
== resources: all test resources are disabled, use -u option to unskip tests
Using random seed: 3877398267
0:00:00 load avg: 1.53 mem: 29.6 MiB Run 1 test sequentially in a single process
0:00:00 load avg: 1.53 mem: 29.6 MiB [1/1] test_marshal
test_reference_loop_is_collectable (test.test_marshal.GCTrackingTestCase.test_reference_loop_is_collectable) ... FAIL
test_reference_loops (test.test_marshal.GCTrackingTestCase.test_reference_loops) ...
test_reference_loops (test.test_marshal.GCTrackingTestCase.test_reference_loops) (data=b'\xa8\x02\x00\x00\x00(\x01\x00\x00\x00r\x00\x00\x00\x00[\x00\x00\x00\x00') ... FAIL
test_reference_loops (test.test_marshal.GCTrackingTestCase.test_reference_loops) (data=b'\xa8\x02\x00\x00\x00[\x01\x00\x00\x00r\x00\x00\x00\x00(\x01\x00\x00\x00r\x00\x00\x00\x00') ... FAIL
test_reference_loops (test.test_marshal.GCTrackingTestCase.test_reference_loops) (data=b'\xa8\x01\x00\x00\x00{N(\x01\x00\x00\x00r\x00\x00\x00\x000') ... FAIL
======================================================================
FAIL: test_reference_loop_is_collectable (test.test_marshal.GCTrackingTestCase.test_reference_loop_is_collectable)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/maurycy/src/github.com/maurycy/cpython/Lib/test/test_marshal.py", line 974, in test_reference_loop_is_collectable
self.assertIsNone(ref())
~~~~~~~~~~~~~~~~~^^^^^^^
AssertionError: set() is not None
======================================================================
FAIL: test_reference_loops (test.test_marshal.GCTrackingTestCase.test_reference_loops) (data=b'\xa8\x02\x00\x00\x00(\x01\x00\x00\x00r\x00\x00\x00\x00[\x00\x00\x00\x00')
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/maurycy/src/github.com/maurycy/cpython/Lib/test/test_marshal.py", line 960, in test_reference_loops
self._check_untracked_invariant(a)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^
File "/Users/maurycy/src/github.com/maurycy/cpython/Lib/test/test_marshal.py", line 939, in _check_untracked_invariant
self._check_untracked_invariant(item, seen)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^
File "/Users/maurycy/src/github.com/maurycy/cpython/Lib/test/test_marshal.py", line 937, in _check_untracked_invariant
self.assertFalse(gc.is_tracked(item), (obj, item))
~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: True is not false : ((((...), []),), (((...),), []))
======================================================================
FAIL: test_reference_loops (test.test_marshal.GCTrackingTestCase.test_reference_loops) (data=b'\xa8\x02\x00\x00\x00[\x01\x00\x00\x00r\x00\x00\x00\x00(\x01\x00\x00\x00r\x00\x00\x00\x00')
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/maurycy/src/github.com/maurycy/cpython/Lib/test/test_marshal.py", line 960, in test_reference_loops
self._check_untracked_invariant(a)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^
File "/Users/maurycy/src/github.com/maurycy/cpython/Lib/test/test_marshal.py", line 939, in _check_untracked_invariant
self._check_untracked_invariant(item, seen)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^
File "/Users/maurycy/src/github.com/maurycy/cpython/Lib/test/test_marshal.py", line 937, in _check_untracked_invariant
self.assertFalse(gc.is_tracked(item), (obj, item))
~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: True is not false : ((([(...)], (...)),), ([(...)], ((...),)))
======================================================================
FAIL: test_reference_loops (test.test_marshal.GCTrackingTestCase.test_reference_loops) (data=b'\xa8\x01\x00\x00\x00{N(\x01\x00\x00\x00r\x00\x00\x00\x000')
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/maurycy/src/github.com/maurycy/cpython/Lib/test/test_marshal.py", line 960, in test_reference_loops
self._check_untracked_invariant(a)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^
File "/Users/maurycy/src/github.com/maurycy/cpython/Lib/test/test_marshal.py", line 939, in _check_untracked_invariant
self._check_untracked_invariant(item, seen)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^
File "/Users/maurycy/src/github.com/maurycy/cpython/Lib/test/test_marshal.py", line 945, in _check_untracked_invariant
self._check_untracked_invariant(item, seen)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^
File "/Users/maurycy/src/github.com/maurycy/cpython/Lib/test/test_marshal.py", line 937, in _check_untracked_invariant
self.assertFalse(gc.is_tracked(item), (obj, item))
~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: True is not false : ((({None: (...)},),), ({None: ((...),)},))
----------------------------------------------------------------------
Ran 2 tests in 0.005s
FAILED (failures=4)
test test_marshal failed
0:00:00 load avg: 1.53 mem: 30.5 MiB [1/1/1] test_marshal failed (4 failures)
== Tests result: FAILURE ==
1 test failed:
test_marshal
Total duration: 32 ms
Total tests: run=2 (filtered) failures=4
Total test files: run=1/1 (filtered) failed=1
Result: FAILURE
[2] maurycy@gimel cpython (bug/155728-untrack-tuples-from-marshal 97526e5*) % While on main (with the addded maurycy@gimel cpython (main 0e29768*?) % ./python.exe -m test test_marshal -m test_reference_loops -m test_reference_loop_is_collectable -v
== CPython 3.16.0a0 (heads/main:0e297684937, Aug 28 2026, 18:00:16) [Clang 21.0.0 (clang-2100.1.1.101)]
== macOS-26.6.2-arm64-arm-64bit-Mach-O little-endian
== Python build: release ThinLTO+PGO
== cwd: /Users/maurycy/src/github.com/maurycy/cpython/build/test_python_worker_1435æ
== CPU count: 10
== encodings: locale=UTF-8 FS=utf-8
== resources: all test resources are disabled, use -u option to unskip tests
Using random seed: 154831838
0:00:00 load avg: 7.56 mem: 29.1 MiB Run 1 test sequentially in a single process
0:00:00 load avg: 7.56 mem: 29.1 MiB [1/1] test_marshal
test_reference_loop_is_collectable (test.test_marshal.CAPI_TestCase.test_reference_loop_is_collectable) ... ok
test_reference_loops (test.test_marshal.CAPI_TestCase.test_reference_loops) ... ok
----------------------------------------------------------------------
Ran 2 tests in 0.003s
OK
0:00:00 load avg: 7.56 mem: 32.5 MiB [1/1] test_marshal passed
== Tests result: SUCCESS ==
1 test OK.
Total duration: 29 ms
Total tests: run=2 (filtered)
Total test files: run=1/1 (filtered)
Result: SUCCESS
maurycy@gimel cpython (main 0e29768*?) % Basically, it seems to end up with an untracked tuple containing a tracked object. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.