Skip to content
25 changes: 25 additions & 0 deletions pyre/extra_tests/snippets/stdlib_marshal.py
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,31 @@ def test_roundtrip(self):
with self.assertRaises(ValueError):
marshal.dumps([orig], allow_code=False)

def test_argument_binding(self):
data = marshal.dumps([1, 2, 3])

# `version` is positional-only: it binds positionally but rejects the
# keyword form.
self.assertEqual(marshal.loads(marshal.dumps([1, 2, 3], 2)), [1, 2, 3])
with self.assertRaises(TypeError):
marshal.dumps([1], version=2)
# a non-integer version reaches int() and raises.
with self.assertRaises(TypeError):
marshal.dumps([1], None)

# `bytes` / `file` are positional-only too.
with self.assertRaises(TypeError):
marshal.loads(bytes=data)
Comment on lines +114 to +116

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add the load(file=...) assertion that the comment claims.

The comment states that bytes and file are positional-only, but only marshal.loads(bytes=data) is tested. Add the load case so the file boundary is covered.

💚 Proposed test addition
         # `bytes` / `file` are positional-only too.
         with self.assertRaises(TypeError):
             marshal.loads(bytes=data)
+        with self.assertRaises(TypeError):
+            marshal.load(file=BytesIO(data))

Import BytesIO in this test, as test_file_api does.

🧰 Tools
🪛 ast-grep (0.45.0)

[warning] 115-115: dill and marshal deserialize arbitrary objects/bytecode and execute code on untrusted input. Use a safe serialization format and never load untrusted data.
Context: marshal.loads(bytes=data)
Note: [CWE-502] Deserialization of Untrusted Data.

(dill-marshal-deserialization-python)

🪛 OpenGrep (1.26.0)

[WARNING] 116-116: marshal.loads() can execute arbitrary code during deserialization. Use a safe format like JSON instead.

(coderabbit.deserialization.python-marshal)

🪛 Ruff (0.16.1)

[warning] 115-115: Use pytest.raises instead of unittest-style assertRaises

Replace assertRaises with pytest.raises

(PT027)


[error] 116-116: Deserialization with the marshal module is possibly dangerous

(S302)

🤖 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 `@pyre/extra_tests/snippets/stdlib_marshal.py` around lines 114 - 116, Extend
the positional-only API assertions near the existing marshal.loads keyword test
to also cover marshal.load with a BytesIO file object passed by keyword. Import
and reuse BytesIO as done in test_file_api, and assert that
marshal.load(file=...) raises TypeError.


# `allow_code` is keyword-only and truth-tested, so a falsy value
# (including None) rejects a nested code object.
code = compile("1 + 1", "", "eval")
dumped_code = marshal.dumps([code])
with self.assertRaises(ValueError):
marshal.dumps([code], allow_code=None)
with self.assertRaises(ValueError):
marshal.loads(dumped_code, allow_code=False)


if __name__ == "__main__":
unittest.main()
16 changes: 16 additions & 0 deletions pyre/extra_tests/snippets/stdlib_struct.py
Original file line number Diff line number Diff line change
Expand Up @@ -102,3 +102,19 @@ def __index__(self):
unpack_iterator_type = type(struct.iter_unpack("B", b""))
with assert_raises(TypeError):
unpack_iterator_type()

# unpack_from accepts buffer / offset positionally or by keyword.
_buf = struct.pack("ii", 111, 222)
assert struct.unpack_from("ii", _buf, offset=0) == (111, 222)
assert struct.unpack_from("ii", buffer=_buf, offset=0) == (111, 222)
_s = struct.Struct("ii")
assert _s.unpack_from(_buf, offset=0) == (111, 222)
assert _s.unpack_from(buffer=_buf) == (111, 222)
Comment on lines +105 to +112

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Add the positional-only negative case for format.

The block proves that buffer and offset accept keywords. It does not prove that format rejects one. That assertion pins the posonly boundary direction for struct.unpack_from.

💚 Proposed additional case
 assert _s.unpack_from(buffer=_buf) == (111, 222)
