Fix #54391: Invalid pointer aliasing in SAS7BDAT parser

[jonashaag]

@thesamesam please review.

• closes BUG: SIGBUS in test_float_byteswap test on arm #54391 (Replace xxxx with the GitHub issue number)
• Tests added and passed if fixing a bug or adding a new feature
• All code checks passed.
• Added type annotations to new arguments/methods/functions.
• Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

[SoapGentoo]

This is UB again, you generally cannot cast pointer types like this.

[jonashaag]

I wonder why there is not compiler warning if this is UB.

What's the correct way to cast those bits?

[jonashaag]

Like this?

[SoapGentoo]

Here's an idea:
assuming this code is always compiled as C (and not C++), you can use a union:

cdef float _byteswap_float(float num):
    cdef union Punning:
        uint32_t u
        float f

    cdef Punning punning = num;
    punning.u = _byteswap4(punning.u)
    return punning.f

I haven't tested this code, but you get the gist. Important: this is UB in C++, which doesn't allow type punning through unions.

[SoapGentoo]

btw, the memcpy was fine, it's just that using a union is more transparent to MSVC's optimizer, which is pretty weak compared to GCC/Clang's optimizer.

[jonashaag]

OK, switched to union, thanks a lot!

[WillAyd]

Why wouldn't we just use a uint8_t * and cast to the appropriate size in the byteswap calls? The union approach seems kind of overkill here

[SoapGentoo]

you mean cast the uint8_t* back to a uint32_t* later again or what?

[WillAyd]

Sorry to be clear I was asking if there was a way to deal with the raw data pointer. I see the pattern is something like:

def read_float_with_byteswap(bytes data, Py_ssize_t offset, bint byteswap):
    assert offset + 4 < len(data)
    cdef:
        const char *data_ptr = data
        float res = (<float*>(data_ptr + offset))[0]
    if byteswap:
        res = _byteswap_float(res)
    return res

So we are casting to float then going back in a circle to work with raw bytes for the byteswap. It would seem more logically structured if we just did

def read_float_with_byteswap(bytes data, Py_ssize_t offset, bint byteswap):
    assert offset + 4 < len(data)
    cdef:
        float res = (<float*>(data_ptr + offset))[0]
    if byteswap:
        return (<float*>_byteswap_float(data_ptr + offset))[0]
   else:
        return (<float*>(data_ptr + offset))[0]

And just have the byteswap function work with the data buffer directly. Within byteswap I think could just do

cdef uint32_t _byteswap_float(bytes data):
    cdef uint32_t value*
    memcpy(value, data, 4)
    return value[0]

[SoapGentoo]

see my other PR, the loader functions are invalid too

[SoapGentoo]

#54407 supersedes this PR

[mroeschke]

Closing in favor of #54407