Skip to content

bpo-39689: _struct: Avoid undefined behavior when loading native _Bool #18925

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
encukou wants to merge 3 commits into from
Closed

bpo-39689: _struct: Avoid undefined behavior when loading native _Bool #18925

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0ab0be8
bpo-39689: _struct: Avoid undefined behavior when loading native _Bool
encukou Mar 11, 2020
92b0e84
test_buffer: Use subTest() to help debug a failure
encukou Mar 11, 2020
38c78c8
Use the same strategy also for memoryview
encukou Mar 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Use the same strategy also for memoryview
  • Loading branch information
@encukou
encukou committed Mar 11, 2020
commit 38c78c851eebbb0852d9efdd84f5c415a8761a9f
6 changes: 3 additions & 3 deletions Misc/NEWS.d/next/Library/2020-03-11-15-46-59.bpo-39689.uzQhJX.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
The struct module now loads native booleans as ``char`` rather than
``_Bool`` to avoid triggering undefined behavior. The behavior is the same
on all supported (tested) platforms.
The struct module and memoryview now load native booleans as ``char`` rather
than ``_Bool`` to avoid triggering undefined behavior. For valid native _Bool,
the behavior is the same on all supported (tested) platforms.
4 changes: 3 additions & 1 deletion Objects/memoryobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1699,7 +1699,9 @@ unpack_single(const char *ptr, const char *fmt)
case 'l': UNPACK_SINGLE(ld, ptr, long); goto convert_ld;

/* boolean */
case '?': UNPACK_SINGLE(ld, ptr, _Bool); goto convert_bool;
// memcpy-ing values other than 0 or 1 to a _Bool variable triggers
// undefined behavior, so cast from char instead. See bpo-39689.
case '?': ld = (_Bool)*ptr; goto convert_bool;

@skrah skrah Mar 11, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In theory this can trigger an unaligned access if sizeof(_Bool) > 1. Unaligned accesses that result in bus errors are the main reason for the memcpy().

Also I wonder if UB checkers can't get smart enough to figure out that e.g. (_Bool)*ptr == 42 and then complain.

@encukou encukou Mar 12, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

In theory this can trigger an unaligned access if sizeof(_Bool) > 1. Unaligned accesses that result in bus errors are the main reason for the memcpy().

Ah! You are right, thank you. This PR won't work as is. I won't have time to work on this soon, so I'll close it in the mean time.

Also I wonder if UB checkers can't get smart enough to figure out that e.g. (_Bool)*ptr == 42 and then complain.

I don't have access to the C standard, but from what I've read, casting to _Bool will convert non-zero values to 1. So this should be fine.

@skrah skrah Mar 12, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So this should be fine.

Yes, I misread the construct as *(_Bool *)ptr), I now see that you are deliberately dereferencing the char pointer.

Hmm, the rank of _Bool is the smallest, but sizeof(char) can still be larger than sizeof(_Bool). So I wouldn't want this construct either.

@encukou encukou Mar 12, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

sizeof(char) is guaranteed to be 1. It can't be larger than sizeof(_Bool).
Alignment may be a different matter (or may not; I don't know).


/* unsigned integers */
case 'H': UNPACK_SINGLE(lu, ptr, unsigned short); goto convert_lu;
Expand Down