diff --git a/Misc/NEWS.d/next/Library/2026-07-30-12-00-00.gh-issue-148286.Kx7Qm2.rst b/Misc/NEWS.d/next/Library/2026-07-30-12-00-00.gh-issue-148286.Kx7Qm2.rst new file mode 100644 index 00000000000000..56911e73f7e54b --- /dev/null +++ b/Misc/NEWS.d/next/Library/2026-07-30-12-00-00.gh-issue-148286.Kx7Qm2.rst @@ -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``. diff --git a/Modules/_ctypes/cfield.c b/Modules/_ctypes/cfield.c index 00b075bccff664..8270679e7ab383 100644 --- a/Modules/_ctypes/cfield.c +++ b/Modules/_ctypes/cfield.c @@ -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 @@ -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); \ } \ /////////////////////////////////////////////////////////////////////////// @@ -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); \ } \ /////////////////////////////////////////////////////////////////////////// diff --git a/Tools/ubsan/suppressions.txt b/Tools/ubsan/suppressions.txt index 2b9ad49ebf1eef..1c894d821f9587 100644 --- a/Tools/ubsan/suppressions.txt +++ b/Tools/ubsan/suppressions.txt @@ -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