gh-156933: Widen narrow integer results in ctypes callbacks by lazerg · Pull Request #157045 · python/cpython

lazerg · 2026-09-06T19:28:50Z

_CallPythonObject() only wrote restype->size bytes into the closure's result buffer, leaving the unused high-order bits of the ffi_arg-sized register untouched. libffi's ffi_prep_closure_loc() documents that integral types narrower than a machine register must be widened to fill it, sign-extending signed types. On architectures that always read the full register for narrow return values (s390x), this leaves garbage in the high bits, which broke libclang callbacks used by cindex.py.

The fix widens narrow integer results into a register-sized buffer before writing them back, sign- or zero-extending depending on the type, replacing the old big-endian-only pointer offset that didn't actually widen anything.

Fixes #156933.

Issue: _ctypes fails to extend the return value per libffi contract #156933

python-cla-bot · 2026-09-06T19:28:54Z

All commit authors signed the Contributor License Agreement.

lazerg · 2026-09-06T22:06:31Z

This failure is a 10 minute timeout in test_subprocess's test_check on the macOS Intel runner, unrelated to this change (which only touches _ctypes/callbacks.c).

ZeroIntensity

This needs a test case.

lazerg · 2026-09-20T01:14:23Z

I couldn't write a test that fails before this fix and passes after it on any CI architecture. The existing test_byte/test_short/test_int callback tests in test_callbacks.py already round-trip narrow return values through a real closure call, and they pass with or without this patch on x86-64 and arm64 — those backends already read back only the bytes they need, so the missing widening never surfaces there. The corruption only shows up on an ABI where the caller trusts the full register width, which in this report is s390x, not one of the PR-gating runners, and I don't have that hardware to verify against. I'm glad to add a value-check test for the narrow int types as basic coverage if you'd still like one, though it won't reproduce the bug itself on x86-64/arm64.

ZeroIntensity · 2026-09-20T01:54:51Z

I'm confused; how did you ensure that your fix works if you can't test on s390x? This feels a lot like you fed an issue into Codex/Claude/whatever, and you just sent the PR to us. Please don't try to fix issues that you can't even reproduce.

lazerg · 2026-09-20T03:54:22Z

It was AI-assisted, yes. I reviewed and drove the actual fix, and I get why the s390x question is fair.

Here's what I actually checked: the widening logic follows libffi's own contract for ffi_prep_closure_loc — narrow integer types have to fill a full ffi_arg-sized register, sign- or zero-extended. The old code just shifted a pointer for big-endian and never actually widened anything. I ran the full test_ctypes suite (628 tests) before and after on arm64, no regressions, and I also tried to reproduce the bug directly through register-priming plus a reinterpreted function pointer — it doesn't reproduce on arm64 or x86-64, because those libffi backends only read back the bytes they actually need. That's also why no portable regression test is possible on any architecture CI runs; s390x is the only one that trusts the full register.

If someone with s390x access can confirm the fix before/after, that's better evidence than anything I can produce here — happy to wait on that if you'd rather not merge on the reasoning alone.

Endilll · 2026-09-20T04:17:02Z

@miladfarca can you help testing this fix or find someone with the access to a s390x machine?

ZeroIntensity · 2026-09-20T15:47:22Z

I really don't appreciate you copy-pasting AI output as a response. Please use your own words.

I'm going to refrain from reviewing this until someone (preferably from the LLVM team) can confirm that this is the right solution for s390x. I don't completely understand this fix, and given that you can't even reproduce the bug locally, I'm not so sure that you do either.

miladfarca · 2026-09-25T14:58:03Z

I have created this small test case to test this PR, it fails without the patch and passes with it on s390x, I cannot run the entire test suite as I don't have the proper environment:

diff --git a/Lib/test/test_ctypes/test_callbacks.py b/Lib/test/test_ctypes/test_callbacks.py
index 6c7c2e52707..204eb6da301 100644
--- a/Lib/test/test_ctypes/test_callbacks.py
+++ b/Lib/test/test_ctypes/test_callbacks.py
@@ -328,6 +328,14 @@ def func():
                              f"of ctypes callback function {func!r}")
             self.assertIsNone(cm.unraisable.object)
 
+    def test_narrow_int_return_widened(self):
+        @CFUNCTYPE(c_int)
+        def cb():
+            return -1
+
+        addr = ctypes.cast(cb, ctypes.c_void_p).value
+        wide = CFUNCTYPE(c_longlong)(addr)
+        self.assertEqual(wide(), -1)
 
 if __name__ == '__main__':
     unittest.main()

lazerg · 2026-09-25T15:18:23Z

Thanks for testing this on s390x! I added your test in ebf7568. I put a 64-bit skip on it, because on 32-bit targets a long long return uses two registers and the callback only sets one. Here on arm64 macOS it passes with and without the fix, so s390x is still the only place where it catches the bug.

lazerg · 2026-09-25T15:52:51Z

I limited the test to s390x in 9338911. It failed on x86-64 CI even with the fix: libffi zero-extends a 32-bit result there, and the x86-64 ABI leaves the upper bits undefined, so reading an int result as long long is not reliable on that platform. On s390x the ABI requires the callee to extend the result, so the test is valid there and the s390x buildbots will run it.

ZeroIntensity · 2026-09-27T01:55:49Z

