Skip to content

Make unsound code opt-in. #51

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Make unsound code opt-in. #51

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ members = [
[package]
name = "bitcode"
authors = [ "Cai Bear", "Finn Bear" ]
version = "0.6.3"
version = "0.6.4"
edition = "2021"
license = "MIT OR Apache-2.0"
repository = "https://github.com/SoftbearStudios/bitcode"
Expand Down Expand Up @@ -38,6 +38,7 @@ zstd = "0.13.0"
[features]
derive = [ "dep:bitcode_derive" ]
std = [ "serde?/std", "glam?/std", "arrayvec?/std" ]
unsound = [] # use benign undefined behavior for more performance. Miri will detect this. See GitHub Wiki "Security".
default = [ "derive", "std" ]

[package.metadata.docs.rs]
Expand Down
43 changes: 24 additions & 19 deletions src/derive/vec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,28 +53,33 @@ macro_rules! unsafe_wild_copy {
([$T:ident; $N:ident], $src:ident, $dst:ident, $n:ident) => {
debug_assert!($n != 0 && $n <= $N);

let page_size = 4096;
let read_size = core::mem::size_of::<[$T; $N]>();
let within_page = $src as usize & (page_size - 1) < (page_size - read_size) && cfg!(all(
// Miri doesn't like this.
not(miri),
// cargo fuzz's memory sanitizer complains about buffer overrun.
// Without nightly we can't detect memory sanitizers, so we check debug_assertions.
not(debug_assertions),
// x86/x86_64/aarch64 all have min page size of 4096, so reading past the end of a non-empty
// buffer won't page fault.
any(target_arch = "x86", target_arch = "x86_64", target_arch = "aarch64")
));
// Note: Neither Miri nor the fuzzer's memory sanitizer like this feature. We prevent
// the latter from seeing it with `not(debug_assertions)`.
#[cfg(all(feature = "unsound", not(debug_assertions)))]
{
let page_size = 4096;
Copy link

Choose a reason for hiding this comment

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

Hello
I was skimming rust serialization libraries and somewhat ended up here.

I'm not 100% sure, but I think that for posix systems the hardcoded page size could be replaced by something likesysconf(PAGE_SIZE); (man sysconf) then in theory be sound on compliant posix systems.

Feel free to take or toss my comment

have a nice day.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hello, thanks for your comment. Note that this optimization is most efficient if the page size is known statically and sysconf appears to retrieve it at runtime. AFAIK 4096 is safe because the page size won't be lower than 4096.

Copy link

Choose a reason for hiding this comment

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

Hello, thanks for your comment. Note that this optimization is most efficient if the page size is known statically and sysconf appears to retrieve it at runtime. AFAIK 4096 is safe because the page size won't be lower than 4096.

Oh makes sense for the compile time optimisations.

Sysconf at runtime could also be used to choose the correct implementation something akin to:

if sysconf(SC_PAGESIZE) == 4096 { // or using something like a lazy static
// read_mem aligned
} else {
ptr.copy_non_overlapping();
}

Downside: using sysconf would introduce a dependency on libc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well a higher page size wouldn't require falling back to copy_non_overlapping. It would just slightly reduce the probability of the fallback, as the fast path could copy over 4096 byte boundaries within the larger page. The run-time overhead of this check (at least one predicted branch) might outweigh the benefits of this.

A lower page size would require something like this but, AFAIK, that's not possible.

let read_size = core::mem::size_of::<[$T; $N]>();
let within_page = $src as usize & (page_size - 1) < (page_size - read_size) && cfg!(any(
// x86/x86_64/aarch64 all have min page size of 4096, so reading past the end of a non-empty
// buffer won't page fault.
target_arch = "x86",
target_arch = "x86_64",
target_arch = "aarch64",
));

if within_page {
*($dst as *mut core::mem::MaybeUninit<[$T; $N]>) = core::ptr::read($src as *const core::mem::MaybeUninit<[$T; $N]>);
} else {
#[cold]
unsafe fn cold<T>(src: *const T, dst: *mut T, n: usize) {
src.copy_to_nonoverlapping(dst, n);
if within_page {
*($dst as *mut core::mem::MaybeUninit<[$T; $N]>) = core::ptr::read($src as *const core::mem::MaybeUninit<[$T; $N]>);
} else {
#[cold]
unsafe fn cold<T>(src: *const T, dst: *mut T, n: usize) {
src.copy_to_nonoverlapping(dst, n);
}
cold($src, $dst, $n);
}
cold($src, $dst, $n);
}

#[cfg(any(not(feature = "unsound"), debug_assertions))]
$src.copy_to_nonoverlapping($dst, $n);
}
}
pub(crate) use unsafe_wild_copy;
Expand Down
Loading