Add call functions to c-api#7937
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds four new C-ABI exported functions ( ChangesC-API object calling
Sequence DiagramsequenceDiagram
participant CCaller as External C Code
participant CallVec as PyObject_Vectorcall
participant CallMethod as PyObject_VectorcallMethod
participant VM as Rust VM
CCaller->>CallVec: callable, args_ptr, nargsf, kwnames
CallVec->>CallVec: extract pos_count from nargsf (nargsf - offset)
CallVec->>CallVec: optionally decode kwnames into keyword slice
CallVec->>CallVec: materialize args_ptr into Rust vector
CallVec->>VM: vectorcall(callable, posargs, kwnames)
VM-->>CallVec: PyObject* result
CallVec-->>CCaller: result
CCaller->>CallMethod: name, args_ptr, nargsf, kwnames
CallMethod->>CallMethod: validate args_len > 0 (receiver present)
CallMethod->>CallMethod: extract receiver from args[0]
CallMethod->>CallMethod: resolve attribute by name to callable
CallMethod->>CallVec: call with receiver removed from args
CallVec->>VM: vectorcall(method, adjusted_args, kwnames)
VM-->>CallVec: PyObject* result
CallVec-->>CallMethod: result
CallMethod-->>CCaller: result
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@crates/capi/src/abstract_.rs`:
- Around line 50-54: The code constructs a slice from a potentially NULL pointer
when computing args: change the logic in the vectorcall handler so that if
args_len == 0 you directly return an empty Vec, otherwise call unsafe
slice::from_raw_parts(args, args_len) and map/collect as before; specifically
update the block that references args_len, num_positional_args, kwnames and args
to short-circuit on args_len == 0 (mirror the pattern used by
PyObject_VectorcallMethod) to avoid undefined behavior from
slice::from_raw_parts on a NULL pointer.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 3028c37c-9c9d-4595-9896-6b445ccdc4cf
📒 Files selected for processing (2)
crates/capi/src/abstract_.rscrates/vm/src/function/argument.rs
4241c83 to
84d90d6
Compare
84d90d6 to
9530575
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/capi/src/abstract_.rs (1)
91-101:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing keyword argument value count in
args_lencalculation causes out-of-bounds read.The
args_lenat line 91 extracts only the positional argument count (nargsf & !PY_VECTORCALL_ARGUMENTS_OFFSET), but per the vectorcall protocol, the rawargsarray contains both positional arguments and keyword argument values in sequence:[receiver, pos_arg_1, ..., pos_arg_N, kwarg_val_1, ..., kwarg_val_M].Creating the slice with only the positional count omits the keyword argument values. When the slice pointer is passed to
PyObject_Vectorcallalong with the originalkwnames, that function attempts to read keyword argument values beyond the slice boundary, causing undefined behavior.The fix is to compute
args_lento include both positional and keyword argument value counts, mirroring the calculation inPyObject_Vectorcallat line 68.Proposed fix: include kwarg values in args_len
- let args_len = nargsf & !PY_VECTORCALL_ARGUMENTS_OFFSET; + let num_positional_args = nargsf & !PY_VECTORCALL_ARGUMENTS_OFFSET; + + let kwnames_len = unsafe { + kwnames + .as_ref() + .map(|tuple| tuple.try_downcast_ref::<PyTuple>(vm).map(|t| t.len())) + .transpose()? + .unwrap_or(0) + }; + + let args_len = num_positional_args + kwnames_len;🤖 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/capi/src/abstract_.rs` around lines 91 - 101, The slice length calculation for args_len wrongly uses only positional count (nargsf & !PY_VECTORCALL_ARGUMENTS_OFFSET), which omits keyword argument values and can cause out-of-bounds reads when PyObject_Vectorcall expects them; update the args_len computation to add the keyword value count when kwnames is non-null (mirror the same calculation used by PyObject_Vectorcall), i.e. derive the keyword count from nargsf (using PY_VECTORCALL_ARGUMENTS_OFFSET mask) and include it in args_len before creating the slice for receiver/args so the full [receiver, pos..., kw_vals...] region is covered.
🤖 Prompt for all review comments with 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.
Outside diff comments:
In `@crates/capi/src/abstract_.rs`:
- Around line 91-101: The slice length calculation for args_len wrongly uses
only positional count (nargsf & !PY_VECTORCALL_ARGUMENTS_OFFSET), which omits
keyword argument values and can cause out-of-bounds reads when
PyObject_Vectorcall expects them; update the args_len computation to add the
keyword value count when kwnames is non-null (mirror the same calculation used
by PyObject_Vectorcall), i.e. derive the keyword count from nargsf (using
PY_VECTORCALL_ARGUMENTS_OFFSET mask) and include it in args_len before creating
the slice for receiver/args so the full [receiver, pos..., kw_vals...] region is
covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: ae630f7a-1a5a-4707-8032-1e0a144ef418
📒 Files selected for processing (2)
crates/capi/src/abstract_.rscrates/vm/src/function/argument.rs
✅ Files skipped from review due to trivial changes (1)
- crates/vm/src/function/argument.rs
|
@coderabbitai your last comment is not correct. Because the pointer to pos_arg_1 points to the start of the array, and the array still contains the keywords arguments after it. It does not patter we do not count the kwargs in the slice. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 5025
In let (receiver, args) = unsafe { slice::from_raw_parts(args, args_len) }
.split_first()
.expect("args_len > 0 should guarantee a receiver");The resulting (ミ・ω・ミ) ✏️ Learnings added
|
Summary by CodeRabbit