+Fix :mod:`ctypes` callbacks returning an integer narrower than a machine
+register: the result is now widened to fill the register, as libffi's
+closure contract requires.


This is too technical; we want changelog entries to be meaningful to users, not to CPython developers. Let's describe what bug is actually fixed by this. Something like "Fix incorrect integer return values from ctypes callbacks on some
platforms, such as s390x."

ZeroIntensity · 2026-09-27T01:56:47Z

+           setfunc() only writes restype->size bytes. Cf. _ctypes_callproc()
+           in callproc.c. */


I'm not sure I understand the last part of this comment:

Suggested change

setfunc() only writes restype->size bytes. Cf. _ctypes_callproc()

in callproc.c. */

setfunc() only writes restype->size bytes. */

ZeroIntensity · 2026-09-27T01:57:14Z

+                widened = (ffi_arg)(ffi_sarg)*(int8_t *)&widened;
+                break;
+            case FFI_TYPE_SINT16:
+                widened = (ffi_arg)(ffi_sarg)*(int16_t *)&widened;


Please fix the compiler warnings here.

Fixed in d54ca1f, it reads the narrow value back through a union now, so the strict-aliasing warnings are gone.

ZeroIntensity · 2026-09-27T02:03:29Z

+    def test_narrow_int_return_widened(self):
+        @CFUNCTYPE(c_int)


Could you add a comment linking to the issue here? Something like gh-156933: Narrow integers were not widened on s390x should work.

ZeroIntensity · 2026-09-27T02:09:34Z

                             f"of ctypes callback function {func!r}")
            self.assertIsNone(cm.unraisable.object)

+    @unittest.skipUnless(support.is_s390x, 's390x only test')


Ideally, this should run on all platforms.

ZeroIntensity · 2026-09-27T02:24:55Z

+        addr = ctypes.cast(cb, ctypes.c_void_p).value
+        wide = CFUNCTYPE(c_longlong)(addr)
+        self.assertEqual(wide(), -1)


We should come up with a test case that isn't dependent on the s390x ABI. I think we could do that by adding a function to _ctypes_test that takes a callback returning int as an argument, but returns it as a long long. Something like this:

EXPORT(long long) _testfunc_callback_int_to_longlong(int (*func)(void)) { return func(); }

Then we call this via a CDLL.

Added in d54ca1f and it runs everywhere now. It can only fail on s390x though, since on x86-64 and arm64 the C caller extends the int to long long itself.

ZeroIntensity · 2026-09-27T02:27:05Z

Thanks @miladfarca for confirming. I think we do have a few s390x buildbots lying around too. I'll try to run them once the review comments are addressed.

@lazerg In the future, please only work on issues that you can actually verify, and please don't use LLM output to reply to comments. I get that AI is good at solving issues like this, but it ultimately just shifts the cognitive effort to the maintainer/reviewer.

lazerg · 2026-09-27T02:42:09Z

Yes, understood.

ZeroIntensity · 2026-09-27T03:03:17Z

+        func = dll._testfunc_callback_int_to_longlong
+        func.argtypes = (CALLBACK,)
+        func.restype = c_longlong


Could you use the new wrap_dll_function util for this?

pythongh-156933: Widen narrow integer results in ctypes callbacks

72a218b

bedevere-app Bot mentioned this pull request Sep 6, 2026

_ctypes fails to extend the return value per libffi contract #156933

Open

bedevere-app Bot added the awaiting review label Sep 6, 2026

This comment was marked as resolved.

Sign in to view

ZeroIntensity reviewed Sep 19, 2026

View reviewed changes

ZeroIntensity added the pending The issue will be closed if no feedback is provided label Sep 20, 2026

Add test for widened narrow int callback results

ebf7568

Only run the widening test on s390x

9338911

ZeroIntensity removed the pending The issue will be closed if no feedback is provided label Sep 27, 2026

ZeroIntensity reviewed Sep 27, 2026

View reviewed changes

Address review comments

d54ca1f

ZeroIntensity reviewed Sep 27, 2026

View reviewed changes

Use wrap_dll_function in the widening test

6490b5f

		setfunc() only writes restype->size bytes. Cf. _ctypes_callproc()
		in callproc.c. */

Uh oh!

Conversation

lazerg commented Sep 6, 2026 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

python-cla-bot Bot commented Sep 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lazerg commented Sep 6, 2026

Uh oh!

This comment was marked as resolved.

ZeroIntensity left a comment

Choose a reason for hiding this comment

Uh oh!

lazerg commented Sep 20, 2026

Uh oh!

ZeroIntensity commented Sep 20, 2026

Uh oh!

lazerg commented Sep 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Endilll commented Sep 20, 2026

Uh oh!

ZeroIntensity commented Sep 20, 2026

Uh oh!

miladfarca commented Sep 25, 2026

Uh oh!

lazerg commented Sep 25, 2026

Uh oh!

lazerg commented Sep 25, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZeroIntensity commented Sep 27, 2026

Uh oh!

lazerg commented Sep 27, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lazerg commented Sep 6, 2026 •

edited by bedevere-app Bot

Loading

python-cla-bot Bot commented Sep 6, 2026 •

edited

Loading

lazerg commented Sep 20, 2026 •

edited

Loading