struct and memoryview tests rely on undefined behavior (as revealed by clang 9)

@gpshead

BPO	39689
Nosy	@gpshead, @ronaldoussoren, @mdickinson, @vstinner, @benjaminp, @encukou, @skrah, @meadori, @stratakis, @ammaraskar, @miss-islington
PRs	bpo-39689: _struct: Avoid undefined behavior when loading native _Bool #18925 bpo-39689: Do not test undefined casts to _Bool (GH-18964) #18964 [3.7] bpo-39689: Do not test undefined casts to _Bool (GH-18964) #18965 [3.8] bpo-39689: Do not test undefined casts to _Bool (GH-18964) #18966 bpo-39689: Do not use native packing for format "?" with standard size #18969 [3.7] bpo-39689: Do not use native packing for format "?" with standard size (GH-18969) #19154 [3.8] bpo-39689: Do not use native packing for format "?" with standard size (GH-18969) #19155

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2020-04-07.15:12:29.798>
created_at = <Date 2020-02-19.15:01:49.939>
labels = ['type-bug', '3.8', '3.9', 'extension-modules', '3.7', 'tests']
title = 'struct and memoryview tests rely on undefined behavior (as revealed by clang 9)'
updated_at = <Date 2020-12-08.00:44:33.929>
user = 'https://github.com/stratakis'

bugs.python.org fields:

activity = <Date 2020-12-08.00:44:33.929>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2020-04-07.15:12:29.798>
closer = 'petr.viktorin'
components = ['Extension Modules', 'Tests']
creation = <Date 2020-02-19.15:01:49.939>
creator = 'cstratak'
dependencies = []
files = []
hgrepos = []
issue_num = 39689
keywords = ['patch']
message_count = 43.0
messages = ['362277', '362278', '362698', '362811', '362815', '362829', '362833', '363275', '363284', '363294', '363299', '363305', '363310', '363472', '363924', '363927', '363930', '363931', '363933', '363954', '363955', '363966', '363978', '364008', '364025', '364026', '364027', '364038', '364042', '364043', '364044', '364437', '364450', '364472', '364501', '364506', '364511', '364927', '364928', '364933', '365382', '365383', '382703']
nosy_count = 11.0
nosy_names = ['gregory.p.smith', 'ronaldoussoren', 'mark.dickinson', 'vstinner', 'benjamin.peterson', 'petr.viktorin', 'skrah', 'meador.inge', 'cstratak', 'ammar2', 'miss-islington']
pr_nums = ['18925', '18964', '18965', '18966', '18969', '19154', '19155']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue39689'
versions = ['Python 3.7', 'Python 3.8', 'Python 3.9']

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?

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:

The '?' conversion code corresponds to the _Bool type defined by C99
... 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. #63169 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.

#63169 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 472fc84 by Stefan Krah in branch 'master':
bpo-39689: Do not use native packing for format "?" with standard size (GH-18969)
472fc84

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 0f9e889 by Miss Islington (bot) in branch '3.7':
bpo-39689: Do not use native packing for format "?" with standard size (GH-18969)
0f9e889

New changeset 572ef74 by Miss Islington (bot) in branch '3.8':
bpo-39689: Do not use native packing for format "?" with standard size (GH-18969)
572ef74

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

Navigation Menu

Repository navigation

struct and memoryview tests rely on undefined behavior (as revealed by clang 9) #83870

24 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

struct and memoryview tests rely on undefined behavior (as revealed by clang 9) #83870

Description

Activity

stratakis commented on Feb 19, 2020

stratakis commented on Feb 19, 2020

stratakis commented on Feb 26, 2020

encukou commented on Feb 27, 2020

encukou commented on Feb 27, 2020

benjaminp commented on Feb 27, 2020

gpshead commented on Feb 27, 2020

24 remaining items

skrah commented on Mar 12, 2020

encukou commented on Mar 17, 2020

skrah commented on Mar 17, 2020

ronaldoussoren commented on Mar 17, 2020

ronaldoussoren commented on Mar 18, 2020

encukou commented on Mar 18, 2020

skrah commented on Mar 18, 2020

miss-islington commented on Mar 24, 2020

encukou commented on Mar 24, 2020

encukou commented on Mar 24, 2020

miss-islington commented on Mar 31, 2020

miss-islington commented on Mar 31, 2020

vstinner commented on Dec 8, 2020

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions