refinements to unsafe code #157
Merged
Conversation
This file contains 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
folkertdev
commented
Aug 21, 2024
Comment on lines
-59
to
-82
| #[allow(unreachable_code)] | ||
| const DEFAULT_ZALLOC: Option<alloc_func> = '_blk: { | ||
| // this `break 'blk'` construction exists to generate just one compile error and not other | ||
| // warnings when multiple allocators are configured. | ||
|
|
||
| #[cfg(feature = "c-allocator")] | ||
| break '_blk Some(zlib_rs::allocate::Allocator::C.zalloc); | ||
|
|
||
| #[cfg(feature = "rust-allocator")] | ||
| break '_blk Some(zlib_rs::allocate::Allocator::RUST.zalloc); | ||
|
|
||
| None | ||
| }; | ||
|
|
||
| #[allow(unreachable_code)] | ||
| const DEFAULT_ZFREE: Option<free_func> = '_blk: { | ||
| #[cfg(feature = "c-allocator")] | ||
| break '_blk Some(zlib_rs::allocate::Allocator::C.zfree); | ||
|
|
||
| #[cfg(feature = "rust-allocator")] | ||
| break '_blk Some(zlib_rs::allocate::Allocator::RUST.zfree); | ||
|
|
||
| None | ||
| }; |
There was a problem hiding this comment.
this is handled by zlib-rs. Only one allocator is available there because of the compile_error on line 66
Comment on lines
+229
to
+231
| /// * Either | ||
| /// - `destLen` is `NULL` | ||
| /// - `destLen` satisfies the requirements of `&mut *destLen` |
There was a problem hiding this comment.
stock zlib happily dereferences a null pointer here. We check for it being null and return an error, which, because zlib has undefined behavior in this case, is compatible.
Comment on lines
411
to
413
| fn initialize_header_null() -> MaybeUninit<gz_header> { | ||
| let mut head = MaybeUninit::<gz_header>::uninit(); | ||
| let mut head = MaybeUninit::<gz_header>::zeroed(); | ||
|
|
There was a problem hiding this comment.
this test uses some low-level operations still, but we now assume that the memory is initialized, so we have to use zeroed here.
folkertdev
force-pushed
the
assume-initialized-memory
branch
from
46f3756
to
c8e4d43
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
among other things
InitandGetHeaderfunctions now assume that their arguments are properly initialized in the rust sense. Providing uninitialized memory to these functions is UB, though unlikely to actually cause issues across an FFI boundary