crates/vm/src/stdlib/ctypes/field.rs (2)

106-125: Consider extracting duplicated buffer access logic.

The structure and union branches have identical logic for reading from the cdata buffer. This could be refactored to reduce duplication, perhaps by extracting a helper that takes the buffer reference.

-        // Instance attribute access - read value from the structure/union's buffer
-        if let Some(structure) = obj.downcast_ref::<PyCStructure>() {
-            let cdata = structure.cdata.read();
-            let offset = zelf.byte_offset;
-            let size = zelf.byte_size;
-
-            if offset + size <= cdata.buffer.len() {
-                let bytes = &cdata.buffer[offset..offset + size];
-                return PyCField::bytes_to_value(bytes, size, vm);
-            }
-        } else if let Some(union) = obj.downcast_ref::<PyCUnion>() {
-            let cdata = union.cdata.read();
-            let offset = zelf.byte_offset;
-            let size = zelf.byte_size;
-
-            if offset + size <= cdata.buffer.len() {
-                let bytes = &cdata.buffer[offset..offset + size];
-                return PyCField::bytes_to_value(bytes, size, vm);
-            }
-        }
+        // Instance attribute access - read value from the structure/union's buffer
+        let cdata_guard = if let Some(structure) = obj.downcast_ref::<PyCStructure>() {
+            Some(structure.cdata.read())
+        } else if let Some(union) = obj.downcast_ref::<PyCUnion>() {
+            Some(union.cdata.read())
+        } else {
+            None
+        };
+
+        if let Some(cdata) = cdata_guard {
+            let offset = zelf.byte_offset;
+            let size = zelf.byte_size;
+            if offset + size <= cdata.buffer.len() {
+                let bytes = &cdata.buffer[offset..offset + size];
+                return PyCField::bytes_to_value(bytes, size, vm);
+            }
+        }

---

200-240: Duplicated write logic could be consolidated.

Similar to descr_get, the write logic is duplicated between structure and union branches. The differentiated delete error messages are appropriate, but the assign paths are identical.

crates/vm/src/stdlib/ctypes/structure.rs (1)

318-324: Add safety documentation for unsafe block.

The unsafe block reads from a raw pointer without any validation that the memory is valid or properly sized. While this is inherent to FFI, a safety comment documenting the requirements would be helpful.

         // Read data from address
         if address == 0 || size == 0 {
             return Err(vm.new_value_error("NULL pointer access".to_owned()));
         }
+        // SAFETY: Caller must ensure `address` points to valid memory of at least
+        // `size` bytes that remains valid for the duration of this read.
         let data = unsafe {
             let ptr = address as *const u8;
             std::slice::from_raw_parts(ptr, size).to_vec()
         };

crates/vm/src/stdlib/ctypes/union.rs (1)

87-111: Duplicated get_field_size implementation.

This method is identical to PyCStructType::get_field_size in structure.rs. Consider extracting to a shared helper function to avoid code duplication.

crates/vm/src/stdlib/ctypes/base.rs (2)

563-591: Verify the safety documentation for from_address.

The unsafe block reads from an arbitrary memory address. While this matches CPython's behavior, consider adding a # Safety doc comment explaining the preconditions (valid readable address, correct size) for maintainability.

---

641-641: Address the TODO: Missing reference tracking could cause use-after-free.

The from_buffer method creates a ctypes instance backed by the source buffer but doesn't store a reference to keep the buffer alive. If the source is garbage-collected while the ctypes instance is still in use, it could lead to undefined behavior.

Would you like me to open an issue to track implementing proper reference tracking via _objects?

crates/vm/src/stdlib/ctypes/array.rs (1)

486-497: Consider validating _type_ attribute exists before downcast.

The _type_ attribute retrieval could fail silently if the class doesn't have this attribute, but the subsequent downcast provides an error. The error message could be more specific.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Learnt from: youknowone
Repo: RustPython/RustPython PR: 6243
File: vm/src/function/buffer.rs:123-129
Timestamp: 2025-11-10T06:27:41.954Z
Learning: In vm/src/function/buffer.rs, line 36 in the try_rw_bytes_like method intentionally uses TypeError (not BufferError) for the "buffer is not a read-write bytes-like object" error case, even though a similar error message in ArgMemoryBuffer::try_from_borrowed_object uses BufferError. The TypeError is the intended exception type for this specific code path.

