Skip to content

Commit d64cc2c

Browse files
youknowoneyouknowone
authored
Fix marshal recursive reference loading (#8501)
* Fix marshal recursive reference loading Create reference-tracked containers before reading their children so recursive list, dict, set, and tuple graphs can be unmarshaled. Preserve interned string markers through the runtime bag and add an initialization-only tuple construction path. Assisted-by: Codex:gpt-5 * Preserve marshal container insertion errors Keep Python exceptions raised while constructing unmarshaled sets, frozensets, and dictionaries instead of collapsing them into ValueError. This makes abnormal recursive hash-container streams report TypeError like CPython and removes the remaining test_marshal expected failure. Assisted-by: Codex:gpt-5 * Enable full abnormal marshal reference loop test * Test direct marshal tuple reference loop Assisted-by: Codex:gpt-5
1 parent 9de06cc commit d64cc2c

4 files changed

Lines changed: 366 additions & 89 deletions

File tree

‎Lib/test/test_marshal.py‎

Lines changed: 7 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -319,7 +319,6 @@ def test_recursion_limit(self):
319319
last.append([0])
320320
self.assertRaises(ValueError, marshal.dumps, head)
321321

322-
@unittest.expectedFailure # TODO: RUSTPYTHON; ValueError: bad marshal data
323322
def test_reference_loop_list(self):
324323
a = []
325324
a.append(a)
@@ -331,7 +330,6 @@ def test_reference_loop_list(self):
331330
self.assertIsInstance(b, list)
332331
self.assertIs(b[0], b)
333332

334-
@unittest.expectedFailure # TODO: RUSTPYTHON; ValueError: bad marshal data
335333
def test_reference_loop_dict(self):
336334
a = {}
337335
a[None] = a
@@ -343,7 +341,6 @@ def test_reference_loop_dict(self):
343341
self.assertIsInstance(b, dict)
344342
self.assertIs(b[None], b)
345343

346-
@unittest.expectedFailure # TODO: RUSTPYTHON; ValueError: bad marshal data
347344
def test_reference_loop_tuple(self):
348345
a = ([],)
349346
a[0].append(a)
@@ -387,21 +384,18 @@ def test_reference_loop_slice(self):
387384
for v in range(marshal.version + 1):
388385
self.assertRaises(ValueError, marshal.dumps, a, v)
389386

390-
@unittest.expectedFailure # TODO: RUSTPYTHON; ValueError: bad marshal data
391387
def test_loads_reference_loop_list(self):
392388
data = b'\xdb\x01\x00\x00\x00r\x00\x00\x00\x00' # [<R>]
393389
a = marshal.loads(data)
394390
self.assertIsInstance(a, list)
395391
self.assertIs(a[0], a)
396392

397-
@unittest.expectedFailure # TODO: RUSTPYTHON; ValueError: bad marshal data
398393
def test_loads_reference_loop_dict(self):
399394
data = b'\xfbNr\x00\x00\x00\x000' # {None: <R>}
400395
a = marshal.loads(data)
401396
self.assertIsInstance(a, dict)
402397
self.assertIs(a[None], a)
403398

404-
@unittest.expectedFailure # TODO: RUSTPYTHON; ValueError: bad marshal data
405399
def test_loads_abnormal_reference_loops(self):
406400
# Indirect self-references of tuples.
407401
data = b'\xa8\x01\x00\x00\x00[\x01\x00\x00\x00r\x00\x00\x00\x00' # ([<R>],)
@@ -416,13 +410,13 @@ def test_loads_abnormal_reference_loops(self):
416410
self.assertIsInstance(a[0], dict)
417411
self.assertIs(a[0][None], a)
418412

419-
# Direct self-reference which cannot be created in Python.
420-
# This creates a reference loop which cannot be collected.
421-
if False:
422-
data = b'\xa8\x01\x00\x00\x00r\x00\x00\x00\x00' # (<R>,)
423-
a = marshal.loads(data)
424-
self.assertIsInstance(a, tuple)
425-
self.assertIs(a[0], a)
413+
# Direct self-reference which cannot be created in Python. CPython
414+
# leaves this disabled because its reference counting cannot collect
415+
# the resulting cycle; RustPython's tracing collector can.
416+
data = b'\xa8\x01\x00\x00\x00r\x00\x00\x00\x00' # (<R>,)
417+
a = marshal.loads(data)
418+
self.assertIsInstance(a, tuple)
419+
self.assertIs(a[0], a)
426420

427421
# Direct self-references which cannot be created in Python
428422
# because of unhashability.
@@ -748,7 +742,6 @@ class InterningTestCase(unittest.TestCase, HelperMixin):
748742
strobj = "this is an interned string"
749743
strobj = sys.intern(strobj)
750744

751-
@unittest.expectedFailure # TODO: RUSTPYTHON
752745
def testIntern(self):
753746
s = marshal.loads(marshal.dumps(self.strobj))
754747
self.assertEqual(s, self.strobj)

‎crates/compiler-core/src/marshal.rs‎

Lines changed: 144 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -516,7 +516,7 @@ fn read_const_value<R: Read, Bag: ConstantBag>(
516516
let code = deserialize_code_inner(rdr, bag, depth - 1, refs)?;
517517
bag.make_code(code)
518518
} else {
519-
deserialize_value_typed(rdr, bag, depth, refs, typ)?
519+
deserialize_value_typed(rdr, bag, depth, refs, typ, slot)?
520520
};
521521
if let Some(idx) = slot {
522522
refs[idx] = Some(value.clone());
@@ -540,6 +540,10 @@ pub trait MarshalBag: Copy {
540540

541541
fn make_str(&self, value: &Wtf8) -> Self::Value;
542542

543+
fn make_interned_str(&self, value: &Wtf8) -> Self::Value {
544+
self.make_str(value)
545+
}
546+
543547
fn make_bytes(&self, value: &[u8]) -> Self::Value;
544548

545549
fn make_int(&self, value: BigInt) -> Self::Value;
@@ -564,6 +568,51 @@ pub trait MarshalBag: Copy {
564568
it: impl Iterator<Item = (Self::Value, Self::Value)>,
565569
) -> Result<Self::Value>;
566570

571+
/// Install partially-built containers in the marshal reference table
572+
/// before reading their children, as CPython's `r_object()` does.
573+
/// Runtime bags can opt in; constant bags retain collect-then-construct.
574+
fn make_tuple_placeholder(&self, _len: usize) -> Option<Self::Value> {
575+
None
576+
}
577+
578+
fn set_tuple_item(
579+
&self,
580+
_tuple: &Self::Value,
581+
_index: usize,
582+
_value: Self::Value,
583+
) -> Result<()> {
584+
Err(MarshalError::BadType)
585+
}
586+
587+
fn make_list_placeholder(&self, _len: usize) -> Option<Self::Value> {
588+
None
589+
}
590+
591+
fn set_list_item(&self, _list: &Self::Value, _index: usize, _value: Self::Value) -> Result<()> {
592+
Err(MarshalError::BadType)
593+
}
594+
595+
fn make_set_placeholder(&self) -> Option<Self::Value> {
596+
None
597+
}
598+
599+
fn insert_set_item(&self, _set: &Self::Value, _value: Self::Value) -> Result<()> {
600+
Err(MarshalError::BadType)
601+
}
602+
603+
fn make_dict_placeholder(&self) -> Option<Self::Value> {
604+
None
605+
}
606+
607+
fn insert_dict_item(
608+
&self,
609+
_dict: &Self::Value,
610+
_key: Self::Value,
611+
_value: Self::Value,
612+
) -> Result<()> {
613+
Err(MarshalError::BadType)
614+
}
615+
567616
fn make_slice(
568617
&self,
569618
_start: Self::Value,
@@ -755,7 +804,7 @@ fn deserialize_value_after_header<R: Read, Bag: MarshalBag>(
755804
let code = deserialize_code_inner(rdr, bag.constant_bag(), depth - 1, &mut inner_refs)?;
756805
bag.make_code(code)
757806
} else {
758-
deserialize_value_typed(rdr, bag, depth, refs, typ)?
807+
deserialize_value_typed(rdr, bag, depth, refs, typ, slot)?
759808
};
760809

761810
if let Some(idx) = slot {
@@ -770,6 +819,7 @@ fn deserialize_value_typed<R: Read, Bag: MarshalBag>(
770819
depth: usize,
771820
refs: &mut Vec<Option<Bag::Value>>,
772821
typ: Type,
822+
slot: Option<usize>,
773823
) -> Result<Bag::Value> {
774824
if depth == 0 {
775825
return Err(MarshalError::InvalidBytecode);
@@ -806,21 +856,42 @@ fn deserialize_value_typed<R: Read, Bag: MarshalBag>(
806856
let value = Complex64 { re, im };
807857
bag.make_complex(value)
808858
}
809-
Type::Ascii | Type::AsciiInterned | Type::Unicode | Type::Interned => {
859+
Type::Ascii | Type::Unicode => {
810860
let len = rdr.read_u32()?;
811861
let value = rdr.read_wtf8(len)?;
812862
bag.make_str(value)
813863
}
814-
Type::ShortAscii | Type::ShortAsciiInterned => {
864+
Type::AsciiInterned | Type::Interned => {
865+
let len = rdr.read_u32()?;
866+
let value = rdr.read_wtf8(len)?;
867+
bag.make_interned_str(value)
868+
}
869+
Type::ShortAscii => {
815870
let len = rdr.read_u8()? as u32;
816871
let value = rdr.read_wtf8(len)?;
817872
bag.make_str(value)
818873
}
874+
Type::ShortAsciiInterned => {
875+
let len = rdr.read_u8()? as u32;
876+
let value = rdr.read_wtf8(len)?;
877+
bag.make_interned_str(value)
878+
}
819879
Type::SmallTuple => {
820880
let len = rdr.read_u8()? as usize;
821881
let d = depth - 1;
822-
let it = (0..len).map(|_| deserialize_value_depth(rdr, bag, d, refs));
823-
itertools::process_results(it, |it| bag.make_tuple(it))?
882+
if let Some(index) = slot
883+
&& let Some(tuple) = bag.make_tuple_placeholder(len)
884+
{
885+
refs[index] = Some(tuple.clone());
886+
for item_index in 0..len {
887+
let item = deserialize_value_depth(rdr, bag, d, refs)?;
888+
bag.set_tuple_item(&tuple, item_index, item)?;
889+
}
890+
tuple
891+
} else {
892+
let it = (0..len).map(|_| deserialize_value_depth(rdr, bag, d, refs));
893+
itertools::process_results(it, |it| bag.make_tuple(it))?
894+
}
824895
}
825896
Type::Null => {
826897
return Err(MarshalError::BadType);
@@ -830,22 +901,55 @@ fn deserialize_value_typed<R: Read, Bag: MarshalBag>(
830901
return Err(MarshalError::BadType);
831902
}
832903
Type::Tuple => {
833-
let len = rdr.read_u32()?;
904+
let len = rdr.read_u32()? as usize;
834905
let d = depth - 1;
835-
let it = (0..len).map(|_| deserialize_value_depth(rdr, bag, d, refs));
836-
itertools::process_results(it, |it| bag.make_tuple(it))?
906+
if let Some(index) = slot
907+
&& let Some(tuple) = bag.make_tuple_placeholder(len)
908+
{
909+
refs[index] = Some(tuple.clone());
910+
for item_index in 0..len {
911+
let item = deserialize_value_depth(rdr, bag, d, refs)?;
912+
bag.set_tuple_item(&tuple, item_index, item)?;
913+
}
914+
tuple
915+
} else {
916+
let it = (0..len).map(|_| deserialize_value_depth(rdr, bag, d, refs));
917+
itertools::process_results(it, |it| bag.make_tuple(it))?
918+
}
837919
}
838920
Type::List => {
839-
let len = rdr.read_u32()?;
921+
let len = rdr.read_u32()? as usize;
840922
let d = depth - 1;
841-
let it = (0..len).map(|_| deserialize_value_depth(rdr, bag, d, refs));
842-
itertools::process_results(it, |it| bag.make_list(it))??
923+
if let Some(index) = slot
924+
&& let Some(list) = bag.make_list_placeholder(len)
925+
{
926+
refs[index] = Some(list.clone());
927+
for item_index in 0..len {
928+
let item = deserialize_value_depth(rdr, bag, d, refs)?;
929+
bag.set_list_item(&list, item_index, item)?;
930+
}
931+
list
932+
} else {
933+
let it = (0..len).map(|_| deserialize_value_depth(rdr, bag, d, refs));
934+
itertools::process_results(it, |it| bag.make_list(it))??
935+
}
843936
}
844937
Type::Set => {
845-
let len = rdr.read_u32()?;
938+
let len = rdr.read_u32()? as usize;
846939
let d = depth - 1;
847-
let it = (0..len).map(|_| deserialize_value_depth(rdr, bag, d, refs));
848-
itertools::process_results(it, |it| bag.make_set(it))??
940+
if let Some(index) = slot
941+
&& let Some(set) = bag.make_set_placeholder()
942+
{
943+
refs[index] = Some(set.clone());
944+
for _ in 0..len {
945+
let item = deserialize_value_depth(rdr, bag, d, refs)?;
946+
bag.insert_set_item(&set, item)?;
947+
}
948+
set
949+
} else {
950+
let it = (0..len).map(|_| deserialize_value_depth(rdr, bag, d, refs));
951+
itertools::process_results(it, |it| bag.make_set(it))??
952+
}
849953
}
850954
Type::FrozenSet => {
851955
let len = rdr.read_u32()?;
@@ -855,17 +959,33 @@ fn deserialize_value_typed<R: Read, Bag: MarshalBag>(
855959
}
856960
Type::Dict => {
857961
let d = depth - 1;
858-
let mut pairs = Vec::new();
859-
loop {
860-
let raw = rdr.read_u8()?;
861-
if raw & !FLAG_REF == b'0' {
862-
break;
962+
if let Some(index) = slot
963+
&& let Some(dict) = bag.make_dict_placeholder()
964+
{
965+
refs[index] = Some(dict.clone());
966+
loop {
967+
let raw = rdr.read_u8()?;
968+
if raw & !FLAG_REF == b'0' {
969+
break;
970+
}
971+
let key = deserialize_value_after_header(rdr, bag, d, refs, raw)?;
972+
let value = deserialize_value_depth(rdr, bag, d, refs)?;
973+
bag.insert_dict_item(&dict, key, value)?;
974+
}
975+
dict
976+
} else {
977+
let mut pairs = Vec::new();
978+
loop {
979+
let raw = rdr.read_u8()?;
980+
if raw & !FLAG_REF == b'0' {
981+
break;
982+
}
983+
let key = deserialize_value_after_header(rdr, bag, d, refs, raw)?;
984+
let value = deserialize_value_depth(rdr, bag, d, refs)?;
985+
pairs.push((key, value));
863986
}
864-
let k = deserialize_value_after_header(rdr, bag, d, refs, raw)?;
865-
let v = deserialize_value_depth(rdr, bag, d, refs)?;
866-
pairs.push((k, v));
987+
bag.make_dict(pairs.into_iter())?
867988
}
868-
bag.make_dict(pairs.into_iter())?
869989
}
870990
Type::Bytes => {
871991
// After marshaling, byte arrays are converted into bytes.

0 commit comments

Comments
 (0)