[issue39689] test_struct failure on s390x Fedora Clang buildbot

[Benjamin Peterson]

Benjamin Peterson  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39689] test_struct failure on s390x Fedora Clang buildbot

[Petr Viktorin]

Change by Petr Viktorin :


--
keywords: +patch
pull_requests: +18277
stage: needs patch -> patch review
pull_request: https://github.com/python/cpython/pull/18925

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39689] test_struct failure on s390x Fedora Clang buildbot

[Petr Viktorin]

Petr Viktorin  added the comment:

> 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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39689] test_struct failure on s390x Fedora Clang buildbot

[Gregory P. Smith]

Gregory P. Smith  added the comment:

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'

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39689] test_struct failure on s390x Fedora Clang buildbot

[STINNER Victor]

STINNER Victor  added the comment:

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 
#include 

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'

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39689] test_struct failure on s390x Fedora Clang buildbot

[Ammar Askar]

Ammar Askar  added the comment:

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

--
nosy: +ammar2

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39689] test_struct failure on s390x Fedora Clang buildbot

[Petr Viktorin]

Petr Viktorin  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39689] test_struct failure on s390x Fedora Clang buildbot

[Gregory P. Smith]

Gregory P. Smith  added the comment:

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

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

struct.unpack('?', ' ')

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39689] test_struct failure on s390x Fedora Clang buildbot

[STINNER Victor]

STINNER Victor  added the comment:

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-...@python.org/thread/O742VLCYX2AE3RWQK5RBQ3BGUOHESLF5/

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39689] test_struct failure on s390x Fedora Clang buildbot

[Petr Viktorin]

Petr Viktorin  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39689] test_struct failure on s390x Fedora Clang buildbot

[Gregory P. Smith]

Gregory P. Smith  added the comment:

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?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39689] test_struct failure on s390x Fedora Clang buildbot

[Gregory P. Smith]

Change by Gregory P. Smith :


--
components: +Extension Modules
nosy: +gregory.p.smith
stage:  -> needs patch
type:  -> behavior

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39689] test_struct failure on s390x Fedora Clang buildbot

[Benjamin Peterson]

Benjamin Peterson  added the comment:

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

--
nosy: +benjamin.peterson

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39689] test_struct failure on s390x Fedora Clang buildbot

[Petr Viktorin]

Petr Viktorin  added the comment:

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.)

--
nosy: +mark.dickinson, meador.inge

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39689] test_struct failure on s390x Fedora Clang buildbot

[Petr Viktorin]

Petr Viktorin  added the comment:

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.

--
nosy: +petr.viktorin

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39689] test_struct failure on s390x Fedora Clang buildbot

[Charalampos Stratakis]

Charalampos Stratakis  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39689] test_struct failure on s390x Fedora Clang buildbot

[Charalampos Stratakis]

Charalampos Stratakis  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue39689] test_struct failure on s390x Fedora Clang buildbot

[Charalampos Stratakis]

New submission from Charalampos Stratakis :

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.

--
components: Tests
messages: 362277
nosy: cstratak, vstinner
priority: normal
severity: normal
status: open
title: test_struct failure on s390x Fedora Clang buildbot
versions: Python 3.7, Python 3.8, Python 3.9

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com