The clang build was recently added for that buildbot and it seems on that particular architecture, test_struct fails with:

======================================================================
FAIL: test_bool (test.test_struct.StructTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/dje/cpython-buildarea/3.x.edelsohn-fedora-rawhide-z.clang-ubsan/build/Lib/test/test_struct.py", line 520, in test_bool
    self.assertTrue(struct.unpack('>?', c)[0])
AssertionError: False is not true

https://buildbot.python.org/all/#/builders/488/builds/6

Fedora rawhide recently upgraded Clang to version 10. The rest of the architectures seem fine.

Failed assertion here: https://github.com/python/cpython/blob/master/Lib/test/test_struct.py#L520

On this loop:

for c in [b'\x01', b'\x7f', b'\xff', b'\x0f', b'\xf0']:
    self.assertTrue(struct.unpack('>?', c)[0])

It fails for the b'\xf0' case

The call:
    struct.unpack('>?', b'\xf0')
means to unpack a "native bool", i.e. native size and alignment. Internally, this does:

    static PyObject *
    nu_bool(const char *p, const formatdef *f)
    {
        _Bool x;
        memcpy((char *)&x, p, sizeof x);
        return PyBool_FromLong(x != 0);
    }

i.e., copies "sizeof x" (1 byte) of memory to a temporary buffer x, and then treats that as _Bool.

While I don't have access to the C standard, I believe it says that assignment of a true value to _Bool can coerce to a unique "true" value. It seems that if a char doesn't have the exact bit pattern for true or false, casting to _Bool is undefined behavior. Is that correct?

Clang 10 on s390x seems to take advantage of this: it probably only looks at the last bit(s) so a _Bool with a bit pattern of 0xf0 turns out false.
But the tests assume that 0xf0 should unpack to True.

C compiler dev that it's indeed undefined behavior.

> Quick and obvious fix:
> 
>       static PyObject *
>       nu_bool(const char *p, const formatdef *f)
>       {
>           char x;
>           memcpy((char *)&x, p, sizeof x);
>           return PyBool_FromLong(x != 0);
>       }
>
> Which is optimized to
>
>     static PyObject *
>     nu_bool(const char *p, const formatdef *f)
>     {
>         return PyBool_FromLong(*p != 0);
>     }


I'm left with a question for CPython's struct experts:

The above would be my preferred fix, but the Python code is asking to convert a memory buffer to bool *using platform-specific semantics*.
Is this fix OK if C treats a \xf0 _Bool as falsey?


(Also, this assumes size of _Bool is the same as size of char.
I guess we can add a build-time assertion for that, and say we don't support platforms where that's not the case.)

maybe we should be raising an error if the bytes are not a valid platform _Bool pattern?

the concept of a native _Bool seems fuzzy.  the important thing for the struct module is to consume sizeof _Bool bytes from the input stream.  how those are interpreted is up to the platform.  So if the platform says a bool is 8 bytes and it only ever looks at the lowest bit in those for bool-ness, good for it.

in that situation our unittest assuming that b'\xf0' should be true when interpreted as a bool is wrong.

just get rid of that value from the loop in the test?

IMO:

- The "native" format should use native _Bool, and we should only test unpacking 0 and 1
- The "standard" format should use portable char semantics: continue to treat any non-zero value as true
- The docs should grow a warning that for the native format of '?', representation of true/false depends on the platform/compiler.

But what is "format"? The docs talk about size, alignment and byte order; bool representation is a slightly different concept. I'm not sure if it should follow Byte order or Size/Alignment: I think that the latter would be better (so only "@" uses the native _Bool semantics, but "=" uses portable char semantics), but it might be be harder to implement.

Charalampos Stratakis also reported this issue to python-dev:
"Unpacking native bools in the struct module: Is Python relying on undefined behavior?"
https://mail.python.org/archives/list/python-dev@python.org/thread/O742VLCYX2AE3RWQK5RBQ3BGUOHESLF5/

fwiw oss-fuzz also finds this on struct (via ubsan)

https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=20949

struct.unpack('?', ' ')

Viewing the oss-fuzz bug requires login. Is there any interesting public info in it?

Nothing too interesting, here's the stack trace:

/src/cpython3/Modules/_struct.c:487:28: runtime error: load of value 32, which is not a valid value for type '_Bool'
    #0 0x7f50371a7670 in nu_bool cpython3/Modules/_struct.c:487:28
    #1 0x7f503719ea3d in s_unpack_internal cpython3/Modules/_struct.c:1512:21
    #2 0x7f5037199f7e in unpack cpython3/Modules/clinic/_struct.c.h:259:20
    #3 0xb8b8bc in cfunction_vectorcall_FASTCALL cpython3/Objects/methodobject.c:366:24
    #4 0x4fe113 in _PyObject_VectorcallTstate cpython3/./Include/cpython/abstract.h:111:21
    #5 0x4fe51f in object_vacall cpython3/Objects/call.c:789:14
    #6 0x4fe7d8 in PyObject_CallFunctionObjArgs cpython3/Objects/call.c:896:14
    #7 0x4d394e in fuzz_struct_unpack cpython3/Modules/_xxtestfuzz/fuzzer.c:121:26
    #8 0x4d394e in _run_fuzz cpython3/Modules/_xxtestfuzz/fuzzer.c:398:14
    #9 0x4d394e in LLVMFuzzerTestOneInput cpython3/Modules/_xxtestfuzz/fuzzer.c:453:11

Using memcpy() to write a value different than 0 or 1 into a _Bool is clearly an undefined behavior. Example with clang UBSan.

bool.c:
---
#include <string.h>
#include <stdbool.h>

int main()
{
    char ch = 42;
    _Bool x;
    memcpy(&x, &ch, 1);
    return x == true;
}
---

$ clang -fsanitize=bool bool.c -o bool
$ ./bool
bool.c:9:12: runtime error: load of value 42, which is not a valid value for type '_Bool'

FYI - I updated the ubsan buildbot to clang 9 so this one shows up in there now:

/var/lib/buildbot/workers/clang-ubsan/3.x.gps-clang-ubsan.clang-ubsan/build/Modules/_struct.c:487:28: runtime error: load of value 116, which is not a valid value for type '_Bool'
Objects/memoryobject.c:1702:15: runtime error: load of value 116, which is not a valid value for type '_Bool'
Objects/memoryobject.c:2704:15: runtime error: load of value 116, which is not a valid value for type '_Bool'
Objects/memoryobject.c:2704:15: runtime error: load of value 116, which is not a valid value for type '_Bool'

> our unittest assuming that b'\xf0' should be true when interpreted as a bool is wrong.
> just get rid of that value from the loop in the test?

That could be the proper thing to do, but it does make it easy to invoke C undefined behavior from Python code. AFAIK, it would be the only such place in the struct module.

So, I'd like to assume and assert/test that sizeof(_Bool) is 1 and the false bit-pattern is (char)0, and with that, just use `PyBool_FromLong((_Bool)*p != 0)`.


> maybe we should be raising an error if the bytes are not a valid platform _Bool pattern?

That's quite hard to test for.
Also, on all current supported platforms, the patterns for bool and char 0 and 1 are the same. I don't see this being different elsewhere, but if there ever is a such a platform, let the test catch the broken assumption.

On Wed, Mar 11, 2020, at 07:41, Petr Viktorin wrote:
> 
> > maybe we should be raising an error if the bytes are not a valid platform _Bool pattern?
> 
> That's quite hard to test for.

How so? We just make the same assumption you're making that true = b'\x01' and false = NUL.

The memoryview module does the same thing as struct, and its tests expect the same behavior as with struct.

So, whatever we do in struct should be also done in memoryview.

> > > maybe we should be raising an error if the bytes are not a valid platform _Bool pattern?
> > 
> > That's quite hard to test for.
> 
> How so? We just make the same assumption you're making that true = b'\x01' and false = NUL.

Right, with that assumption, it's not that hard. And it becomes a test-only assumption, which is great!

But, I'm still not convinced this would be a good fix. The current struct documentation is consistent with *casting* char to _Bool, rather than doing the memcpy and reinterpreting as _Bool. The memcpy makes sense for larget types, but not so much for _Bool.

(Certainly, the docs' assertion that "the conversion between C and Python values should be obvious given their types" is not true here...)

Oh! I just reead the docs more carefully; they say:

For the '?' format character, the return value is either True or False. When packing, the truth value of the argument object is used. Either 0 or 1 in the native or standard bool representation will be packed, and any non-zero value will be True when unpacking.

Concerning memoryview, I've looked at this very briefly: memoryview
does not pack values directly, it casts first, which is legal:


#define PACK_SINGLE(ptr, src, type) \
    do {                                     \
        type x;                              \
        x = (type)src;                       \
        memcpy(ptr, (char *)&x, sizeof x);   \
    } while (0)


This macro has exposed compiler bugs before.

So which memoryview test (unless it is using the struct module)
relies on UB?

Okay, in memoryview the cast tests can trigger UB checks. memoryview assumes that bool is packed correctly, so just casting does not work.
Casting anything to bool is of course a bit silly anyway.

diff --git a/Lib/test/test_buffer.py b/Lib/test/test_buffer.py
index 6178ffde7a..86cf4c309f 100644
--- a/Lib/test/test_buffer.py
+++ b/Lib/test/test_buffer.py
@@ -2758,6 +2758,8 @@ class TestBufferProtocol(unittest.TestCase):
                 tsize = struct.calcsize(tfmt)
                 n = prod(_tshape) * tsize
                 obj = 'memoryview' if is_byte_format(tfmt) else 'bytefmt'
+                if "?" in tfmt:
+                    continue
                 for fmt, items, _ in iter_format(n, obj):
                     size = struct.calcsize(fmt)
                     shape = [n] if n > 0 else []

I checked that NumPy also packs correctly:

>>> import numpy as np
>>> x = np.array([0,1,2,3], dtype=np.bool)
>>> x.tobytes()
b'\x00\x01\x01\x01'

So I vote for not handling incorrectly packed values and removing
"and any non-zero value will be True when unpacking" from the docs,
which does not seem to make any sense for _Bool.

memoryview also does not guard against the theoretical possibility
of incorrectly packed values being trap representations. Values need
to be packed correctly, or you get UB.

> So I vote for not handling incorrectly packed values and removing
"and any non-zero value will be True when unpacking" from the docs,
which does not seem to make any sense for _Bool.

I disagree. I don't think struct module's job is to be faithful to _Bool semantics.

Up to this point, "?" worked for bytes with "only 0 is false" semantics, in a reliable and documented way. I don't see a reason to let that continue.

You're right about trap representations, but IMO floats are much more tied to hardware (and serious users of float are aware of the pitfalls), while _Bool semantics are governed by the whims of the compiler.

Also, the "@" prefix is specifically documented to select native Byte order, Size, and Alignment; not how the bit-pattern is interpreted.

The docs for native mode (we now always assume C99):

"The '?' conversion code corresponds to the _Bool type defined by C99."


The memoryview tests that fail are essentially auto-generated and not
prescriptive. They just happened to work without UBSan:


>>> x = array.array("B", [0,1,2,3])
>>> m = memoryview(x)
>>> m.format
'B'
>>> c = m.cast("?") # Wrong!
>>> c.tolist()
[False, True, True, True]


I don't see the point in working around UB for the purposes of
displaying the wrong cast.

If you do subsequent array operations with the variable "c", UB will happen there.



In theory you can do the same with a cast from unsigned to signed
integers. Signed integers are allowed to be trap representations,
though I don't know an actual platform where this is the case.

> The docs for native mode (we now always assume C99):
>
> "The '?' conversion code corresponds to the _Bool type defined by C99."

But, nowhere is it suggested that conversion is "do memcpy and then interpret the bit pattern". That is pretty weird way to convert values in C; it seems to me that it's only a hack to avoid unaligned access.
IMO, casting is a more natural way to convert values. And casting to _Bool coerces the value to 0 or 1.

The memcpy() is NOT a hack and performs exactly the same operation
as casting the pointer and then dereferencing, only in a manner that
avoids unaligned accesses.

On platforms like x86 the memcpy() is optimized to a simple assignment.

Casting the pointer and then dereferencing would also be subject
to the UB warnings.


You are the one who wanted to *introduce* a hack by dereferencing
as char and then cast to _Bool. :-)

New changeset 1ae9cde4b2323235b5f9ff4bc76e4175a2257172 by Stefan Krah in branch 'master':
bpo-39689: Do not test undefined casts to _Bool (GH-18964)
https://github.com/python/cpython/commit/1ae9cde4b2323235b5f9ff4bc76e4175a2257172

New changeset 636eecbbbbc16229432ec8e26e6da287c52f3ca3 by Miss Islington (bot) in branch '3.7':
bpo-39689: Do not test undefined casts to _Bool (GH-18964) (#18965)
https://github.com/python/cpython/commit/636eecbbbbc16229432ec8e26e6da287c52f3ca3

New changeset f8ce3e2dae277baa2ef92e8a3e935953dc6c3f39 by Miss Islington (bot) in branch '3.8':
bpo-39689: Do not test undefined casts to _Bool (GH-18964) (#18966)
https://github.com/python/cpython/commit/f8ce3e2dae277baa2ef92e8a3e935953dc6c3f39

memoryview only supports the native format, so I've disabled the
(wrong) test that casts arrays with arbitrary values to _Bool. So
memoryview is done.


IMO the problem in _struct is that it swaps the x->unpack function
for the native one, which does not seem right for _Bool:

        /* Scan through the native table, find a matching
           entry in the endian table and swap in the
           native implementations whenever possible
           (64-bit platforms may not have "standard" sizes) */

If one disables that swap, the tests pass here.

> You are the one who wanted to *introduce* a hack by dereferencing
as char and then cast to _Bool. :-)

Yes, I did change my mind after reading the documentation.

The docs say two contradicting things:
1. The '?' conversion code corresponds to the _Bool type defined by C99
2. ... any non-zero value will be True when unpacking.

So it's clear that something has to change. IMO, preserving (2) and relaxing (1) is the more useful choice.

> So it's clear that something has to change. IMO, preserving (2) and relaxing (1) is the more useful choice.

But not in this issue I think. GH-18969 is a minimal change that
*removes* UB for the standard sizes.


UB for the native type is a direct consequence of using _Bool.
Native types should be left as is because that's what array
libraries expect. The docs could need a change (in another issue).


Also, UB can only happen in a constructed example --- correctly
packed arrays don't have any incorrect values.


So I think any fear of UB here is not warranted.

Note that the implementation of np_bool in _struct.c [1] is incorrect because this is supposed to access a boolean of a standard size, but uses _Bool. The size of _Bool is not prescribed, and IIRC sizeof(_Bool) was 4 with the compilers used for macOS/PPC.

[1] https://github.com/python/cpython/blob/master/Modules/_struct.c#L703

Sigh... never mind, I misread the code.  Please ignore msg364472

I think we are speaking past each other.

In my (current) view, the semantics are spelled out in the documentation: "any non-zero value will be True when unpacking".
There's also a mention that this corresponds to the _Bool type in C. While this was the case with compilers in the past, it's no longer true with clang 9.


In your view, the semantics are dictated by the correspondence to _Bool, and the "non-zero value will be True when unpacking" is the fluff to be ignored and removed.


The docs assume the two behaviors (_Bool and non-zero) are equivalent. In this bug we find out that they are not, so to fix the bug, we need to make a choice which one to keep and which one to throw out.
I see nothing that would make one view inherently better than the other.


What "array libraries expect" is IMO not relevant: under any of the two views, libraries that are well-written (under that view) will be fine. Problems come when the library and Python choose different sides, e.g. when a non-C library can't use _Bool and thus packs arrays with the expectation that "any non-zero value will be True when unpacking".

What is a minimal change in *implementation* is a bigger change in *behavior*: unpacking of arrays will now depend greatly on details like the compiler used to build Python. I see that as the greater evil: since the data can be sharted across environments, languages and compilers, keeping the semantics well-defined seems better than leaving them to the compiler.
I don't see a compelling reason to choose _Bool semantics, but perhaps there is one.

I think this issue should be about fixing the tests so that people
looking at the sanitizer buildbots can move on.

GH-18969 fixes "<?", ">?" and "!?", which clearly used wrong
semantics with the new compiler behavior. This should be an
uncontroversial fix that also takes care of test_struct.


Can we please discuss native _Bool in another issue?

There is no non-hackish way of unpacking _Bool if new compilers
essentially treat values outside [0, 1] as a trap representation.


You could determine sizeof(_Bool), use the matching unsigned type,
unpack as that, then cast to _Bool. But do you really want to force
that procedure on all array libraries that want to be PEP-3118
compatible?

I'd rather deprecate _Bool and use uint8_t, but that definitely
deserves a separate issue.

New changeset 472fc843ca816d65c12f9508ac762ca492165c45 by Stefan Krah in branch 'master':
bpo-39689: Do not use native packing for format "?" with standard size (GH-18969)
https://github.com/python/cpython/commit/472fc843ca816d65c12f9508ac762ca492165c45

I see. Thanks for your patience explaining this to me!

I will merge and continue in a different issue.

Moved to Discourse, IMO that's a better place for maintainers of other PEP-3118-compatible libraries to chime in:
https://discuss.python.org/t/behavior-of-struct-format-native-bool/3774

New changeset 0f9e889cd94c1e86f8ff28733b207c99803ca8a4 by Miss Islington (bot) in branch '3.7':
bpo-39689: Do not use native packing for format "?" with standard size (GH-18969)
https://github.com/python/cpython/commit/0f9e889cd94c1e86f8ff28733b207c99803ca8a4

New changeset 572ef747692055a270a9fbf8eeaf5c4a15c8e332 by Miss Islington (bot) in branch '3.8':
bpo-39689: Do not use native packing for format "?" with standard size (GH-18969)
https://github.com/python/cpython/commit/572ef747692055a270a9fbf8eeaf5c4a15c8e332

Objects/memoryview.c uses memcpy() on _Bool which leads to undefined behavior with GCC 11: see bpo-42587.