Skip to content

gh-148286: Fix undefined behaviour in the ctypes bit field accessors #154912

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
matthiasgoergens wants to merge 1 commit into
base: main
Choose a base branch
from
+50 −22
Open

gh-148286: Fix undefined behaviour in the ctypes bit field accessors #154912

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions Misc/NEWS.d/next/Library/2026-07-30-12-00-00.gh-issue-148286.Kx7Qm2.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
Fix undefined behaviour in the :mod:`ctypes` bit field accessors. The
``BIT_MASK``, ``GET_BITFIELD`` and ``SET`` macros in
``Modules/_ctypes/cfield.c`` shifted negative values left and shifted ones
into the sign bit of a signed type. The bit manipulation is now done in
``uint64_t`` and converted back to the field's own type, which removes the
two ``Modules/_ctypes/cfield.c`` entries from
``Tools/ubsan/suppressions.txt``.
59 changes: 43 additions & 16 deletions Modules/_ctypes/cfield.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -492,23 +492,50 @@ Py_ssize_t NUM_BITS(Py_ssize_t bitsize) {
return bitsize >> 16;
}

/* Doesn't work if NUM_BITS(size) == 0, but it never happens in SET() call. */
#define BIT_MASK(type, size) (((((type)1 << (NUM_BITS(size) - 1)) - 1) << 1) + 1)
/* Bit fields are manipulated as uint64_t -- the widest type the accessors
below support -- and converted back to the field's own type at the end.
Doing the arithmetic in the field's type would be undefined behaviour
whenever that type is signed: shifting a negative value left, and shifting
a one into the sign bit, are both UB. */

/* A mask with the low `num_bits` bits set.
Doesn't work if num_bits == 0, but that never happens here. */
static inline
uint64_t LOW_BITS_MASK(Py_ssize_t num_bits) {
assert(0 < num_bits);
assert(num_bits <= 64);
return (~(uint64_t)0) >> (64 - num_bits);
}

/* This macro CHANGES the first parameter IN PLACE. For proper sign handling,
we must first shift left, then right.
*/
#define GET_BITFIELD(v, size) \
if (NUM_BITS(size)) { \
v <<= (sizeof(v)*8 - LOW_BIT(size) - NUM_BITS(size)); \
v >>= (sizeof(v)*8 - NUM_BITS(size)); \
}
/* True if `type` is signed. Spelled so that neither comparison is the
`unsigned < 0` pattern that compilers warn about. */
#define IS_SIGNED_TYPE(type) (!((type)0 < (type)-1))

/* This macro CHANGES the `v` parameter IN PLACE. */
#define GET_BITFIELD(type, v, size) \
do { \
if (NUM_BITS(size)) { \
uint64_t bitfield_bits = ((uint64_t)(v) >> LOW_BIT(size)) \
& LOW_BITS_MASK(NUM_BITS(size)); \
if (IS_SIGNED_TYPE(type)) { \
/* Sign-extend: flipping the field's sign bit and then \
subtracting it leaves the low bits alone and fills the \
high bits with copies of that sign bit. */ \
uint64_t sign_bit = (uint64_t)1 << (NUM_BITS(size) - 1); \
bitfield_bits = (bitfield_bits ^ sign_bit) - sign_bit; \
} \
(v) = (type)bitfield_bits; \
} \
} while (0)

/* This macro RETURNS the first parameter with the bit field CHANGED. */
/* This macro RETURNS the `x` parameter with the bit field CHANGED. */
#define SET(type, x, v, size) \
(NUM_BITS(size) ? \
( ( (type)(x) & ~(BIT_MASK(type, size) << LOW_BIT(size)) ) | ( ((type)(v) & BIT_MASK(type, size)) << LOW_BIT(size) ) ) \
: (type)(v))
((type)(NUM_BITS(size) \
? (((uint64_t)(x) \
& ~(LOW_BITS_MASK(NUM_BITS(size)) << LOW_BIT(size))) \
| (((uint64_t)(v) & LOW_BITS_MASK(NUM_BITS(size))) \
<< LOW_BIT(size))) \
: (uint64_t)(v)))

/*****************************************************************
* The setter methods return an object which must be kept alive, to keep the
Expand Down Expand Up @@ -571,7 +598,7 @@ Py_ssize_t NUM_BITS(Py_ssize_t bitsize) {
assert(NUM_BITS(size_arg) || (size_arg == (NBITS) / 8)); \
CTYPE val; \
memcpy(&val, ptr, sizeof(val)); \
GET_BITFIELD(val, size_arg); \
GET_BITFIELD(CTYPE, val, size_arg); \
return PYAPI_FROMFUNC(val); \
} \
///////////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -604,7 +631,7 @@ Py_ssize_t NUM_BITS(Py_ssize_t bitsize) {
CTYPE val; \
memcpy(&val, ptr, sizeof(val)); \
val = PY_SWAPFUNC(val); \
GET_BITFIELD(val, size_arg); \
GET_BITFIELD(CTYPE, val, size_arg); \
return PYAPI_FROMFUNC(val); \
} \
///////////////////////////////////////////////////////////////////////////
Expand Down
6 changes: 0 additions & 6 deletions Tools/ubsan/suppressions.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,5 @@
# Objects/object.c:97:5: runtime error: member access within null pointer of type 'PyThreadState' (aka 'struct _ts')
null:Objects/object.c

# Modules/_ctypes/cfield.c:644:1: runtime error: left shift of 1 by 63 places cannot be represented in type 'int64_t' (aka 'long')
shift-base:Modules/_ctypes/cfield.c

# Modules/_ctypes/cfield.c:640:1: runtime error: signed integer overflow: -2147483648 - 1 cannot be represented in type 'int'
signed-integer-overflow:Modules/_ctypes/cfield.c

# Modules/_io/stringio.c:350:24: runtime error: addition of unsigned offset to 0x7fd01ec25850 overflowed to 0x7fd01ec2584c
pointer-overflow:Modules/_io/stringio.c
Loading