Fix static-size FFI type aliases #463
Merged
davidhewitt merged 2 commits intoPyO3:mainfrom Oct 27, 2024
Merged
Conversation
Several type aliases with statically sized names (e.g. `npy_uint64`) were being aliased to platform-dependent types like `::std::os::raw::c_long`. Most of these were barely used, so it seems like they didn't cause problems before, but now with `npy_uint64` being used in certain structs, which are the target of pointer punning, the mismatch in sizes became visible. For example, `npy_uint64` was previously typedef'd to `c_ulong`, which is 64 bits on 64-bit platforms and on all Unix-likes, but 32 bits on Windows 32-bit. This caused inaccurate reads through pointers to structs containing them, eventually resulting in attempts to derefence null pointers.
The parent commit fixes the problems with 32-bit Windows, this one simply re-enables the build and re-activates the CI runs of it.
This was referenced Oct 21, 2024
Contributor
|
Thanks for digging into this! |
Closed
davidhewitt
reviewed
Oct 26, 2024
Member
davidhewitt
left a comment
There was a problem hiding this comment.
Thanks, looks very promising! Have triggered CI...
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Several type aliases with statically sized names (e.g.
npy_uint64) were being aliased to platform-dependent types like::std::os::raw::c_long. Most of these were barely used, so it seems like they didn't cause problems before, but now withnpy_uint64being used in certain structs, which are the target of pointer punning, the mismatch in sizes became visible. For example,npy_uint64was previously typedef'd toc_ulong, which is 64 bits on 64-bit platforms and on all Unix-likes, but 32 bits on Windows 32-bit. This caused inaccurate reads through pointers to structs containing them, eventually resulting in attempts to dereference null pointers.That appears to be the only problem with win32 support, so the second commit re-adds it to the CI matrix and removes the enforced compiler error.
Fix #448