Skip to content

Fix marshal recursive reference loading #8501

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

Merged
youknowone merged 4 commits into from
Aug 12, 2026
+366 −89
Merged

Fix marshal recursive reference loading #8501

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
df83bc2
Fix marshal recursive reference loading
youknowone Aug 12, 2026
c8ea55d
Preserve marshal container insertion errors
youknowone Aug 12, 2026
64d233f
Enable full abnormal marshal reference loop test
youknowone Aug 12, 2026
7371547
Test direct marshal tuple reference loop
youknowone Aug 12, 2026
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
21 changes: 7 additions & 14 deletions Lib/test/test_marshal.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,6 @@ def test_recursion_limit(self):
last.append([0])
self.assertRaises(ValueError, marshal.dumps, head)

@unittest.expectedFailure # TODO: RUSTPYTHON; ValueError: bad marshal data
def test_reference_loop_list(self):
a = []
a.append(a)
Expand All @@ -331,7 +330,6 @@ def test_reference_loop_list(self):
self.assertIsInstance(b, list)
self.assertIs(b[0], b)

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

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

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

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

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

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

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

@unittest.expectedFailure # TODO: RUSTPYTHON
def testIntern(self):
s = marshal.loads(marshal.dumps(self.strobj))
self.assertEqual(s, self.strobj)
Expand Down
168 changes: 144 additions & 24 deletions crates/compiler-core/src/marshal.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -516,7 +516,7 @@ fn read_const_value<R: Read, Bag: ConstantBag>(
let code = deserialize_code_inner(rdr, bag, depth - 1, refs)?;
bag.make_code(code)
} else {
deserialize_value_typed(rdr, bag, depth, refs, typ)?
deserialize_value_typed(rdr, bag, depth, refs, typ, slot)?
};
if let Some(idx) = slot {
refs[idx] = Some(value.clone());
Expand All @@ -540,6 +540,10 @@ pub trait MarshalBag: Copy {

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

fn make_interned_str(&self, value: &Wtf8) -> Self::Value {
self.make_str(value)
}

fn make_bytes(&self, value: &[u8]) -> Self::Value;

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

/// Install partially-built containers in the marshal reference table
/// before reading their children, as CPython's `r_object()` does.
/// Runtime bags can opt in; constant bags retain collect-then-construct.
fn make_tuple_placeholder(&self, _len: usize) -> Option<Self::Value> {
None
}

fn set_tuple_item(
&self,
_tuple: &Self::Value,
_index: usize,
_value: Self::Value,
) -> Result<()> {
Err(MarshalError::BadType)
}

fn make_list_placeholder(&self, _len: usize) -> Option<Self::Value> {
None
}

fn set_list_item(&self, _list: &Self::Value, _index: usize, _value: Self::Value) -> Result<()> {
Err(MarshalError::BadType)
}

fn make_set_placeholder(&self) -> Option<Self::Value> {
None
}

fn insert_set_item(&self, _set: &Self::Value, _value: Self::Value) -> Result<()> {
Err(MarshalError::BadType)
}

fn make_dict_placeholder(&self) -> Option<Self::Value> {
None
}

fn insert_dict_item(
&self,
_dict: &Self::Value,
_key: Self::Value,
_value: Self::Value,
) -> Result<()> {
Err(MarshalError::BadType)
}

fn make_slice(
&self,
_start: Self::Value,
Expand Down Expand Up @@ -755,7 +804,7 @@ fn deserialize_value_after_header<R: Read, Bag: MarshalBag>(
let code = deserialize_code_inner(rdr, bag.constant_bag(), depth - 1, &mut inner_refs)?;
bag.make_code(code)
} else {
deserialize_value_typed(rdr, bag, depth, refs, typ)?
deserialize_value_typed(rdr, bag, depth, refs, typ, slot)?
};

if let Some(idx) = slot {
Expand All @@ -770,6 +819,7 @@ fn deserialize_value_typed<R: Read, Bag: MarshalBag>(
depth: usize,
refs: &mut Vec<Option<Bag::Value>>,
typ: Type,
slot: Option<usize>,
) -> Result<Bag::Value> {
if depth == 0 {
return Err(MarshalError::InvalidBytecode);
Expand Down Expand Up @@ -806,21 +856,42 @@ fn deserialize_value_typed<R: Read, Bag: MarshalBag>(
let value = Complex64 { re, im };
bag.make_complex(value)
}
Type::Ascii | Type::AsciiInterned | Type::Unicode | Type::Interned => {
Type::Ascii | Type::Unicode => {
let len = rdr.read_u32()?;
let value = rdr.read_wtf8(len)?;
bag.make_str(value)
}
Type::ShortAscii | Type::ShortAsciiInterned => {
Type::AsciiInterned | Type::Interned => {
let len = rdr.read_u32()?;
let value = rdr.read_wtf8(len)?;
bag.make_interned_str(value)
}
Type::ShortAscii => {
let len = rdr.read_u8()? as u32;
let value = rdr.read_wtf8(len)?;
bag.make_str(value)
}
Type::ShortAsciiInterned => {
let len = rdr.read_u8()? as u32;
let value = rdr.read_wtf8(len)?;
bag.make_interned_str(value)
}
Type::SmallTuple => {
let len = rdr.read_u8()? as usize;
let d = depth - 1;
let it = (0..len).map(|_| deserialize_value_depth(rdr, bag, d, refs));
itertools::process_results(it, |it| bag.make_tuple(it))?
if let Some(index) = slot
&& let Some(tuple) = bag.make_tuple_placeholder(len)
{
refs[index] = Some(tuple.clone());
for item_index in 0..len {
let item = deserialize_value_depth(rdr, bag, d, refs)?;
bag.set_tuple_item(&tuple, item_index, item)?;
}
tuple
} else {
let it = (0..len).map(|_| deserialize_value_depth(rdr, bag, d, refs));
itertools::process_results(it, |it| bag.make_tuple(it))?
}
}
Type::Null => {
return Err(MarshalError::BadType);
Expand All @@ -830,22 +901,55 @@ fn deserialize_value_typed<R: Read, Bag: MarshalBag>(
return Err(MarshalError::BadType);
}
Type::Tuple => {
let len = rdr.read_u32()?;
let len = rdr.read_u32()? as usize;
let d = depth - 1;
let it = (0..len).map(|_| deserialize_value_depth(rdr, bag, d, refs));
itertools::process_results(it, |it| bag.make_tuple(it))?
if let Some(index) = slot
&& let Some(tuple) = bag.make_tuple_placeholder(len)
{
refs[index] = Some(tuple.clone());
for item_index in 0..len {
let item = deserialize_value_depth(rdr, bag, d, refs)?;
bag.set_tuple_item(&tuple, item_index, item)?;
}
tuple
} else {
let it = (0..len).map(|_| deserialize_value_depth(rdr, bag, d, refs));
itertools::process_results(it, |it| bag.make_tuple(it))?
}
}
Type::List => {
let len = rdr.read_u32()?;
let len = rdr.read_u32()? as usize;
let d = depth - 1;
let it = (0..len).map(|_| deserialize_value_depth(rdr, bag, d, refs));
itertools::process_results(it, |it| bag.make_list(it))??
if let Some(index) = slot
&& let Some(list) = bag.make_list_placeholder(len)
{
refs[index] = Some(list.clone());
for item_index in 0..len {
let item = deserialize_value_depth(rdr, bag, d, refs)?;
bag.set_list_item(&list, item_index, item)?;
}
list
} else {
let it = (0..len).map(|_| deserialize_value_depth(rdr, bag, d, refs));
itertools::process_results(it, |it| bag.make_list(it))??
}
Comment on lines 920 to +935

@coderabbitai coderabbitai Bot Aug 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Check for existing length guards in the marshal decoder and for documented untrusted-input policy.

rg -n -C4 'read_u32|MAX_MARSHAL|too (large|long)|InvalidBytecode' crates/compiler-core/src/marshal.rs | head -80

# Look for any documented stance on untrusted marshal data.
rg -rn -i -C3 'untrusted|malicious' --glob '*.rs' --glob '*.md' | rg -i marshal | head -40

Repository: RustPython/RustPython

Length of output: 3074

🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- marshal structure and placeholder implementations ---'
ast-grep outline crates/compiler-core/src/marshal.rs
rg -n -C8 'make_(list|tuple)_placeholder|fn deserialize_value|marshal|loads|MAX_MARSHAL_STACK_DEPTH' crates/compiler-core/src/marshal.rs crates -g '*.rs' | head -240
printf '%s\n' '--- list and tuple construction semantics ---'
rg -n -C6 'make_list_placeholder|make_tuple_placeholder|vec!\[none|with_capacity' crates -g '*.rs' | head -180

Repository: RustPython/RustPython

Length of output: 32918

🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- decoder branches ---'
sed -n '448,527p' crates/compiler-core/src/marshal.rs
sed -n '736,935p' crates/compiler-core/src/marshal.rs
printf '%s\n' '--- MarshalBag implementations and runtime allocation ---'
rg -n -C10 'impl .*MarshalBag|make_tuple_placeholder|make_list_placeholder|set_tuple_item|set_list_item' crates -g '*.rs'

Repository: RustPython/RustPython

Length of output: 21839

🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- marshal Python entry points and documentation ---'
rg -n -C8 'deserialize_value|deserialize_code|marshal::|loads|load\(' crates/vm/src/stdlib/marshal.rs crates/vm/src -g '*.rs' | head -260
rg -rn -i -C4 'marshal.*(trusted|untrusted|security|warning|unsafe)|untrusted.*marshal|marshal.*not.*secure|not.*secure.*marshal' . -g '*.rs' -g '*.md' -g '*.rst' | head -160
printf '%s\n' '--- allocation and error behavior relevant to the claimed failure ---'
sed -n '100,190p' crates/compiler-core/src/marshal.rs
sed -n '390,510p' crates/vm/src/stdlib/marshal.rs
rg -n -C5 'new_marshal_placeholder|struct PyList|struct PyTuple|pub fn new_list|pub fn new_tuple' crates/vm/src crates -g '*.rs' | head -220

Repository: RustPython/RustPython

Length of output: 37737

🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- complete marshal load implementation ---'
rg -n 'fn (load|loads)|deserialize_value|deserialize_code|MarshalError' crates/vm/src/stdlib/marshal.rs
sed -n '550,760p' crates/vm/src/stdlib/marshal.rs
printf '%s\n' '--- placeholder constructors ---'
sed -n '25,75p' crates/vm/src/builtins/tuple.rs
sed -n '330,365p' crates/vm/src/builtins/tuple.rs
rg -n -C5 'pub struct PyList|impl From<Vec|borrow_vec_mut|fn from' crates/vm/src/builtins/list.rs | head -140

Repository: RustPython/RustPython

Length of output: 14561

🏁 Script executed:

#!/bin/bash
set -e
python3 - <<'PY'
import pathlib, struct

marshal = pathlib.Path("crates/compiler-core/src/marshal.rs").read_text()
vm_marshal = pathlib.Path("crates/vm/src/stdlib/marshal.rs").read_text()

assert "let len = rdr.read_u32()? as usize;" in marshal
assert "make_list_placeholder(len)" in marshal
assert "make_tuple_placeholder(len)" in marshal
assert "vec![self.vm.ctx.none(); len]" in vm_marshal
assert "fn loads(" in vm_marshal
assert "deserialize_value(&mut &buf[..], vm)" in vm_marshal

payload = b"[" + struct.pack("<I", 0xFFFF_FFFF)
length = struct.unpack_from("<I", payload, 1)[0]
print(f"TYPE_LIST payload length field: {length}")
print("placeholder allocation occurs before child decoding: true")
print("u32 length guard in the inspected decoder: false")
print("marshal.loads reaches the inspected decoder: true")
PY

Repository: RustPython/RustPython

Length of output: 359

🌐 Web query:

Python marshal documentation warning untrusted data marshal.loads malicious data

💡 Result:

The Python documentation explicitly warns that the marshal module is not intended to be secure against erroneous or maliciously constructed data [1][2]. You should never use marshal.loads to process data received from an untrusted or unauthenticated source [1][3]. The reasons for this security warning include: 1. Internal Design: The marshal module was not designed as a general-purpose persistence or data interchange format [4][5]. Its primary purpose is to support the reading and writing of pseudo-compiled code for Python modules (.pyc files) [4][5]. 2. Lack of Security Audits: The C code responsible for unmarshalling data has not been thoroughly analyzed or audited for security vulnerabilities such as buffer overflows [4][5]. Because it was not written with security in mind, it may be susceptible to hypothetical exploits similar to those historically found in complex file format parsers [4][5]. 3. Potential for Crashes and Hangs: Maliciously crafted marshal data can trigger significant issues, including segmentation faults (crashes) or denial-of-service conditions (e.g., causing the interpreter to hang or consume excessive resources) [4][5][6]. 4. No Security Promises: Python maintainers explicitly state that they cannot make guarantees about the safety or security of the marshal data format, as it is not fuzz-tested for security and lacks the rigorous hardening expected of tools meant for untrusted input [6]. In summary, because marshal lacks the necessary security safeguards, it should be treated similarly to pickle: it is safe only when used with strictly trusted data [4][6]. If your application must handle external or untrusted data, safer formats such as JSON should be used instead [4][5].

Citations:

Bound marshal container lengths before placeholder allocation

marshal.loads passes untrusted bytes to this decoder. List and tuple lengths are converted from u32 without a limit, and runtime placeholders allocate all elements before reading children. A truncated payload can therefore trigger excessive allocation or process termination instead of returning Eof. Reject lengths that exceed a safe decode budget before creating list or tuple placeholders.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/compiler-core/src/marshal.rs` around lines 920 - 935, Bound decoded
list and tuple lengths in the marshal deserialization logic before calling
make_list_placeholder or allocating tuple storage. Validate each u32-derived
length against the established safe decode budget, returning Eof for oversized
or truncated container payloads while preserving normal decoding for valid
lengths.

}
Type::Set => {
let len = rdr.read_u32()?;
let len = rdr.read_u32()? as usize;
let d = depth - 1;
let it = (0..len).map(|_| deserialize_value_depth(rdr, bag, d, refs));
itertools::process_results(it, |it| bag.make_set(it))??
if let Some(index) = slot
&& let Some(set) = bag.make_set_placeholder()
{
refs[index] = Some(set.clone());
for _ in 0..len {
let item = deserialize_value_depth(rdr, bag, d, refs)?;
bag.insert_set_item(&set, item)?;
}
set
} else {
let it = (0..len).map(|_| deserialize_value_depth(rdr, bag, d, refs));
itertools::process_results(it, |it| bag.make_set(it))??
}
}
Type::FrozenSet => {
let len = rdr.read_u32()?;
Expand All @@ -855,17 +959,33 @@ fn deserialize_value_typed<R: Read, Bag: MarshalBag>(
}
Type::Dict => {
let d = depth - 1;
let mut pairs = Vec::new();
loop {
let raw = rdr.read_u8()?;
if raw & !FLAG_REF == b'0' {
break;
if let Some(index) = slot
&& let Some(dict) = bag.make_dict_placeholder()
{
refs[index] = Some(dict.clone());
loop {
let raw = rdr.read_u8()?;
if raw & !FLAG_REF == b'0' {
break;
}
let key = deserialize_value_after_header(rdr, bag, d, refs, raw)?;
let value = deserialize_value_depth(rdr, bag, d, refs)?;
bag.insert_dict_item(&dict, key, value)?;
}
dict
} else {
let mut pairs = Vec::new();
loop {
let raw = rdr.read_u8()?;
if raw & !FLAG_REF == b'0' {
break;
}
let key = deserialize_value_after_header(rdr, bag, d, refs, raw)?;
let value = deserialize_value_depth(rdr, bag, d, refs)?;
pairs.push((key, value));
}
let k = deserialize_value_after_header(rdr, bag, d, refs, raw)?;
let v = deserialize_value_depth(rdr, bag, d, refs)?;
pairs.push((k, v));
bag.make_dict(pairs.into_iter())?
}
bag.make_dict(pairs.into_iter())?
}
Type::Bytes => {
// After marshaling, byte arrays are converted into bytes.
Expand Down
Loading
Loading