Skip to content

Fix ctypes.py_object handling#7836

Merged
youknowone merged 2 commits into
RustPython:mainfrom
bschoenmaeckers:fix-ctypes-py_object
May 11, 2026
Merged

Fix ctypes.py_object handling#7836
youknowone merged 2 commits into
RustPython:mainfrom
bschoenmaeckers:fix-ctypes-py_object

Conversation

@bschoenmaeckers
Copy link
Copy Markdown
Contributor

@bschoenmaeckers bschoenmaeckers commented May 11, 2026

While testing the python c-api using ctypes in #7562, I stumbled on this bug. Previously all ctypes.py_object values were returned as None. After this fix the PythonAPITestCase.test_PyBytes_FromStringAndSize test case succeeds when PyBytes_FromStringAndSize is present in the c-api crate.

Summary by CodeRabbit

Release Notes

  • Bug Fixes
    • Enhanced ctypes py_object type support with improved Python object marshalling and NULL pointer validation during type conversions.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 11, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 4189000e-d0c7-4935-a679-7d5a8a3b2087

📥 Commits

Reviewing files that changed from the base of the PR and between 028b4c2 and 7639516.

📒 Files selected for processing (1)
  • crates/vm/src/stdlib/_ctypes/base.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • crates/vm/src/stdlib/_ctypes/base.rs

📝 Walkthrough

Walkthrough

Adds ctypes py_object ("O") support: marshals Python objects into FFI pointer arguments and deserializes raw pointer buffers back into PyObjectRef, with explicit NULL-pointer checks.

Changes

ctypes py_object Support

Layer / File(s) Summary
Argument Marshaling
crates/vm/src/stdlib/_ctypes/simple.rs
PyCSimpleType::from_param adds support for type code "O", wrapping Python objects into a CArgObject with FfiArgValue::Pointer and retaining the original object for lifetime management.
Buffer Deserialization
crates/vm/src/stdlib/_ctypes/base.rs
bytes_to_pyobject() adds a type "O" branch that reads a raw PyObject pointer, errors on NULL, and reconstructs a PyObjectRef from non-NULL pointers using from_raw in an unsafe block.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I hop through pointers, small and spry,
"O" brings objects back to eye,
From arg to buffer, round they go,
Safe checks catch the NULLy woe,
A rabbit nods — the bindings flow.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Fix ctypes.py_object handling' directly and specifically describes the main change in the pull request, which adds proper handling for ctypes py_object type across two files.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@ShaharNaveh ShaharNaveh left a comment

Choose a reason for hiding this comment

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

Thanks!

If this doesn't fix anything in CPython's test_ctypes, could you please add a test in extra_tests/ so we won't have a regression for it?

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

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/vm/src/stdlib/_ctypes/base.rs`:
- Around line 2345-2358: The "O" match arm in base.rs reconstructs a PyObject
via read_ptr_from_buffer and uses crate::PyObjectRef::from_raw +
core::mem::ManuallyDrop::new + clone which always increments the refcount but
never consumes ownership; fix this by distinguishing borrowed vs owned pointers
and handling owned C-API returns by taking ownership (call from_raw without
wrapping in ManuallyDrop so the destructor runs / triggers a Py_DECREF when
dropped) or explicitly calling the equivalent decref for owned pointers, while
keeping the current clone path for borrowed/stored pointers; update the code
around read_ptr_from_buffer, the "O" match arm, and uses of
crate::PyObjectRef::from_raw / ManuallyDrop accordingly, or alternatively add a
clear comment in that same location documenting the chosen memory model if you
opt not to change ownership handling.
🪄 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: 039f45e8-ef72-4edd-be30-4ddd8d5a3094

📥 Commits

Reviewing files that changed from the base of the PR and between 320355f and 028b4c2.

📒 Files selected for processing (2)
  • crates/vm/src/stdlib/_ctypes/base.rs
  • crates/vm/src/stdlib/_ctypes/simple.rs

Comment thread crates/vm/src/stdlib/_ctypes/base.rs
@bschoenmaeckers
Copy link
Copy Markdown
Contributor Author

bschoenmaeckers commented May 11, 2026

Thanks!

If this doesn't fix anything in CPython's test_ctypes, could you please add a test in extra_tests/ so we won't have a regression for it?

We do not have enough capi implemented to test this. After #7787 is merged, I will be able to add a testcase. And probably implement the bytes c-api and enable the PythonAPITestCase.test_PyBytes_FromStringAndSize testcase.

Copy link
Copy Markdown
Contributor

@ShaharNaveh ShaharNaveh left a comment

Choose a reason for hiding this comment

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

concerns addressed at #7836 (comment)

@youknowone
Copy link
Copy Markdown
Member

Unfortunately, test_ctypes is very poor state and writing own test for ctypes is reasonably painful.
We'd better to add a few more important integrated test to CI to prevent ctype regression

@youknowone youknowone merged commit 77070eb into RustPython:main May 11, 2026
25 checks passed
@bschoenmaeckers bschoenmaeckers deleted the fix-ctypes-py_object branch May 12, 2026 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants