gh-132470: Prevent crash in ctypes.CField when `byte_size` is incorrect by dura0ok · Pull Request #132475 · python/cpython

dura0ok · 2025-04-13T11:37:25Z

When creating a ctypes.CField with an incorrect byte_size (e.g., using byte_size=2 for ctypes.c_byte), the code would previously abort due to the failed assertion byte_size == info->size.

This commit replaces the assertion with a proper error handling mechanism that raises a ValueError when byte_size does not match the expected type size. This prevents the crash and provides a more informative error message to the us

Issue: Building a ctypes.CField with wrong byte_size aborts #132470

…e size (pythongh-132470) When creating a ctypes.CField with an incorrect byte_size (e.g., using byte_size=2 for ctypes.c_byte), the code would previously abort due to the failed assertion byte_size == info->size. This commit replaces the assertion with a proper error handling mechanism that raises a ValueError when byte_size does not match the expected type size. This prevents the crash and provides a more informative error message to the us

bedevere-app · 2025-04-13T11:37:30Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

dura0ok · 2025-04-13T11:40:59Z

I not found place where to write regression test for it.

sobolevn

Please, add a test case somewhere to test_ctypes and a NEWS entry.

bedevere-app · 2025-04-13T13:17:53Z

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

dura0ok · 2025-04-13T13:25:10Z

@sobolevn @ZeroIntensity
news and test added

brianschubert · 2025-04-13T13:36:42Z

Lib/test/leakers/test_ctypes.py

    gc.collect()
+
+
+def test_invalid_byte_size_raises(self):


I'm not sure Lib/test/leakers is the right place for this test. This should probably go with the ctypes-proper tests in Lib/test/test_ctypes. Lib/test/test_ctypes/test_struct_fields.py might be a good fit.

yep, moved, thanks.

picnixz · 2025-04-13T13:41:15Z

Misc/NEWS.d/next/C_API/2025-04-13-20-22-14.gh-issue-132470.uUUrUJ.rst

@@ -0,0 +1,4 @@
+Creating a `ctypes.CField` with a `byte_size` that does not match the actual


No need for a NEWS entry. The structure is currently opaque and only exposed as a read-only structure. Users shouldn't be able to call the constructor directly.

picnixz · 2025-04-13T13:41:25Z

Misc/NEWS.d/next/C_API/2025-04-13-20-23-59.gh-issue-132470.uUUrUJ.rst

@@ -0,0 +1,4 @@
+Creating a `ctypes.CField` with a `byte_size` that does not match the actual


Remove this NEWs entry as well.

Adds a test to ensure that creating a ctypes.CField with a byte_size that doesn't match the underlying C type size (e.g., 2 bytes for c_byte) correctly raises a ValueError. This test verifies the fix for pythongh-132470 and prevents future regressions where such mismatches could cause an abort.

picnixz · 2025-04-13T13:51:45Z

Lib/test/test_ctypes/test_struct_fields.py

+        with self.assertRaises(ValueError) as cm:
+            CField(
+                name="a",
+                type=c_byte,
+                byte_size=2, # Wrong size: c_byte is only 1 byte
+                byte_offset=2,
+                index=1,
+                _internal_use=True
+            )
+
+            self.assertIn("does not match type size", str(cm.exception))


I suggest using assertRaisesRegex instead here.

picnixz · 2025-04-13T13:51:55Z

Lib/test/test_ctypes/test_struct_fields.py

