gh-96821: Fix undefined behaviour in _ctypes/cfield.c

[matthiasgoergens]

Left-shifting negative numbers is undefined behaviour.

Fortunately, multiplication works just as well, is (implementation) defined behaviour, and gets compiled to the same machine code as before by optimizing compilers.

• Issue: Enable -fstrict-overflow #96821

[matthiasgoergens]

@kumaraditya303 @mdickinson Would you please have a look? Thanks!

[mdickinson]

I'm not super-keen on this fix - I agree that it would be good to fix the undefined behaviour, but I wonder whether there's a better way. The multiplication bugs me on both conceptual grounds and technical grounds: conceptually, the intent really is just to shift bits around (unlike the similar changes in audioop.c, where arithmetic is what's intended), and it would be nice if the code reflected that. Technically, reasoning about correctness becomes even harder when arithmetic is brought into the mix, since now you're worrying about the usual arithmetic conversions as well as the integer promotion rules.

It feels as though the right solution is to ensure that we always do the bitwise operations (or at the very least, the shifts) on unsigned types rather than signed types, so that everything's well-defined, and then do sign-extension as necessary. So for h_get, for example, the current code is this:

static PyObject *
h_get(void *ptr, Py_ssize_t size)
{
    short val;
    memcpy(&val, ptr, sizeof(val));
    GET_BITFIELD(val, size);
    return PyLong_FromLong((long)val);
}

Could we arrange for the h_get code to be something that looks more like this, after macro expansion (it can almost certainly be streamlined with appropriate macros)? The key change here is that val now has type unsigned short instead of short, so that we do all of the bit manipulation with unsigned types. There's still implementation-defined behaviour in the very last line, in the cast from unsigned short to short, but I can live with that.

static PyObject *
h_get(void *ptr, Py_ssize_t size)
{
    unsigned short val, sign_bit;
    memcpy(&val, ptr, sizeof(val));

    if (NUM_BITS(size)) {
        // Assumes that 0 < NUM_BITS(size) <= width of short
        assert(0 < NUM_BITS(size));
        assert(NUM_BITS(size) <= 16);
        sign_bit = (unsigned short)1 << NUM_BITS(size) - 1;

        // Assumes that 0 <= LOW_BIT(size) < width of short
        assert(0 <= LOW_BIT(size));
        // assert(LOW_BIT(size) < 16);
        val = val >> LOW_BIT(size) & (sign_bit | sign_bit - 1U);
        val = (val ^ sign_bit) - sign_bit;
    }
    return PyLong_FromLong((long)(short)val);
}

N.B. The assert(LOW_BIT(size) < 16), when uncommented, fails on my machine when I run test_ctypes - there are apparently test cases where LOW_BIT(size) = 17. I'm not sure whether this is a bug in the test, or whether it's genuinely considered okay to have a low bit >= 16 for a bit field in a short. There isn't undefined behaviour here because thanks to the integer promotions we're actually shifting something of type int in this case, and 17 is a legal right shift for int on my machine.

[matthiasgoergens]

Thanks for having a look.
@mdickinson

I actually had a similar idea at first: work in unsigned for the left shift, then convert to signed for the right shift.

But I had some bugs in my implementation and things were getting complicated. So instead of thinking too hard, I went with the multiplication instead, which was much simpler for me to wrap my head around.

Your code looks good. Probably better than what I was trying to do. If it's similar enough to other fix, then our conpilers can probably see through it just as well, and compile it to the same code we used to have.

Btw, I am running benchmarks on main vs a version with -fstrict-overflow enabled, and so far it seems like I'm getting about 1% speedup on (geometric) average across the board for all benchmarks that have a 'significant' difference.

It's nothing too exciting, but considering that this is a fairly trivial compiler flag change, I guess we shouldn't complain.

(I'm rerunning the benchmarks with slightly different settings, I don't really have much experience with pyperformance.)

[mdickinson]

Drive-by comment: there's a use of the support.skip_if_sanitizer decorator in test_bitfields.py that should be removable as part of this PR:

cpython/Lib/test/test_ctypes/test_bitfields.py

Lines 43 to 44 in c8c0afc

	# bpo-46913: _ctypes/cfield.c h_get() has an undefined behavior
	@support.skip_if_sanitizer(ub=True)

[mdickinson]

it can almost certainly be streamlined with appropriate macros [...]

I'm thinking something along the lines of this:

static PyObject *
h_get(void *ptr, Py_ssize_t size)
{
    unsigned short val;
    memcpy(&val, ptr, sizeof(val));
    GET_SIGNED_BITFIELD(unsigned short, val, size);
    return PyLong_FromLong((long)(short)val);
}

where:

#define GET_SIGNED_BITFIELD(unsigned_type, v, size)                      \
    if (NUM_BITS(size)) {                                                \
        unsigned_type sign_bit = (unsigned_type)1 << NUM_BITS(size) - 1; \
        v = v >> LOW_BIT(size) & (sign_bit | sign_bit - 1U);             \
        v = (v ^ sign_bit) - sign_bit;                                   \
    }

[mdickinson]

there are apparently test cases where LOW_BIT(size) = 17

I'm a bit worried that there's a deeper bug here. The relevant test case arises from testing this structure:

class BITS(Structure):
    _fields_ = [("A", c_int, 1),
                ("B", c_int, 2),
                ("C", c_int, 3),
                ("D", c_int, 4),
                ("E", c_int, 5),
                ("F", c_int, 6),
                ("G", c_int, 7),
                ("H", c_int, 8),
                ("I", c_int, 9),

                ("M", c_short, 1),
                ("N", c_short, 2),
                ("O", c_short, 3),
                ("P", c_short, 4),
                ("Q", c_short, 5),
                ("R", c_short, 6),
                ("S", c_short, 7)]

It seems possible that we're trying to cram the bitfield for M into the same int that H and I live in. (With A through G having been put into 28 bits in a separate int.)

In any case, if there is a bug there it would be orthogonal to the fixes in this PR.

[mdickinson]

In any case, if there is a bug there it would be orthogonal to the fixes in this PR.

Many bugs. #73939, #84039, #86098, #59324 (which apparently I opened, a long time ago), and more ... In the case of that BITS structure, we have (on my machine):

(Pdb) BITS.M
<Field type=c_short, ofs=6:17, bits=1>
(Pdb) BITS.N
<Field type=c_short, ofs=6:18, bits=2>
(Pdb) BITS.O
<Field type=c_short, ofs=6:20, bits=3>
(Pdb) BITS.P
<Field type=c_short, ofs=6:23, bits=4>
(Pdb) BITS.Q
<Field type=c_short, ofs=6:27, bits=5>
(Pdb) BITS.R

and none of those make much sense. But discussion should be deferred to the other issues mentioned above.

@matthiasgoergens I don't suppose you'd be interested in becoming a ctypes bitfield expert / champion? There's a definite maintenance hole here. (I'm ruling myself out, I'm afraid: I do have a small portion of the necessary expertise, but unfortunately close to zero of the necessary energy and time.)

[matthiasgoergens]

@mdickinson I have a bit of time until mid October, as I am waiting out a non-compete before starting my next job. I'll have a look at the bitfield bugs already mentioned here, let's see how much I can learn.

[mdickinson]

As far as I can tell, it's trying to fit values in range(256) into signed bit-fields of sizes 1 to 7 bits. There's not enough space to fit those value (and we aren't checking negative numbers, either?)

I think the best we can do in this case, is to throw an error?

Throwing an error may be a backwards incompatible change. Having assignments to bit fields wrap modulo the appropriate power of two wouldn't be unreasonable behaviour; after all, that's what most compilers would do for direct assignments to an integer type that's too small (and is defined behaviour in the case of an unsigned type, and implementation-defined but typical behaviour for signed types).

But one of the things that makes ctypes development hard is that we don't just need something reasonable in a self-contained sense; we need something that also matches the behaviour of commonly-used C compilers for the relevant system.

So it may be worth a test at C level: if you assign 7 to an unsigned bit field of bit width 2 with gcc (say) and then retrieve the value, what's the behaviour? And then the same question for a signed bit field.

[mdickinson]

So it may be worth a test at C level: if you assign 7 to an unsigned bit field of bit width 2 with gcc (say) and then retrieve the value, what's the behaviour? And then the same question for a signed bit field.

Here's what happens with Clang 14.0.0. Using this test code:

#include <stdio.h>

struct bitfields {
    unsigned int some_val: 2;
    signed int other_val: 2;
};

int main(void) {
    struct bitfields x;
    x.some_val = 7;
    x.other_val = 7;
    printf("%u %d\n", x.some_val, x.other_val);
    return 0;
}

when I compile and run on my laptop, I get 3 -1. (Not surprisingly, I also get compile-time warnings about implicit truncation.)

[matthiasgoergens]

@mdickinson Yes, the modulo behaviour is exactly what you get on the C level. (Not sure if that's what the standard says, but it's definitely what GCC and clang give you.)

I figured that out after I wrote that comment, that's why I hid it afterwards.

However, I found some other problems. See the new issue and failing-test PR that I made.