Fix import ctypes #6304
Fix import ctypes #6304
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughUnix library loader now accepts Changes
Sequence DiagramsequenceDiagram
participant Caller
participant PyCFuncPtr as PyCFuncPtr::py_new
participant FormInspector as Form Branching
participant AddrHandler as Address Path
participant TupleHandler as Tuple (name,dll) Path
participant LibLoader as Library Loader
participant Result as PyCFuncPtr Construction
Caller->>PyCFuncPtr: call py_new(args)
PyCFuncPtr->>FormInspector: inspect args
alt args empty
FormInspector->>Result: create uninitialized PyCFuncPtr
else first arg is integer
FormInspector->>AddrHandler: extract address
AddrHandler->>Result: build PyCFuncPtr with pointer, name, handler
else first arg is tuple (name,dll)
FormInspector->>TupleHandler: extract name and dll
TupleHandler->>LibLoader: resolve library handle (load_library_unix / LoadLibrary)
LibLoader->>LibLoader: lookup symbol by name
LibLoader->>Result: return code pointer or None
Result->>Result: construct PyCFuncPtr with resolved data
else invalid form
FormInspector->>Result: return TypeError
end
Result-->>Caller: PyResult<PyCFuncPtr>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
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 |
crates/vm/src/stdlib/ctypes/base.rs
Outdated
| return PyCSimpleType::from_param(cls, as_parameter, vm); | ||
| } | ||
|
|
||
| Err(vm.new_type_error("wrong type".to_string())) |
There was a problem hiding this comment.
| Err(vm.new_type_error("wrong type".to_string())) | |
| Err(vm.new_type_error("wrong type")) |
crates/vm/src/stdlib/ctypes.rs
Outdated
| // If None, call libc::dlopen(null, mode) to get the current process handle | ||
| let handle = unsafe { libc::dlopen(std::ptr::null(), libc::RTLD_NOW) }; | ||
| if handle.is_null() { | ||
| return Err(vm.new_os_error("dlopen() error".to_string())); |
There was a problem hiding this comment.
| return Err(vm.new_os_error("dlopen() error".to_string())); | |
| return Err(vm.new_os_error("dlopen() error")); |
3e78ddf to
469c93a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/vm/src/stdlib/ctypes.rs (1)
192-205: Inconsistent error handling with Unix version.Line 203 uses
.unwrap()which will panic if library loading fails, while the Unix version on lines 220-222 properly maps the error to a Python OSError. Consider consistent error handling.fn load_library_windows( name: String, _load_flags: OptionalArg<i32>, vm: &VirtualMachine, ) -> PyResult<usize> { // TODO: audit functions first // TODO: load_flags let cache = library::libcache(); let mut cache_write = cache.write(); - let (id, _) = cache_write.get_or_insert_lib(&name, vm).unwrap(); + let (id, _) = cache_write + .get_or_insert_lib(&name, vm) + .map_err(|e| vm.new_os_error(e.to_string()))?; Ok(id) }
🧹 Nitpick comments (1)
crates/vm/src/stdlib/ctypes.rs (1)
245-283: Implementation is correct, but consider caching the module lookup.The function works correctly for creating and caching pointer types. However,
vm.import("_ctypes", 0)?is called on every invocation. For frequently-called code paths, this could be optimized by accessing the module throughvmcontext directly if available.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Lib/ctypes/__init__.pyis excluded by!Lib/**
📒 Files selected for processing (3)
crates/vm/src/stdlib/ctypes.rs(3 hunks)crates/vm/src/stdlib/ctypes/base.rs(1 hunks)crates/vm/src/stdlib/ctypes/function.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style in Rust code (cargo fmtto format)
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by your changes
Follow Rust best practices for error handling and memory management
Files:
crates/vm/src/stdlib/ctypes/function.rscrates/vm/src/stdlib/ctypes/base.rscrates/vm/src/stdlib/ctypes.rs
**/src/**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use the macro system (
pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
crates/vm/src/stdlib/ctypes/function.rscrates/vm/src/stdlib/ctypes/base.rscrates/vm/src/stdlib/ctypes.rs
🧬 Code graph analysis (1)
crates/vm/src/stdlib/ctypes/function.rs (1)
crates/vm/src/stdlib/ctypes/base.rs (2)
py_new(214-242)new(167-173)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
🔇 Additional comments (4)
crates/vm/src/stdlib/ctypes/base.rs (1)
175-188: LGTM! Thefrom_paramimplementation follows CPython's pattern correctly.The method properly handles:
- Direct instance check with early return
- Fallback to
_as_parameter_attribute lookup- Recursive conversion via
from_paramcrates/vm/src/stdlib/ctypes.rs (2)
225-232: LGTM! Proper handling for None to get current process handle.The implementation correctly uses
libc::dlopen(null, RTLD_NOW)to obtain the current process handle whennameisNone, with appropriate null-check error handling. The error message now matches the past review suggestion.
286-295: LGTM!Clean delegation pattern that correctly:
- Gets the object's type
- Creates the corresponding pointer type via
create_pointer_type- Instantiates the pointer with the object
crates/vm/src/stdlib/ctypes/function.rs (1)
195-201: The review comment is incorrect: trailing null termination is explicitly allowed and potentially beneficial in libloading.According to libloading's documentation,
Library::get()accepts symbol names with optional trailing NUL bytes (e.g.,b"foo\0"), and the trailing NUL is explicitly noted as potentially avoiding an allocation. The current code patternformat!("{}\0", &name)followed bylib.get(terminated.as_bytes())is valid and may be preferable to the suggestedname.as_bytes()for efficiency reasons.The concern that "the library expects a clean symbol name" is unfounded—trailing NUL bytes are explicitly documented as allowed and do not cause issues.
| if let Ok(addr) = first_arg.try_int(vm) { | ||
| let ptr_val = addr.as_bigint().to_usize().unwrap_or(0); | ||
| return Ok(Self { | ||
| ptr: PyRwLock::new(Some(CodePtr(ptr_val as *mut _))), | ||
| needs_free: AtomicCell::new(false), | ||
| arg_types: PyRwLock::new(None), | ||
| _flags_: AtomicCell::new(0), | ||
| res_type: PyRwLock::new(None), | ||
| name: PyRwLock::new(Some(format!("CFuncPtr@{:#x}", ptr_val))), | ||
| handler: vm.ctx.new_int(ptr_val).into(), | ||
| } | ||
| .to_pyobject(vm)); | ||
| } |
There was a problem hiding this comment.
Silent fallback to address 0 may mask invalid input.
When to_usize() fails (e.g., negative or oversized integer), the code silently uses address 0, which will likely cause a crash or undefined behavior when the function is called later. Consider returning an error instead.
if let Ok(addr) = first_arg.try_int(vm) {
- let ptr_val = addr.as_bigint().to_usize().unwrap_or(0);
+ let ptr_val = addr
+ .as_bigint()
+ .to_usize()
+ .ok_or_else(|| vm.new_value_error("Invalid function address".to_string()))?;
return Ok(Self {📝 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.
| if let Ok(addr) = first_arg.try_int(vm) { | |
| let ptr_val = addr.as_bigint().to_usize().unwrap_or(0); | |
| return Ok(Self { | |
| ptr: PyRwLock::new(Some(CodePtr(ptr_val as *mut _))), | |
| needs_free: AtomicCell::new(false), | |
| arg_types: PyRwLock::new(None), | |
| _flags_: AtomicCell::new(0), | |
| res_type: PyRwLock::new(None), | |
| name: PyRwLock::new(Some(format!("CFuncPtr@{:#x}", ptr_val))), | |
| handler: vm.ctx.new_int(ptr_val).into(), | |
| } | |
| .to_pyobject(vm)); | |
| } | |
| if let Ok(addr) = first_arg.try_int(vm) { | |
| let ptr_val = addr | |
| .as_bigint() | |
| .to_usize() | |
| .ok_or_else(|| vm.new_value_error("Invalid function address".to_string()))?; | |
| return Ok(Self { | |
| ptr: PyRwLock::new(Some(CodePtr(ptr_val as *mut _))), | |
| needs_free: AtomicCell::new(false), | |
| arg_types: PyRwLock::new(None), | |
| _flags_: AtomicCell::new(0), | |
| res_type: PyRwLock::new(None), | |
| name: PyRwLock::new(Some(format!("CFuncPtr@{:#x}", ptr_val))), | |
| handler: vm.ctx.new_int(ptr_val).into(), | |
| } | |
| .to_pyobject(vm)); | |
| } |
🤖 Prompt for AI Agents
In crates/vm/src/stdlib/ctypes/function.rs around lines 147-159, the code
currently uses addr.as_bigint().to_usize().unwrap_or(0) which silently falls
back to address 0 on conversion failure; instead match on to_usize() and when it
returns None return an Err created from the VM (e.g., vm.new_value_error or
vm.new_type_error) with a clear message about invalid pointer value, so the
function creation aborts rather than constructing a CFuncPtr@0.
469c93a to
7c66122
Compare
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.