Skip to content

Commit b390d91

Browse files
gh-148286: Fix undefined behaviour in the ctypes bit field accessors
BIT_MASK, GET_BITFIELD and SET in Modules/_ctypes/cfield.c did their bit manipulation in the bit field's own type. When that type is signed, both shifting a negative value left and shifting a one into the sign bit are undefined behaviour, so UBSan reported, among others: cfield.c:644: left shift of 1 by 63 places cannot be represented in 'int64_t' cfield.c:640: signed integer overflow: -2147483648 - 1 cfield.c:636: left shift of negative value -32768 Do the arithmetic in uint64_t -- the widest type the accessors support -- and convert back to the field's own type at the end. BIT_MASK is replaced by LOW_BITS_MASK(), which no longer needs a type argument now that the mask is always computed unsigned. GET_BITFIELD takes the field type so it can still sign-extend signed fields, and is wrapped in do/while(0). Verified against the old macros with an exhaustive differential test: for every fixed-width type (int8_t..uint64_t), every num_bits and low_bit combination, ten storage patterns and twenty stored values -- 1,167,600 cases, no behaviour change, under both gcc and clang. The two Modules/_ctypes/cfield.c entries in Tools/ubsan/suppressions.txt are no longer needed. With them removed, test_ctypes passes under UBSAN_OPTIONS=halt_on_error=1; before this change it aborts at cfield.c:644 in i64_set.
1 parent f4b1d3e commit b390d91

3 files changed

Lines changed: 49 additions & 22 deletions

File tree

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
Fix undefined behaviour in the :mod:`ctypes` bit field accessors. The
2+
``BIT_MASK``, ``GET_BITFIELD`` and ``SET`` macros in
3+
``Modules/_ctypes/cfield.c`` shifted negative values left and shifted ones
4+
into the sign bit of a signed type. The bit manipulation is now done in
5+
``uint64_t`` and converted back to the field's own type, which removes the
6+
two ``Modules/_ctypes/cfield.c`` entries from
7+
``Tools/ubsan/suppressions.txt``.

Modules/_ctypes/cfield.c

Lines changed: 42 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -492,23 +492,49 @@ Py_ssize_t NUM_BITS(Py_ssize_t bitsize) {
492492
return bitsize >> 16;
493493
}
494494

495-
/* Doesn't work if NUM_BITS(size) == 0, but it never happens in SET() call. */
496-
#define BIT_MASK(type, size) (((((type)1 << (NUM_BITS(size) - 1)) - 1) << 1) + 1)
495+
/* Bit fields are manipulated as uint64_t -- the widest type the accessors
496+
below support -- and converted back to the field's own type at the end.
497+
Doing the arithmetic in the field's type would be undefined behaviour
498+
whenever that type is signed: shifting a negative value left, and shifting
499+
a one into the sign bit, are both UB. */
500+
501+
/* A mask with the low `num_bits` bits set.
502+
Doesn't work if num_bits == 0, but that never happens here. */
503+
static inline
504+
uint64_t LOW_BITS_MASK(Py_ssize_t num_bits) {
505+
assert(0 < num_bits);
506+
assert(num_bits <= 64);
507+
return (~(uint64_t)0) >> (64 - num_bits);
508+
}
497509

498-
/* This macro CHANGES the first parameter IN PLACE. For proper sign handling,
499-
we must first shift left, then right.
500-
*/
501-
#define GET_BITFIELD(v, size) \
502-
if (NUM_BITS(size)) { \
503-
v <<= (sizeof(v)*8 - LOW_BIT(size) - NUM_BITS(size)); \
504-
v >>= (sizeof(v)*8 - NUM_BITS(size)); \
505-
}
510+
/* True if `type` is signed. Spelled so that neither comparison is the
511+
`unsigned < 0` pattern that compilers warn about. */
512+
#define IS_SIGNED_TYPE(type) (!((type)0 < (type)-1))
513+
514+
/* This macro CHANGES the `v` parameter IN PLACE. */
515+
#define GET_BITFIELD(type, v, size) \
516+
do { \
517+
if (NUM_BITS(size)) { \
518+
uint64_t bitfield_bits = ((uint64_t)(v) >> LOW_BIT(size)) \
519+
& LOW_BITS_MASK(NUM_BITS(size)); \
520+
if (IS_SIGNED_TYPE(type) \
521+
&& (bitfield_bits >> (NUM_BITS(size) - 1))) \
522+
{ \
523+
/* The field's top bit is set: sign-extend it. */ \
524+
bitfield_bits |= ~LOW_BITS_MASK(NUM_BITS(size)); \
525+
} \
526+
(v) = (type)bitfield_bits; \
527+
} \
528+
} while (0)
506529

507-
/* This macro RETURNS the first parameter with the bit field CHANGED. */
530+
/* This macro RETURNS the `x` parameter with the bit field CHANGED. */
508531
#define SET(type, x, v, size) \
509-
(NUM_BITS(size) ? \
510-
( ( (type)(x) & ~(BIT_MASK(type, size) << LOW_BIT(size)) ) | ( ((type)(v) & BIT_MASK(type, size)) << LOW_BIT(size) ) ) \
511-
: (type)(v))
532+
((type)(NUM_BITS(size) \
533+
? (((uint64_t)(x) \
534+
& ~(LOW_BITS_MASK(NUM_BITS(size)) << LOW_BIT(size))) \
535+
| (((uint64_t)(v) & LOW_BITS_MASK(NUM_BITS(size))) \
536+
<< LOW_BIT(size))) \
537+
: (uint64_t)(v)))
512538

513539
/*****************************************************************
514540
* The setter methods return an object which must be kept alive, to keep the
@@ -571,7 +597,7 @@ Py_ssize_t NUM_BITS(Py_ssize_t bitsize) {
571597
assert(NUM_BITS(size_arg) || (size_arg == (NBITS) / 8)); \
572598
CTYPE val; \
573599
memcpy(&val, ptr, sizeof(val)); \
574-
GET_BITFIELD(val, size_arg); \
600+
GET_BITFIELD(CTYPE, val, size_arg); \
575601
return PYAPI_FROMFUNC(val); \
576602
} \
577603
///////////////////////////////////////////////////////////////////////////
@@ -604,7 +630,7 @@ Py_ssize_t NUM_BITS(Py_ssize_t bitsize) {
604630
CTYPE val; \
605631
memcpy(&val, ptr, sizeof(val)); \
606632
val = PY_SWAPFUNC(val); \
607-
GET_BITFIELD(val, size_arg); \
633+
GET_BITFIELD(CTYPE, val, size_arg); \
608634
return PYAPI_FROMFUNC(val); \
609635
} \
610636
///////////////////////////////////////////////////////////////////////////

Tools/ubsan/suppressions.txt

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,5 @@
99
# Objects/object.c:97:5: runtime error: member access within null pointer of type 'PyThreadState' (aka 'struct _ts')
1010
null:Objects/object.c
1111

12-
# Modules/_ctypes/cfield.c:644:1: runtime error: left shift of 1 by 63 places cannot be represented in type 'int64_t' (aka 'long')
13-
shift-base:Modules/_ctypes/cfield.c
14-
15-
# Modules/_ctypes/cfield.c:640:1: runtime error: signed integer overflow: -2147483648 - 1 cannot be represented in type 'int'
16-
signed-integer-overflow:Modules/_ctypes/cfield.c
17-
1812
# Modules/_io/stringio.c:350:24: runtime error: addition of unsigned offset to 0x7fd01ec25850 overflowed to 0x7fd01ec2584c
1913
pointer-overflow:Modules/_io/stringio.c

0 commit comments

Comments
 (0)