gh-125118: don't copy arbitrary values to _Bool in the struct module

[skirpichev]

memcopy'ing arbitrary values to _Bool variable triggers undefined behaviour, e.g.:

/* a.c */
#include <stdbool.h>
#include <stdio.h>
#include <string.h>
int main(void)
{
    _Bool x;
    char y = 2;
    memcpy(&x, &y, 1);
    printf("%d\n", x != 0);
    return 0;
}

$ gcc-12 a.c -Wall && ./a.out
2
$ clang-15 a.c -Wall && ./a.out
0
$ gcc-12 a.c -Wall -fsanitize=undefined && ./a.out
a.c:9:5: runtime error: load of value 2, which is not a valid value for type '_Bool'
0
$ clang-15 a.c -Wall -fsanitize=undefined && ./a.out
a.c:9:20: runtime error: load of value 2, which is not a valid value for type '_Bool'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior a.c:9:20 in
0

Cridits to Alex Gaynor.

• Issue: struct module has undefined behavior when loading bools #125118

[vstinner]

LGTM, but I would prefer to have a second review on this change.

@encukou: You worked on a similar issue some time ago, would you mind to review this change?

[encukou]

Yes, please wait for my review. I'll get to it this week.

[ZeroIntensity]

I think this looks fine. Out of curiosity, what bug was this causing in practice?

[alex]

It was triggering UBSAN warnings, because it was UB. AFAIK it wasn't misbehaving on any platform yet, but it'd have been within the compiler's right to rewrite != 0 as == 1, and that would have changed behavior.

Should we add a static_assert(sizeof(_Bool) == 1);? Otherwise LGTM.

[skirpichev]

@ZeroIntensity, you could try added tests on gcc-12 (PASS) and clang-17 (FAIL). I suspect specific versions aren't important.

[encukou]

As far as I can see, the safest assumption we can make is that false is all zero bytes.

To allow bigger sizeof(_Bool), that would be:

    static const _Bool false_bool = 0;
    return PyBool_FromLong(memcmp(p, &false_bool, sizeof(_Bool)));

[vstinner]

static const _Bool false_bool = 0;

I don't think that static is needed here.

[vstinner]

LGTM.

[encukou]

Thank you!

[miss-islington-app]

Thanks @skirpichev for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

[miss-islington-app]

Thanks @skirpichev for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

[miss-islington-app]

Sorry, @skirpichev and @encukou, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 87d7315ac57250046372b0d9ae4619ba619c8c87 3.13

[miss-islington-app]

Sorry, @skirpichev and @encukou, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 87d7315ac57250046372b0d9ae4619ba619c8c87 3.12

[skirpichev]

Working on backports...

[bedevere-app]

GH-125263 is a backport of this pull request to the 3.13 branch.

[bedevere-app]

GH-125265 is a backport of this pull request to the 3.12 branch.