Learnt from: youknowone
Repo: RustPython/RustPython PR: 6110
File: vm/src/frame.rs:1311-1316
Timestamp: 2025-08-26T05:20:54.540Z
Learning: In the RustPython codebase, only certain builtin types should be marked with the SEQUENCE flag for pattern matching. List and tuple are sequences, but bytes, bytearray, and range are not considered sequences in this context, even though they may implement sequence-like protocols.

Learnt from: youknowone
Repo: RustPython/RustPython PR: 6110
File: vm/src/frame.rs:1311-1316
Timestamp: 2025-08-26T05:20:54.540Z
Learning: In RustPython's pattern matching implementation, only certain builtin types should have the SEQUENCE flag: list and tuple are confirmed sequences. The user youknowone indicated that bytes, bytearray are not considered sequences in this context, even though they implement sequence-like protocols.

crates/vm/src/stdlib/ctypes/array.rs (1)

36-67: CDataObject integration for arrays and the to_arg fix look correct

• Type and instance ctors now consistently wrap the underlying Vec in CDataObject, and PyCArrayType / PyCArray store the element type as PyObjectRef, which plays well with nested array types.
• to_arg now takes &cdata.buffer instead of &cdata.buffer.clone(), fixing the previous dangling-pointer issue by ensuring the libffi Arg points into the live backing buffer.
• ARRAY_BUFFER_METHODS and AsBuffer for PyCArray mirror the patterns used for simple types and structures, providing a writable view over cdata.buffer.

These changes align the array implementation with the new shared CData model and make the FFI path safer.

Also applies to: 211-219, 570-575, 578-612

crates/vm/src/stdlib/ctypes/base.rs (1)

14-15: Fix platform‑dependent size mismatches for c_wchar/c_long/c_ulong in value_to_bytes

The new value_to_bytes drives the backing CDataObject size for simple types, so size mistakes here now directly affect .cdata.buffer and the buffer protocol:

• "u" (c_wchar) always emits 4 bytes, while _ctypes::get_size("u") and _ctypes::bytes_to_pyobject use the native WideChar width. On Windows (2‑byte wchar_t) this will make c_wchar instances expose a 4‑byte buffer even though sizeof(c_wchar) is 2.
• "l"/"L" still hardcode 4‑byte layouts, the same issue previously raised for 64‑bit platforms where c_long/c_ulong are 8 bytes. With the new buffer backing this now also mis-sizes PyCSimple.cdata and AsBuffer views for these types.

Consider making these arms respect the native C sizes, e.g.:

-use std::ffi::{c_uint, c_ulong, c_ulonglong, c_ushort};
+use std::ffi::{c_long, c_uint, c_ulong, c_ulonglong, c_ushort};
+use widestring::WideChar;
@@
-        "u" => {
-            // c_wchar - 4 bytes (wchar_t on most platforms)
-            if let Ok(s) = value.str(vm)
-                && let Some(c) = s.as_str().chars().next()
-            {
-                return (c as u32).to_ne_bytes().to_vec();
-            }
-            vec![0; 4]
-        }
+        "u" => {
+            // c_wchar - native WideChar width
+            const SIZE: usize = std::mem::size_of::<WideChar>();
+            if let Ok(s) = value.str(vm)
+                && let Some(c) = s.as_str().chars().next()
+            {
+                let code = c as u32;
+                if SIZE == 2 {
+                    let w = code as u16;
+                    return w.to_ne_bytes().to_vec();
+                } else {
+                    return code.to_ne_bytes().to_vec();
+                }
+            }
+            vec![0; SIZE]
+        }
@@
-        "l" => {
-            // c_long (platform dependent, using 4 bytes)
-            if let Ok(int_val) = value.try_int(vm)
-                && let Some(v) = int_val.as_bigint().to_i32()
-            {
-                return v.to_ne_bytes().to_vec();
-            }
-            vec![0; 4]
-        }
-        "L" => {
-            // c_ulong (platform dependent, using 4 bytes)
-            if let Ok(int_val) = value.try_int(vm)
-                && let Some(v) = int_val.as_bigint().to_u32()
-            {
-                return v.to_ne_bytes().to_vec();
-            }
-            vec![0; 4]
-        }
+        "l" => {
+            // c_long (platform dependent)
+            let size = std::mem::size_of::<c_long>();
+            if let Ok(int_val) = value.try_int(vm) {
+                let i = int_val.as_bigint();
+                if size == 4 {
+                    if let Some(v) = i.to_i32() {
+                        return v.to_ne_bytes().to_vec();
+                    }
+                } else if size == 8 {
+                    if let Some(v) = i.to_i64() {
+                        return v.to_ne_bytes().to_vec();
+                    }
+                }
+            }
+            vec![0; size]
+        }
+        "L" => {
+            // c_ulong (platform dependent)
+            let size = std::mem::size_of::<c_ulong>();
+            if let Ok(int_val) = value.try_int(vm) {
+                let i = int_val.as_bigint();
+                if size == 4 {
+                    if let Some(v) = i.to_u32() {
+                        return v.to_ne_bytes().to_vec();
+                    }
+                } else if size == 8 {
+                    if let Some(v) = i.to_u64() {
+                        return v.to_ne_bytes().to_vec();
+                    }
+                }
+            }
+            vec![0; size]
+        }

