Skip to content

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

Merged
encukou merged 6 commits intopython:mainfrom
skirpichev:nu_bool-125118
Oct 10, 2024
Merged

gh-125118: don't copy arbitrary values to _Bool in the struct module#125169
encukou merged 6 commits intopython:mainfrom
skirpichev:nu_bool-125118

Conversation

@skirpichev
Copy link
Member

@skirpichev skirpichev commented Oct 9, 2024 •
edited
Loading

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.

@skirpichev
pythongh-125118: don't copy arbitrary values to _Bool in the struct m…
656ba8e 
…odule

memcopy'ing arbitrary values to _Bool variable triggers undefined
behaviour, e.g.:
```c
/* a.c */
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.
@bedevere-app bedevere-app bot mentioned this pull request Oct 9, 2024
Closed
vstinner
vstinner approved these changes Oct 9, 2024
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

encukou commented Oct 9, 2024

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

ZeroIntensity
ZeroIntensity approved these changes Oct 9, 2024
View reviewed changes
Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@alex
Copy link
Member

alex commented Oct 9, 2024

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
Copy link
Member Author

skirpichev commented Oct 9, 2024

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

colesbury
colesbury reviewed Oct 9, 2024
View reviewed changes
@encukou
Copy link
Member

encukou commented Oct 9, 2024

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
Copy link
Member

vstinner commented Oct 9, 2024

static const _Bool false_bool = 0;

I don't think that static is needed here.

@skirpichev @colesbury
+ adapt for sizeof(_Bool) > 1
ee99e65 
Co-authored-by: Sam Gross <colesbury@gmail.com>
@skirpichev skirpichev requested a review from encukou October 9, 2024 15:09
vstinner
vstinner reviewed Oct 9, 2024
View reviewed changes
vstinner
vstinner reviewed Oct 9, 2024
View reviewed changes
vstinner
vstinner reviewed Oct 9, 2024
View reviewed changes
@skirpichev @vstinner
Update Modules/_struct.c
02976e7 
Co-authored-by: Victor Stinner <vstinner@python.org>
vstinner
vstinner approved these changes Oct 9, 2024
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

encukou
encukou reviewed Oct 9, 2024
View reviewed changes
@skirpichev @encukou
Update Modules/_struct.c
78291e4 
Co-authored-by: Petr Viktorin <encukou@gmail.com>
@encukou encukou merged commit 87d7315 into python:main Oct 10, 2024
@encukou
Copy link
Member

encukou commented Oct 10, 2024

Thank you!

@encukou encukou added the needs backport to 3.12 only security fixes label Oct 10, 2024
@encukou encukou added the needs backport to 3.13 bugs and security fixes label Oct 10, 2024
@miss-islington-app
Copy link

miss-islington-app bot commented Oct 10, 2024

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
Copy link

miss-islington-app bot commented Oct 10, 2024

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

@miss-islington-app
Copy link

miss-islington-app bot commented Oct 10, 2024

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
Copy link

miss-islington-app bot commented Oct 10, 2024

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 skirpichev deleted the nu_bool-125118 branch October 10, 2024 13:51
@skirpichev
Copy link
Member Author

skirpichev commented Oct 10, 2024

Working on backports...

skirpichev added a commit to skirpichev/cpython that referenced this pull request Oct 10, 2024
@skirpichev @colesbury
@vstinner @encukou
[3.13] pythongh-125118: don't copy arbitrary values to _Bool in the s…
ad7a561 
…truct module (pythonGH-125169)

memcopy'ing arbitrary values to _Bool variable triggers undefined
behaviour. Avoid this.
We assume that `false` is represented by all zero bytes.

Credits to Alex Gaynor.

(cherry picked from commit 87d7315)

Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
Co-authored-by: Sam Gross <colesbury@gmail.com>
Co-authored-by: Victor Stinner <vstinner@python.org>
Co-authored-by: Petr Viktorin <encukou@gmail.com>
@bedevere-app
Copy link

bedevere-app bot commented Oct 10, 2024

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

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Oct 10, 2024
skirpichev added a commit to skirpichev/cpython that referenced this pull request Oct 10, 2024
@skirpichev @colesbury
@vstinner @encukou
[3.12] pythongh-125118: don't copy arbitrary values to _Bool in the s…
aa2a3d5 
…truct module (pythonGH-125169)

memcopy'ing arbitrary values to _Bool variable triggers undefined
behaviour. Avoid this.
We assume that `false` is represented by all zero bytes.

Credits to Alex Gaynor.

(cherry picked from commit 87d7315)

Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
Co-authored-by: Sam Gross <colesbury@gmail.com>
Co-authored-by: Victor Stinner <vstinner@python.org>
Co-authored-by: Petr Viktorin <encukou@gmail.com>
@bedevere-app
Copy link

bedevere-app bot commented Oct 10, 2024

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

@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label Oct 10, 2024
vstinner added a commit that referenced this pull request Oct 10, 2024
@skirpichev @colesbury
@vstinner @encukou
[3.12] gh-125118: don't copy arbitrary values to _Bool in the struct …
67f8302 
…module (GH-125169) (#125265)

memcopy'ing arbitrary values to _Bool variable triggers undefined
behaviour. Avoid this.
We assume that `false` is represented by all zero bytes.

Credits to Alex Gaynor.

(cherry picked from commit 87d7315)

Co-authored-by: Sam Gross <colesbury@gmail.com>
Co-authored-by: Victor Stinner <vstinner@python.org>
Co-authored-by: Petr Viktorin <encukou@gmail.com>
vstinner added a commit that referenced this pull request Oct 10, 2024
@skirpichev @colesbury
@vstinner @encukou
[3.13] gh-125118: don't copy arbitrary values to _Bool in the struct …
c2cb1a8 
…module (GH-125169) (#125263)

memcopy'ing arbitrary values to _Bool variable triggers undefined
behaviour. Avoid this.
We assume that `false` is represented by all zero bytes.

Credits to Alex Gaynor.

(cherry picked from commit 87d7315)

Co-authored-by: Sam Gross <colesbury@gmail.com>
Co-authored-by: Victor Stinner <vstinner@python.org>
Co-authored-by: Petr Viktorin <encukou@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@encukou encukou encukou left review comments

@colesbury colesbury colesbury left review comments

@alex alex alex approved these changes

@vstinner vstinner vstinner approved these changes

@ZeroIntensity ZeroIntensity ZeroIntensity approved these changes

Assignees

@encukou encukou

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@skirpichev @encukou @alex @vstinner @colesbury @ZeroIntensity