+with assert_raises(TypeError):
+    struct.unpack_from(format="ii", buffer=_buf)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# unpack_from accepts buffer / offset positionally or by keyword.
_buf = struct.pack("ii", 111, 222)
assert struct.unpack_from("ii", _buf, offset=0) == (111, 222)
assert struct.unpack_from("ii", buffer=_buf, offset=0) == (111, 222)
_s = struct.Struct("ii")
assert _s.unpack_from(_buf, offset=0) == (111, 222)
assert _s.unpack_from(buffer=_buf) == (111, 222)
# unpack_from accepts buffer / offset positionally or by keyword.
_buf = struct.pack("ii", 111, 222)
assert struct.unpack_from("ii", _buf, offset=0) == (111, 222)
assert struct.unpack_from("ii", buffer=_buf, offset=0) == (111, 222)
_s = struct.Struct("ii")
assert _s.unpack_from(_buf, offset=0) == (111, 222)
assert _s.unpack_from(buffer=_buf) == (111, 222)
with assert_raises(TypeError):
struct.unpack_from(format="ii", buffer=_buf)
🤖 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 `@pyre/extra_tests/snippets/stdlib_struct.py` around lines 105 - 112, Add a
negative assertion to the unpack_from tests showing that passing format by
keyword raises the expected positional-only TypeError, while retaining the
existing valid positional and keyword cases for buffer and offset in the
struct.unpack_from block.


# pack / pack_into (module and method) reject keyword arguments.
with assert_raises(TypeError):
struct.pack(format="ii")
with assert_raises(TypeError):
struct.pack_into("ii", bytearray(8), 0, 1, 2, extra=3)
with assert_raises(TypeError):
_s.pack_into(bytearray(8), 0, 1, 2, extra=3)
Comment on lines +114 to +120

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add cases for omitted required arguments.

The new tests cover keyword rejection only. The binder pads omitted positionals with PY_NULL, so the missing-argument path is the risky one for the hand-registered vararg gateways. Add calls that omit required slots.

💚 Proposed additional cases
 with assert_raises(TypeError):
     _s.pack_into(bytearray(8), 0, 1, 2, extra=3)
+
+# pack / pack_into reject calls that omit required positional slots.
+with assert_raises(TypeError):
+    struct.pack()
+with assert_raises(TypeError):
+    struct.pack_into("ii", bytearray(8))
+with assert_raises(TypeError):
+    _s.pack_into()
+with assert_raises(TypeError):
+    _s.pack_into(bytearray(8))
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# pack / pack_into (module and method) reject keyword arguments.
with assert_raises(TypeError):
struct.pack(format="ii")
with assert_raises(TypeError):
struct.pack_into("ii", bytearray(8), 0, 1, 2, extra=3)
with assert_raises(TypeError):
_s.pack_into(bytearray(8), 0, 1, 2, extra=3)
# pack / pack_into (module and method) reject keyword arguments.
with assert_raises(TypeError):
struct.pack(format="ii")
with assert_raises(TypeError):
struct.pack_into("ii", bytearray(8), 0, 1, 2, extra=3)
with assert_raises(TypeError):
_s.pack_into(bytearray(8), 0, 1, 2, extra=3)
# pack / pack_into reject calls that omit required positional slots.
with assert_raises(TypeError):
struct.pack()
with assert_raises(TypeError):
struct.pack_into("ii", bytearray(8))
with assert_raises(TypeError):
_s.pack_into()
with assert_raises(TypeError):
_s.pack_into(bytearray(8))
🤖 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 `@pyre/extra_tests/snippets/stdlib_struct.py` around lines 114 - 120, Extend
the struct.pack and struct.pack_into tests to call each hand-registered gateway
with required positional arguments omitted, including the module and
bound-method variants represented by struct.pack, struct.pack_into, and
_s.pack_into. Assert that every incomplete call raises TypeError, while
preserving the existing keyword-rejection cases.

22 changes: 14 additions & 8 deletions pyre/pyre-interpreter/src/call.rs
Original file line number Diff line number Diff line change
Expand Up @@ -918,10 +918,16 @@ pub(crate) fn set_orig_class(
}
}