This also addresses the earlier review about 4‑byte c_long/c_ulong while keeping behavior correct on both 32‑ and 64‑bit targets.

Also applies to: 277-420

crates/vm/src/stdlib/ctypes/union.rs (1)

133-153: Union.from_address still doesn’t read from the given address (and still uses size_of(Either::A(cls)))

This method is still constructing a zero-initialized buffer with CDataObject::new(size) and never reading from address, so the resulting union doesn’t reflect the memory at that location. It also uses size_of(Either::A(cls)), which is unlikely to hit the size_of_instances path on the type object.

Align it with the Structure.from_address pattern instead:

     #[pyclassmethod]
     fn from_address(cls: PyTypeRef, address: isize, vm: &VirtualMachine) -> PyResult {
         use crate::function::Either;
         use crate::stdlib::ctypes::_ctypes::size_of;
 
-        // Get size from cls
-        let size = size_of(Either::A(cls.clone()), vm)?;
-
-        // Create instance with data from address
+        // Get size from cls
+        let size = size_of(Either::B(cls.as_object().clone()), vm)?;
+
+        // Create instance with data from address
         if address == 0 || size == 0 {
             return Err(vm.new_value_error("NULL pointer access".to_owned()));
         }
 
-        Ok(PyCUnion {
-            cdata: PyRwLock::new(CDataObject::new(size)),
-        }
+        let data = unsafe {
+            let ptr = address as *const u8;
+            std::slice::from_raw_parts(ptr, size).to_vec()
+        };
+
+        Ok(PyCUnion {
+            cdata: PyRwLock::new(CDataObject::from_bytes(data, None)),
+        }
         .into_ref_with_type(vm, cls)?
         .into())
     }

This addresses the original concern and makes from_address actually reflect the pointed-to memory.

crates/vm/src/stdlib/ctypes/field.rs (1)

106-125: Union support in CField looks correct; consider surfacing OOB write errors

The new structure/union branches correctly use cdata and guard against offset + size > buffer.len(), matching the new buffer model. Right now, failed bounds checks just skip the write (and reads fall back to 0), which can make descriptor/size bugs hard to notice. If you ever see mis-sized fields at runtime, it may be worth turning these silent skips into explicit errors or at least debug assertions.

Also applies to: 200-238

crates/vm/src/stdlib/ctypes/pointer.rs (1)

30-37: Pointer constructors are safe and consistent with current .contents semantics

The new from_address / from_buffer / from_buffer_copy helpers perform the right null/offset/bounds checks and only stash the address as a Python int in contents, which is safe given that .contents is still marked “TODO: not correct”. Once you implement real pointee semantics, you’ll likely want these paths to also consult _type_ and keep a reference to the source buffer for from_buffer/from_buffer_copy, but for now the implementation is internally consistent.

Also applies to: 87-178

crates/vm/src/stdlib/ctypes/array.rs (1)

279-295: Avoid cloning the whole buffer in getitem_by_index for large arrays

getitem_by_index currently does:

let buffer = zelf.cdata.read().buffer.clone();
if offset + element_size <= buffer.len() {
    let bytes = &buffer[offset..offset + element_size];
    ...
}

This clones the entire backing buffer on every element access. For large arrays, it would be cheaper to slice directly under the read lock:

-        let buffer = zelf.cdata.read().buffer.clone();
-        if offset + element_size <= buffer.len() {
-            let bytes = &buffer[offset..offset + element_size];
+        let cdata = zelf.cdata.read();
+        if offset + element_size <= cdata.buffer.len() {
+            let bytes = &cdata.buffer[offset..offset + element_size];

The semantics stay the same, but you avoid an O(n) copy per access.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration using #ifdef checks rather than providing fallback values for other platforms.

Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration rather than providing fallback values for other platforms.

Learnt from: youknowone
Repo: RustPython/RustPython PR: 6243
File: vm/src/function/buffer.rs:123-129
Timestamp: 2025-11-10T06:27:41.954Z
Learning: In vm/src/function/buffer.rs, line 36 in the try_rw_bytes_like method intentionally uses TypeError (not BufferError) for the "buffer is not a read-write bytes-like object" error case, even though a similar error message in ArgMemoryBuffer::try_from_borrowed_object uses BufferError. The TypeError is the intended exception type for this specific code path.

Learnt from: youknowone
Repo: RustPython/RustPython PR: 6110
File: vm/src/frame.rs:1311-1316
Timestamp: 2025-08-26T05:20:54.540Z
Learning: In the RustPython codebase, only certain builtin types should be marked with the SEQUENCE flag for pattern matching. List and tuple are sequences, but bytes, bytearray, and range are not considered sequences in this context, even though they may implement sequence-like protocols.

Learnt from: youknowone
Repo: RustPython/RustPython PR: 6110
File: vm/src/frame.rs:1311-1316
Timestamp: 2025-08-26T05:20:54.540Z
Learning: In RustPython's pattern matching implementation, only certain builtin types should have the SEQUENCE flag: list and tuple are confirmed sequences. The user youknowone indicated that bytes, bytearray are not considered sequences in this context, even though they implement sequence-like protocols.

crates/vm/src/stdlib/ctypes/union.rs (2)

197-214: from_address doesn't read data from the provided memory address.

This implementation creates a zero-initialized buffer instead of reading the actual bytes at the given address. The stg_info is used to create an empty CDataObject, but the data at address is never read.

+        // SAFETY: Caller must ensure address points to valid memory of at least size bytes
+        let data = unsafe {
+            let ptr = address as *const u8;
+            std::slice::from_raw_parts(ptr, size).to_vec()
+        };
+
         Ok(PyCUnion {
-            cdata: PyRwLock::new(CDataObject::from_stg_info(&stg_info)),
+            cdata: PyRwLock::new(CDataObject::from_bytes(data, None)),
         }

---

250-259: from_buffer should preserve reference to source buffer.

The None passed to CDataObject::from_bytes means no reference to the source buffer is kept. If the source buffer is garbage collected, data integrity could be compromised for shared buffer scenarios.

-            cdata: PyRwLock::new(CDataObject::from_bytes(data, None)),
+            cdata: PyRwLock::new(CDataObject::from_bytes(data, Some(buffer.obj.clone()))),

crates/vm/src/stdlib/ctypes/array.rs (1)

750-755: Critical: to_arg still creates a dangling pointer.

This was flagged in a previous review. Arg::new(&cdata.buffer) borrows a reference to data inside the RwLockReadGuard, but the guard is dropped when the function returns, making the Arg hold a dangling reference.

The returned Arg will point to freed memory. The caller must ensure the buffer outlives the FFI call:

 impl PyCArray {
     #[allow(unused)]
-    pub fn to_arg(&self, _vm: &VirtualMachine) -> PyResult<libffi::middle::Arg> {
-        let cdata = self.cdata.read();
-        Ok(libffi::middle::Arg::new(&cdata.buffer))
+    pub fn to_arg(&self, _vm: &VirtualMachine) -> PyResult<(Vec<u8>, libffi::middle::Arg)> {
+        let buffer = self.cdata.read().buffer.clone();
+        // Caller must keep the Vec alive for the duration of the FFI call
+        let arg = libffi::middle::Arg::new(&buffer);
+        Ok((buffer, arg))
     }
 }

Note: This fix returns the owned buffer so the caller can keep it alive. The exact solution depends on how to_arg is used in FFI calls.

crates/vm/src/stdlib/ctypes/field.rs (2)

106-129: Silent fallback on bounds check failure may hide bugs.

When offset + size > cdata.buffer.len(), the code falls through to return vm.ctx.new_int(0) without any indication that the access was out of bounds. This could mask programming errors in field offset calculations.

Consider returning an error for out-of-bounds access:

             if offset + size <= cdata.buffer.len() {
                 let bytes = &cdata.buffer[offset..offset + size];
                 return PyCField::bytes_to_value(bytes, size, vm);
+            } else {
+                return Err(vm.new_value_error(format!(
+                    "field offset {} + size {} exceeds buffer length {}",
+                    offset, size, cdata.buffer.len()
+                )));
             }

---

200-240: Consider extracting common buffer write logic.

The PyCStructure and PyCUnion branches share nearly identical logic for writing to cdata.buffer. A helper method or macro could reduce duplication:

// Example helper approach
fn write_to_cdata_buffer(
    cdata: &mut CDataObject,
    offset: usize,
    size: usize,
    bytes: &[u8],
) -> bool {
    if offset + size <= cdata.buffer.len() {
        cdata.buffer[offset..offset + size].copy_from_slice(bytes);
        true
    } else {
        false
    }
}

crates/vm/src/stdlib/ctypes/base.rs (2)

794-798: Address TODO: Store buffer reference to prevent premature GC.

The TODO notes that from_buffer should store a reference to the source buffer in _objects. While the value is copied, failing to keep the source alive could cause issues if the ctypes object is expected to share memory with the original buffer (which is the semantic of from_buffer vs from_buffer_copy).

Consider implementing this before merge or tracking it as a follow-up issue.

Would you like me to open an issue to track implementing buffer reference storage?

---

959-978: Consider simplifying the double-map pattern.

The nested PyMappedRwLockReadGuard::map(PyRwLockReadGuard::map(...)) pattern is verbose. A simpler approach would be:

obj_bytes: |buffer| {
    rustpython_common::lock::PyRwLockReadGuard::map(
        buffer.obj_as::<PyCSimple>().cdata.read(),
        |x| x.buffer.as_slice(),
    )
    .into()
},

The intermediate map to &CDataObject before mapping to the slice appears redundant.

crates/vm/src/stdlib/ctypes/pointer.rs (2)

134-176: from_buffer reads pointer value correctly but doesn't track source reference.

The implementation correctly reads a pointer-sized value from the buffer. However, unlike CPython's from_buffer, this doesn't maintain a reference to the source buffer. For pointers this is less critical since only a usize is read, but for consistency with other ctypes, consider storing the source reference in CDataObject.objects.

---

249-259: Unsafe symbol access looks correct but lacks documentation.

The unsafe block correctly retrieves the symbol address from the library. The dereference *symbol gets the address stored in the symbol table. Consider adding a safety comment explaining why this is safe (library is locked, symbol lifetime is managed by libloading).

         let symbol_address = if let Some(lib) = &*inner_lib {
             unsafe {
+                // SAFETY: Library is held via lock, and symbol is valid for the lifetime
+                // of the library reference. We only read the address value.
                 // Try to get the symbol from the library
                 let symbol: Symbol<'_, *mut u8> = lib.get(symbol_name.as_bytes()).map_err(|e| {
                     vm.new_attribute_error(format!("{}: symbol '{}' not found", e, name.as_str()))
                 })?;
                 *symbol as usize
             }

crates/vm/src/stdlib/ctypes/structure.rs (1)

354-357: Unsafe memory read from arbitrary address - expected but dangerous.

This correctly implements CPython's from_address behavior, which is inherently unsafe. The caller is responsible for ensuring the address points to valid, properly-sized memory. Consider adding a safety comment documenting the contract.

         // Read data from address
         if address == 0 || size == 0 {
             return Err(vm.new_value_error("NULL pointer access".to_owned()));
         }
+        // SAFETY: Caller must ensure `address` points to valid, readable memory
+        // of at least `size` bytes. This matches CPython's from_address behavior.
         let data = unsafe {
             let ptr = address as *const u8;
             std::slice::from_raw_parts(ptr, size).to_vec()
         };

crates/vm/src/stdlib/ctypes/array.rs (1)

657-747: Duplicate in_dll implementation between PyCArrayType and PyCArray.

There are two in_dll implementations with nearly identical logic - one on PyCArrayType (lines 123-202) and one on PyCArray (lines 657-747). Consider consolidating to reduce code duplication.

The implementations are almost identical except for how they obtain element_type/length. Consider extracting the common DLL symbol resolution logic into a helper function.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Learnt from: youknowone
Repo: RustPython/RustPython PR: 6243
File: vm/src/function/buffer.rs:123-129
Timestamp: 2025-11-10T06:27:41.954Z
Learning: In vm/src/function/buffer.rs, line 36 in the try_rw_bytes_like method intentionally uses TypeError (not BufferError) for the "buffer is not a read-write bytes-like object" error case, even though a similar error message in ArgMemoryBuffer::try_from_borrowed_object uses BufferError. The TypeError is the intended exception type for this specific code path.

Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-25T11:05:02.900Z
Learning: When testing Python code, use RustPython (`cargo run -- script.py`) instead of the standard `python` command

Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-25T11:05:02.899Z
Learning: Applies to **/*test*.py : CRITICAL: Do not modify test assertions, test logic, or test data

Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration using #ifdef checks rather than providing fallback values for other platforms.

Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration rather than providing fallback values for other platforms.

Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-25T11:05:02.899Z
Learning: Applies to **/src/**/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust

Learnt from: youknowone
Repo: RustPython/RustPython PR: 6110
File: vm/src/frame.rs:1311-1316
Timestamp: 2025-08-26T05:20:54.540Z
Learning: In the RustPython codebase, only certain builtin types should be marked with the SEQUENCE flag for pattern matching. List and tuple are sequences, but bytes, bytearray, and range are not considered sequences in this context, even though they may implement sequence-like protocols.

crates/vm/src/stdlib/ctypes/thunk.rs (1)

59-64: Result buffer is left uninitialized when the Python callable raises

If the wrapped Python callable returns Err, python_to_ffi simply returns without touching result, so C callers see whatever garbage was in the result slot previously.

Even if the C side is expected to check for a Python exception, it’s safer and more deterministic to zero‑initialize the result buffer (according to the result type) before returning on error. This was already noted in earlier review; keeping it in mind while refining the thunk API would be beneficial.

crates/vm/src/stdlib/ctypes.rs (1)

763-810: _cast only mutates .contents and never touches pointer backing; semantics may diverge from CPython

pycfunction_cast constructs an instance of ctype and then implements cast by doing:

let result = ctype.call((), vm)?;
…
result.set_attr("contents", ptr_value, vm)?;

This treats cast essentially as “make a fresh ctypes instance and assign its contents attribute”:

• It doesn’t validate that ctype is actually a pointer type (only checks _type_ exists).
• It doesn’t interact with CDataObject / buffer storage, so any code that relies on pointer value being encoded in the backing cdata may not see the casted address.
• For non‑pointer types that define a contents attribute, this will behave oddly.

If you intend to match CPython’s ctypes.cast, consider:

• Verifying ctype is a subclass of _Pointer, not just something with _type_.
• Setting the pointer value in the underlying cdata representation instead of (or in addition to) assigning contents.
• Adding tests that mirror CPython’s ctypes.cast behaviour for ctypes instances vs integer addresses.

crates/vm/src/stdlib/ctypes/base.rs (2)

566-569: Pointer-like simple types expose zeroed buffers independent of their value

For "P" | "z" | "Z" the new value_to_bytes implementation currently returns:

// Pointer types (platform pointer size)
vec![0; std::mem::size_of::<usize>()]

ignoring the actual Python value. That means memoryviews and any buffer‑based consumers of _SimpleCData pointer types will always see a zeroed buffer, even when the object holds a non‑NULL address or string.

If you intend the buffer protocol to reflect the underlying C representation, consider encoding:

• For "P": the integer address (or 0 for None) as a native‑endian usize.
• For "z"/"Z": either the address of the underlying bytes/wchar buffer (if you model it), or at least an integer address consistent with bytes_to_pyobject/from_address.

---

628-672: Unsafe direct access to AtomicCell<PyObjectRef> is a concurrency footgun

PyCSimple::value clones the stored value via:

let raw_value = unsafe { (*zelf.value.as_ptr()).clone() };

This bypasses AtomicCell’s safe API and relies on manual unsafe pointer reads. While ctypes values are unlikely to be mutated concurrently in practice, this pattern is fragile and easy to get wrong if the representation evolves.

Consider replacing this with a safer container (e.g., PyRwLock<PyObjectRef>) or wrapping the unsafety into a small, well‑documented helper so that concurrent mutations and future refactors don’t accidentally introduce UB.

crates/vm/src/stdlib/ctypes/pointer.rs (1)

75-213: PyCPointer.contents mixes raw addresses and referent objects

The new constructor, __init__, from_address, from_buffer*, and in_dll all write different kinds of values into contents:

• py_new/__init__ store an arbitrary PyObjectRef (typically a ctypes instance).
• from_address/from_buffer*/in_dll store an integer address in the same field.

Downstream code will need to distinguish “pointer to object” vs “raw address” based solely on runtime type, which is error‑prone. It may be clearer to standardize representation (e.g., keep the raw address in cdata and treat contents purely as the dereferenced value, or vice versa) and have helpers consistently translate between the two.

crates/vm/src/stdlib/ctypes/structure.rs (1)

140-156: Structure layout tracks alignment but never pads fields or final size

get_field_align computes an alignment for each field (defaulting to 1), and PyCStructure::py_new keeps a max_align and stores it in StgInfo. However, field offsets are still computed as a simple running sum of sizes:

let mut offset = 0usize;
…
offset += size;
…
total_size = offset;
let mut stg_info = StgInfo::new(total_size, max_align);

There’s no per‑field padding to alignment boundaries and no rounding of total_size up to max_align, so the actual in‑memory layout is tightly packed regardless of the computed alignments. If you intend to approximate C struct layout (as CPython ctypes.Structure does), you’ll likely need to:

• Align each field’s offset up to its alignment, and
• Round the final total_size up to max_align.

Otherwise, the tracked alignment metadata may be misleading.

Also applies to: 245-295

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Learnt from: youknowone
Repo: RustPython/RustPython PR: 6243
File: vm/src/function/buffer.rs:123-129
Timestamp: 2025-11-10T06:27:41.954Z
Learning: In vm/src/function/buffer.rs, line 36 in the try_rw_bytes_like method intentionally uses TypeError (not BufferError) for the "buffer is not a read-write bytes-like object" error case, even though a similar error message in ArgMemoryBuffer::try_from_borrowed_object uses BufferError. The TypeError is the intended exception type for this specific code path.

Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration using #ifdef checks rather than providing fallback values for other platforms.

Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration rather than providing fallback values for other platforms.

Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-25T11:05:02.899Z
Learning: Applies to **/src/**/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust

crates/vm/src/stdlib/ctypes/union.rs (1)

198-214: from_address creates zeroed buffer instead of reading from address.

This implementation allocates a zero-initialized buffer rather than reading the actual data from the provided memory address. This is inconsistent with the expected behavior of from_address.

This issue was flagged in a previous review but appears to still be present. The implementation should read data from the address:

     fn from_address(cls: PyTypeRef, address: isize, vm: &VirtualMachine) -> PyResult {
         use crate::stdlib::ctypes::_ctypes::size_of;

         // Get size from cls
         let size = size_of(cls.clone().into(), vm)?;

         // Create instance with data from address
         if address == 0 || size == 0 {
             return Err(vm.new_value_error("NULL pointer access".to_owned()));
         }
-        let stg_info = StgInfo::new(size, 1);
+        
+        // SAFETY: Caller must ensure address points to valid memory
+        let data = unsafe {
+            let ptr = address as *const u8;
+            std::slice::from_raw_parts(ptr, size).to_vec()
+        };
+
         Ok(PyCUnion {
-            cdata: PyRwLock::new(CDataObject::from_stg_info(&stg_info)),
+            cdata: PyRwLock::new(CDataObject::from_bytes(data, None)),
         }

crates/vm/src/stdlib/ctypes/thunk.rs (1)

60-64: Exception in callable leaves FFI result uninitialized.

When the Python callable raises an exception, the function returns without writing to result, leaving uninitialized memory that the C caller may read.

This was flagged in a previous review. Consider zero-initializing:

     let obj = match obj {
         Ok(o) => o,
-        Err(_) => return, // Exception occurred, leave result as-is
+        Err(_) => {
+            // Zero-initialize result on error to avoid undefined behavior
+            // Note: We don't know the exact size, but zeroing a reasonable max is safer
+            unsafe { std::ptr::write_bytes(result, 0, std::mem::size_of::<u64>()); }
+            return;
+        }
     };

crates/vm/src/stdlib/ctypes/array.rs (1)

750-756: Critical: to_arg creates a dangling pointer.

The Arg::new(&cdata.buffer) borrows from cdata which is a RwLockReadGuard. The guard is dropped at the end of the function, but the returned Arg still holds a reference to the buffer data, causing undefined behavior.

This was flagged in a previous review. The Arg holds a pointer to data that will be deallocated when the lock guard is dropped.

 impl PyCArray {
     #[allow(unused)]
-    pub fn to_arg(&self, _vm: &VirtualMachine) -> PyResult<libffi::middle::Arg> {
-        let cdata = self.cdata.read();
-        Ok(libffi::middle::Arg::new(&cdata.buffer))
+    pub fn to_arg(&self, _vm: &VirtualMachine) -> PyResult<(Vec<u8>, libffi::middle::Arg)> {
+        let buffer = self.cdata.read().buffer.clone();
+        // SAFETY: Caller must ensure buffer outlives the Arg
+        let arg = unsafe { libffi::middle::Arg::new(&buffer) };
+        Ok((buffer, arg))
     }
 }

crates/vm/src/stdlib/ctypes.rs (1)

163-166: get_align assumes alignment equals size, which may be incorrect for some types.

For most simple C types this holds, but some platforms may have different alignment requirements (e.g., double on 32-bit x86 may have 4-byte alignment despite 8-byte size).

Consider documenting this assumption or using proper alignment intrinsics if precision is needed:

 /// Get alignment for a simple type - for C types, alignment equals size
+/// Note: This is a simplification that works for most platforms.
+/// Some exotic platforms may have different alignment requirements.
 pub fn get_align(ty: &str) -> usize {
     get_size(ty)
 }

crates/vm/src/stdlib/ctypes/pointer.rs (1)

75-89: Constructor and __init__ have overlapping initialization logic.

Both py_new (Constructor) and __init__ can set the contents value. When Python calls the constructor, py_new runs first setting contents, then __init__ runs potentially overwriting it with the same value. This is redundant but not incorrect.

Consider having py_new always initialize with None and letting __init__ handle the actual initialization:

 fn py_new(cls: PyTypeRef, args: Self::Args, vm: &VirtualMachine) -> PyResult {
-    // Get the initial contents value if provided
-    let initial_contents = args.0.into_option().unwrap_or_else(|| vm.ctx.none());
-
-    // Create a new PyCPointer instance with the provided value
     PyCPointer {
-        contents: PyRwLock::new(initial_contents),
+        contents: PyRwLock::new(vm.ctx.none()),
     }
     .into_ref_with_type(vm, cls)
     .map(Into::into)
 }

crates/vm/src/stdlib/ctypes/array.rs (1)

657-747: Significant code duplication with PyCArrayType::in_dll.

The PyCArray::in_dll implementation (lines 657-747) duplicates most of the logic from PyCArrayType::in_dll (lines 123-202). Both methods perform identical steps for handle extraction, library lookup, and symbol resolution.

Consider extracting the common logic into a shared helper function:

fn get_symbol_address(
    dll: PyObjectRef,
    name: &str,
    vm: &VirtualMachine,
) -> PyResult<usize> {
    // Common handle extraction and symbol lookup logic
    // ...
}

This would reduce duplication and make maintenance easier.

crates/vm/src/stdlib/ctypes/structure.rs (1)

372-464: Consider extracting common from_buffer* patterns.

The from_buffer, from_buffer_copy, and related methods follow the same pattern across PyCArray, PyCStructure, and PyCPointer:

1. Validate offset
2. Get buffer/size
3. Check buffer bounds
4. Read/copy data
5. Create instance

Consider creating helper functions in a utility module to reduce duplication:

// In util.rs
fn validate_buffer_offset(offset: isize) -> PyResult<usize> { ... }
fn check_buffer_bounds(buffer_len: usize, offset: usize, size: usize, vm: &VirtualMachine) -> PyResult<()> { ... }

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-25T11:05:02.899Z
Learning: Applies to **/src/**/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust

Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration using #ifdef checks rather than providing fallback values for other platforms.

Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration rather than providing fallback values for other platforms.

Learnt from: youknowone
Repo: RustPython/RustPython PR: 6243
File: vm/src/function/buffer.rs:123-129
Timestamp: 2025-11-10T06:27:41.954Z
Learning: In vm/src/function/buffer.rs, line 36 in the try_rw_bytes_like method intentionally uses TypeError (not BufferError) for the "buffer is not a read-write bytes-like object" error case, even though a similar error message in ArgMemoryBuffer::try_from_borrowed_object uses BufferError. The TypeError is the intended exception type for this specific code path.

Learnt from: youknowone
Repo: RustPython/RustPython PR: 6110
File: vm/src/frame.rs:1311-1316
Timestamp: 2025-08-26T05:20:54.540Z
Learning: In the RustPython codebase, only certain builtin types should be marked with the SEQUENCE flag for pattern matching. List and tuple are sequences, but bytes, bytearray, and range are not considered sequences in this context, even though they may implement sequence-like protocols.