+            CField(
+                name="a",
+                type=c_byte,
+                byte_size=2, # Wrong size: c_byte is only 1 byte


Suggested change

byte_size=2, # Wrong size: c_byte is only 1 byte

byte_size=2, # Wrong size: c_byte is only 1 byte

Hmm, did the commit get lost somewhere? It looks like this is still one space instead of two.

brianschubert · 2025-04-13T13:53:20Z

@dura0ok it's best to avoid force pushing once you've received feedback, since that makes it harder for reviewers to see what changed. All commits are squashed on merge anyway

Misc/NEWS.d/next/C_API/2025-04-13-20-52-39.gh-issue-132470.UqBQjN.rst

Co-authored-by: sobolevn <mail@sobolevn.me>

sobolevn

Thank you!

ZeroIntensity

LGTM as well, but the formatting change that Bénédikt suggested seemingly got un-fixed.

That, and I didn't realize we had skip news on this. I'm going to remove that because I'd argue this is public enough for a news entry, but someone is free to bicker with me on that.

ZeroIntensity · 2025-04-16T10:03:34Z

Lib/test/test_ctypes/test_struct_fields.py

+            CField(
+                name="a",
+                type=c_byte,
+                byte_size=2, # Wrong size: c_byte is only 1 byte


Hmm, did the commit get lost somewhere? It looks like this is still one space instead of two.

dura0ok · 2025-04-16T10:09:17Z

@ZeroIntensity problem with space fixed.

sobolevn

Thanks! I will backport this to 3.13 as well.

miss-islington-app · 2025-04-22T13:13:04Z

Thanks @dura0ok for the PR, and @sobolevn for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington-app · 2025-04-22T13:13:20Z

Sorry, @dura0ok and @sobolevn, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 3b4b56f46dbfc0c336a1f70704f127593ec1f4ce 3.13

sobolevn · 2025-04-22T13:14:22Z

@dura0ok can you please work on a manual backport?

dura0ok · 2025-04-22T13:17:56Z

@sobolevn yes, sure. In which pr?

encukou · 2025-04-28T15:09:42Z

Thanks for getting this in everyone!

Python 3.13 does not allow creating CField from Python at all, and doesn't have a byte_size argument, so this shouldn't be backported.

@ZeroIntensity: A friendly bicker, just so you know my opinion: This should not have a NEWS entry, since the CField constructor is not public. If you call it (with _internal_use=True), you are using fragile undocumented internal API.
But, now that the entry it's there I don't think it's worth it to remove it.

ZeroIntensity · 2025-04-28T15:18:50Z

FWIW, I do sometimes see changelog entries for changes to the private API if people were relying on it. I don't know if that applied here.

bedevere-app bot added the awaiting review label Apr 13, 2025

dura0ok changed the title ~~Fix: Prevent crash in ctypes.CField when byte_size does not match typ…~~ gh-132470: Prevent crash in ctypes.CField when byte_size does not match typ… Apr 13, 2025

bedevere-app bot mentioned this pull request Apr 13, 2025

Building a ctypes.CField with wrong byte_size aborts #132470

Closed

sobolevn reviewed Apr 13, 2025

View reviewed changes

dura0ok force-pushed the gh-132470 branch from 6be500a to 00530ba Compare April 13, 2025 13:27

brianschubert reviewed Apr 13, 2025

View reviewed changes

picnixz changed the title ~~gh-132470: Prevent crash in ctypes.CField when byte_size does not match typ…~~ gh-132470: Prevent crash in ctypes.CField when byte_size is incorrect Apr 13, 2025

picnixz reviewed Apr 13, 2025

View reviewed changes

dura0ok force-pushed the gh-132470 branch from 00530ba to c52e20f Compare April 13, 2025 13:43

picnixz added the skip news label Apr 13, 2025

picnixz reviewed Apr 13, 2025

View reviewed changes

add news

4c558be

dura0ok force-pushed the gh-132470 branch from c52e20f to 4c558be Compare April 13, 2025 13:53

fix review test

b20b0d8

ZeroIntensity added the topic-ctypes label Apr 15, 2025

ZeroIntensity reviewed Apr 15, 2025

View reviewed changes

Misc/NEWS.d/next/C_API/2025-04-13-20-52-39.gh-issue-132470.UqBQjN.rst Show resolved Hide resolved

sobolevn reviewed Apr 16, 2025

View reviewed changes

Misc/NEWS.d/next/C_API/2025-04-13-20-52-39.gh-issue-132470.UqBQjN.rst Outdated Show resolved Hide resolved

Misc/NEWS.d/next/C_API/2025-04-13-20-52-39.gh-issue-132470.UqBQjN.rst Show resolved Hide resolved

Update news

be00372

Co-authored-by: sobolevn <mail@sobolevn.me>

sobolevn approved these changes Apr 16, 2025

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting review labels Apr 16, 2025

ZeroIntensity approved these changes Apr 16, 2025

View reviewed changes

ZeroIntensity removed the skip news label Apr 16, 2025

fix space

1d5f7cb

sobolevn approved these changes Apr 22, 2025

View reviewed changes

sobolevn added the needs backport to 3.13 bugs and security fixes label Apr 22, 2025

sobolevn merged commit 3b4b56f into python:main Apr 22, 2025
44 checks passed

bedevere-app bot removed the awaiting merge label Apr 22, 2025

miss-islington-app bot assigned sobolevn Apr 22, 2025

encukou removed the needs backport to 3.13 bugs and security fixes label Apr 28, 2025

		@@ -0,0 +1,4 @@
		Creating a `ctypes.CField` with a `byte_size` that does not match the actual

	byte_size=2, # Wrong size: c_byte is only 1 byte
	byte_size=2, # Wrong size: c_byte is only 1 byte

Uh oh!

Conversation

dura0ok commented Apr 13, 2025 • edited by bedevere-app bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-app bot commented Apr 13, 2025

Uh oh!

dura0ok commented Apr 13, 2025

Uh oh!

sobolevn left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-app bot commented Apr 13, 2025

Uh oh!

dura0ok commented Apr 13, 2025

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!

picnixz Apr 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

brianschubert commented Apr 13, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sobolevn left a comment

Choose a reason for hiding this comment

Uh oh!

ZeroIntensity left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dura0ok commented Apr 16, 2025

Uh oh!

sobolevn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

miss-islington-app bot commented Apr 22, 2025

Uh oh!

miss-islington-app bot commented Apr 22, 2025

Uh oh!

sobolevn commented Apr 22, 2025

Uh oh!

dura0ok commented Apr 22, 2025

Uh oh!

encukou commented Apr 28, 2025

Uh oh!

ZeroIntensity commented Apr 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dura0ok commented Apr 13, 2025 •

edited by bedevere-app bot

Loading

picnixz Apr 13, 2025 •

edited

Loading