fn finish_builtin_code_positional(
/// Invoke a builtin from a slice of raw positional arguments, binding through
/// its `Signature` first when it has a HOPELESS fast arity. `builtin_code_call`
/// itself never binds — the direct call sites hand it an already-bound flat
/// slice — so any entry that starts from raw positionals (the frame dispatch
/// here, and the JIT residual-call path in `pyre-jit`) must route through this
/// to give a `*args`/optional-positional body the slot shape it reads.
pub fn builtin_code_call_positional(
current_code: PyObjectRef,
current_args: &[PyObjectRef],
) -> PyResult {
) -> Result<PyObjectRef, crate::PyError> {
if let Some(sig) = unsafe { crate::builtin_code_get_signature(current_code) } {
// Every HOPELESS signature needs `_match_signature`, not only
// *args/**kwargs/kw-only shapes. A plain optional positional
Expand All @@ -941,7 +947,7 @@ fn finish_builtin_code_positional(
fn call_builtin_code_many_from_roots(root_base: usize, nargs: usize) -> PyResult {
let mut rooted = vec![pyre_object::PY_NULL; 1 + nargs];
pyre_object::gc_roots::shadow_stack_copy_range(root_base, &mut rooted);
finish_builtin_code_positional(rooted[0], &rooted[1..])
builtin_code_call_positional(rooted[0], &rooted[1..])
}

fn call_builtin_code_positional(code: PyObjectRef, args: &[PyObjectRef]) -> PyResult {
Expand All @@ -963,28 +969,28 @@ fn call_builtin_code_positional(code: PyObjectRef, args: &[PyObjectRef]) -> PyRe
// indirect call. The uncommon variadic case stays a residual helper.
let current_code = _roots.get(root_base);
match args.len() {
0 => finish_builtin_code_positional(current_code, &[]),
0 => builtin_code_call_positional(current_code, &[]),
1 => {
let a0 = _roots.get(root_base + 1);
finish_builtin_code_positional(current_code, &[a0])
builtin_code_call_positional(current_code, &[a0])
}
2 => {
let a0 = _roots.get(root_base + 1);
let a1 = _roots.get(root_base + 2);
finish_builtin_code_positional(current_code, &[a0, a1])
builtin_code_call_positional(current_code, &[a0, a1])
}
3 => {
let a0 = _roots.get(root_base + 1);
let a1 = _roots.get(root_base + 2);
let a2 = _roots.get(root_base + 3);
finish_builtin_code_positional(current_code, &[a0, a1, a2])
builtin_code_call_positional(current_code, &[a0, a1, a2])
}
4 => {
let a0 = _roots.get(root_base + 1);
let a1 = _roots.get(root_base + 2);
let a2 = _roots.get(root_base + 3);
let a3 = _roots.get(root_base + 4);
finish_builtin_code_positional(current_code, &[a0, a1, a2, a3])
builtin_code_call_positional(current_code, &[a0, a1, a2, a3])
}
nargs => call_builtin_code_many_from_roots(root_base, nargs),
}
Expand Down
44 changes: 44 additions & 0 deletions pyre/pyre-interpreter/src/module/_random/macro_smoke.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,20 @@ fn _kwonly_bound_probe(
value + adjustment
}

/// A `#[posonly]` marker on the first non-positional-only parameter ends the
/// positional-only run before it: `base` is positional-only, so the derived
/// `Signature` carries `posonlyargcount == 1` and a keyword named `base` is
/// rejected by `raise_if_posonly_kwds`.
#[crate::pyre_function]
fn _posonly_bound_probe(
base: i64,
#[posonly]
#[default(0i64)]
offset: i64,
) -> i64 {
base + offset
}

crate::py_module! {
"_pyre_smoke",
interpleveldefs: {
Expand Down Expand Up @@ -184,6 +198,36 @@ mod tests {
assert_eq!(unsafe { w_int_get_value(result) }, 42);
}

#[test]
fn posonly_marker_makes_leading_param_positional_only() {
crate::typedef::init_typeobjects();
let signature = _posonly_bound_probe_pyre_sig().expect("derived signature");
assert_eq!(signature.posonlyargcount, 1);
assert_eq!(signature.argnames, vec!["base", "offset"]);

// The positional-only `base` binds fine by position, and `offset`
// still binds by keyword.
let bound = crate::call::bind_kwargs_to_signature(
&signature,
"_posonly_bound_probe",
&[w_int_new(40)],
&[(rustpython_wtf8::Wtf8Buf::from("offset"), w_int_new(2))],
)
.expect("signature binding");
let result = _posonly_bound_probe(&bound).expect("bound positional-only scope");
assert_eq!(unsafe { w_int_get_value(result) }, 42);

// Passing the positional-only `base` as a keyword is a TypeError.
let err = crate::call::bind_kwargs_to_signature(
&signature,
"_posonly_bound_probe",
&[],
&[(rustpython_wtf8::Wtf8Buf::from("base"), w_int_new(40))],
)
.expect_err("positional-only name as keyword must error");
assert_eq!(err.kind, crate::PyErrorKind::TypeError);
}

/// `Vec<i64>` return auto-wraps to a list.
#[test]
fn path_bytes_returns_list() {
Expand Down
Loading
Loading