Fix import ctypes - #6304
Conversation
Sorry, something went wrong.
|
Caution Review failedThe pull request is closed. WalkthroughUnix library loader now accepts None to obtain the current process handle; ctypes gains cached pointer-type creation and pointer-instantiation APIs exposed as POINTER/pointer; PyCSimpleType.from_param added; PyCFuncPtr constructor refactored to support empty, address, and (name,dll) forms. 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)
📜 Recent review details Configuration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📥 CommitsReviewing files that changed from the base of the PR and between 469c93a and 7c66122. ⛔ Files ignored due to path filters (1)
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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
| 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")) |
Sorry, something went wrong.
| // 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")); |
Sorry, something went wrong.
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) }
crates/vm/src/stdlib/ctypes.rs (1)📜 Review details245-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 through vm context directly if available.
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between fef9de2 and 469c93a.
⛔ Files ignored due to path filters (1)- Lib/ctypes/__init__.py is excluded by !Lib/**
- 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)
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style in Rust code (cargo fmt to 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.rs
- crates/vm/src/stdlib/ctypes/base.rs
- crates/vm/src/stdlib/ctypes.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.rs
- crates/vm/src/stdlib/ctypes/base.rs
- crates/vm/src/stdlib/ctypes.rs
crates/vm/src/stdlib/ctypes/base.rs (2)⏰ 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)
- py_new (214-242)
- new (167-173)
- 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)
crates/vm/src/stdlib/ctypes/base.rs (1)crates/vm/src/stdlib/ctypes.rs (2)175-188: LGTM! The from_param implementation 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_param
crates/vm/src/stdlib/ctypes/function.rs (1)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 when name is None, 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
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 pattern format!("{}\0", &name) followed by lib.get(terminated.as_bytes()) is valid and may be preferable to the suggested name.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.
Sorry, something went wrong.
| 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.
⚠️ Potential issue | 🟠 Major
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 {‼️ 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)); | |
| } |
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.
Sorry, something went wrong.
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.