Skip to content
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.

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

Increase MSRV to 1.38 and use ptr.cast::<T>() whenever practical to clarify casts. #425

Merged
merged 3 commits into from
May 26, 2024
Merged
Changes from 1 commit
Commits
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
Next Next commit
Use p.cast::<T>() instead of as as *mut T.
briansmith committed May 22, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
commit bdf0d716442eae47d58c1802ec264ea9217f042a
2 changes: 1 addition & 1 deletion src/apple-other.rs
Original file line number Diff line number Diff line change
@@ -14,7 +14,7 @@ extern "C" {
}

pub fn getrandom_inner(dest: &mut [MaybeUninit<u8>]) -> Result<(), Error> {
let ret = unsafe { CCRandomGenerateBytes(dest.as_mut_ptr() as *mut c_void, dest.len()) };
let ret = unsafe { CCRandomGenerateBytes(dest.as_mut_ptr().cast::<c_void>(), dest.len()) };
Copy link
Member

Choose a reason for hiding this comment

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

I think we can rely on type inference and use just .cast().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I intentionally avoid using type inference so that we ensure we're casting to the expected type. That is one of the goals of this change.

Copy link
Member

@newpavlov newpavlov May 20, 2024

Choose a reason for hiding this comment

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

I don't think it helps in any practical way in this case. In the first place, pointer types do not matter on the ABI level, so we are doing casting to just satisfy conventional signature of external functions. Also, IIRC std also uses a lot of pointer casts which rely on type inference in cases like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't seem to help in our code because our code does seem to be correct. But this style of explicitly naming the type has helped prevent bugs in practice in other code, and it makes the code easier to audit (from my own experience).

Copy link
Member

Choose a reason for hiding this comment

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

@josephlr WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

I personally like having the type explicit in the cast. It makes it easier for me to understand what's going on. It's not a huge deal, but it is nice to confirm that nothing unexpected is happening with the cast.

// kCCSuccess (from CommonCryptoError.h) is always zero.
if ret != 0 {
Err(Error::IOS_SEC_RANDOM)
2 changes: 1 addition & 1 deletion src/error.rs
Original file line number Diff line number Diff line change
@@ -99,7 +99,7 @@ impl Error {
cfg_if! {
if #[cfg(unix)] {
fn os_err(errno: i32, buf: &mut [u8]) -> Option<&str> {
let buf_ptr = buf.as_mut_ptr() as *mut libc::c_char;
let buf_ptr = buf.as_mut_ptr().cast::<libc::c_char>();
if unsafe { libc::strerror_r(errno, buf_ptr, buf.len()) } != 0 {
return None;
}
2 changes: 1 addition & 1 deletion src/fuchsia.rs
Original file line number Diff line number Diff line change
@@ -8,6 +8,6 @@ extern "C" {
}

pub fn getrandom_inner(dest: &mut [MaybeUninit<u8>]) -> Result<(), Error> {
unsafe { zx_cprng_draw(dest.as_mut_ptr() as *mut u8, dest.len()) }
unsafe { zx_cprng_draw(dest.as_mut_ptr().cast::<u8>(), dest.len()) }
Ok(())
}
4 changes: 2 additions & 2 deletions src/getentropy.rs
Original file line number Diff line number Diff line change
@@ -8,11 +8,11 @@
//!
//! For these targets, we use getentropy(2) because getrandom(2) doesn't exist.
use crate::{util_libc::last_os_error, Error};
use core::mem::MaybeUninit;
use core::{ffi::c_void, mem::MaybeUninit};

pub fn getrandom_inner(dest: &mut [MaybeUninit<u8>]) -> Result<(), Error> {
for chunk in dest.chunks_mut(256) {
let ret = unsafe { libc::getentropy(chunk.as_mut_ptr() as *mut libc::c_void, chunk.len()) };
let ret = unsafe { libc::getentropy(chunk.as_mut_ptr().cast::<c_void>(), chunk.len()) };
if ret != 0 {
return Err(last_os_error());
}
4 changes: 2 additions & 2 deletions src/getrandom.rs
Original file line number Diff line number Diff line change
@@ -16,10 +16,10 @@
//! nothing. On illumos, the default pool is used to implement getentropy(2),
//! so we assume it is acceptable here.
use crate::{util_libc::sys_fill_exact, Error};
use core::mem::MaybeUninit;
use core::{ffi::c_void, mem::MaybeUninit};

pub fn getrandom_inner(dest: &mut [MaybeUninit<u8>]) -> Result<(), Error> {
sys_fill_exact(dest, |buf| unsafe {
libc::getrandom(buf.as_mut_ptr() as *mut libc::c_void, buf.len(), 0)
libc::getrandom(buf.as_mut_ptr().cast::<c_void>(), buf.len(), 0)
})
}
2 changes: 1 addition & 1 deletion src/hermit.rs
Original file line number Diff line number Diff line change
@@ -13,7 +13,7 @@ extern "C" {

pub fn getrandom_inner(mut dest: &mut [MaybeUninit<u8>]) -> Result<(), Error> {
while !dest.is_empty() {
let res = unsafe { sys_read_entropy(dest.as_mut_ptr() as *mut u8, dest.len(), 0) };
let res = unsafe { sys_read_entropy(dest.as_mut_ptr().cast::<u8>(), dest.len(), 0) };
// Positive `isize`s can be safely casted to `usize`
if res > 0 && (res as usize) <= dest.len() {
dest = &mut dest[res as usize..];
4 changes: 2 additions & 2 deletions src/js.rs
Original file line number Diff line number Diff line change
@@ -40,7 +40,7 @@ pub(crate) fn getrandom_inner(dest: &mut [MaybeUninit<u8>]) -> Result<(), Error>
// have a notion of "uninitialized memory", this is purely
// a Rust/C/C++ concept.
let res = n.random_fill_sync(unsafe {
Uint8Array::view_mut_raw(chunk.as_mut_ptr() as *mut u8, chunk.len())
Uint8Array::view_mut_raw(chunk.as_mut_ptr().cast::<u8>(), chunk.len())
});
if res.is_err() {
return Err(Error::NODE_RANDOM_FILL_SYNC);
@@ -60,7 +60,7 @@ pub(crate) fn getrandom_inner(dest: &mut [MaybeUninit<u8>]) -> Result<(), Error>
}

// SAFETY: `sub_buf`'s length is the same length as `chunk`
unsafe { sub_buf.raw_copy_to_ptr(chunk.as_mut_ptr() as *mut u8) };
unsafe { sub_buf.raw_copy_to_ptr(chunk.as_mut_ptr().cast::<u8>()) };
}
}
};
4 changes: 2 additions & 2 deletions src/netbsd.rs
Original file line number Diff line number Diff line change
@@ -9,7 +9,7 @@ fn kern_arnd(buf: &mut [MaybeUninit<u8>]) -> libc::ssize_t {
libc::sysctl(
MIB.as_ptr(),
MIB.len() as libc::c_uint,
buf.as_mut_ptr() as *mut _,
buf.as_mut_ptr().cast::<c_void>(),
&mut len,
ptr::null(),
0,
@@ -38,7 +38,7 @@ pub fn getrandom_inner(dest: &mut [MaybeUninit<u8>]) -> Result<(), Error> {
if !fptr.is_null() {
let func: GetRandomFn = unsafe { core::mem::transmute(fptr) };
return sys_fill_exact(dest, |buf| unsafe {
func(buf.as_mut_ptr() as *mut u8, buf.len(), 0)
func(buf.as_mut_ptr().cast::<u8>(), buf.len(), 0)
});
}

4 changes: 2 additions & 2 deletions src/solaris.rs
Original file line number Diff line number Diff line change
@@ -13,13 +13,13 @@
//! https://blogs.oracle.com/solaris/post/solaris-new-system-calls-getentropy2-and-getrandom2
//! which also explains why this crate should not use getentropy(2).
use crate::{util_libc::last_os_error, Error};
use core::mem::MaybeUninit;
use core::{ffi::c_void, mem::MaybeUninit};

const MAX_BYTES: usize = 1024;

pub fn getrandom_inner(dest: &mut [MaybeUninit<u8>]) -> Result<(), Error> {
for chunk in dest.chunks_mut(MAX_BYTES) {
let ptr = chunk.as_mut_ptr() as *mut libc::c_void;
let ptr = chunk.as_mut_ptr().cast::<c_void>();
let ret = unsafe { libc::getrandom(ptr, chunk.len(), libc::GRND_RANDOM) };
// In case the man page has a typo, we also check for negative ret.
if ret <= 0 {
2 changes: 1 addition & 1 deletion src/solid.rs
Original file line number Diff line number Diff line change
@@ -7,7 +7,7 @@ extern "C" {
}

pub fn getrandom_inner(dest: &mut [MaybeUninit<u8>]) -> Result<(), Error> {
let ret = unsafe { SOLID_RNG_SampleRandomBytes(dest.as_mut_ptr() as *mut u8, dest.len()) };
let ret = unsafe { SOLID_RNG_SampleRandomBytes(dest.as_mut_ptr().cast::<u8>(), dest.len()) };
if ret >= 0 {
Ok(())
} else {
3 changes: 2 additions & 1 deletion src/use_file.rs
Original file line number Diff line number Diff line change
@@ -5,6 +5,7 @@ use crate::{
};
use core::{
cell::UnsafeCell,
ffi::c_void,
mem::MaybeUninit,
sync::atomic::{AtomicUsize, Ordering::Relaxed},
};
@@ -21,7 +22,7 @@ const FD_UNINIT: usize = usize::max_value();
pub fn getrandom_inner(dest: &mut [MaybeUninit<u8>]) -> Result<(), Error> {
let fd = get_rng_fd()?;
sys_fill_exact(dest, |buf| unsafe {
libc::read(fd, buf.as_mut_ptr() as *mut libc::c_void, buf.len())
libc::read(fd, buf.as_mut_ptr().cast::<c_void>(), buf.len())
})
}

2 changes: 1 addition & 1 deletion src/util_libc.rs
Original file line number Diff line number Diff line change
@@ -92,7 +92,7 @@ pub fn getrandom_syscall(buf: &mut [MaybeUninit<u8>]) -> libc::ssize_t {
unsafe {
libc::syscall(
libc::SYS_getrandom,
buf.as_mut_ptr() as *mut libc::c_void,
buf.as_mut_ptr().cast::<core::ffi::c_void>(),
buf.len(),
0,
) as libc::ssize_t
2 changes: 1 addition & 1 deletion src/vxworks.rs
Original file line number Diff line number Diff line change
@@ -20,7 +20,7 @@ pub fn getrandom_inner(dest: &mut [MaybeUninit<u8>]) -> Result<(), Error> {

// Prevent overflow of i32
for chunk in dest.chunks_mut(i32::max_value() as usize) {
let ret = unsafe { libc::randABytes(chunk.as_mut_ptr() as *mut u8, chunk.len() as i32) };
let ret = unsafe { libc::randABytes(chunk.as_mut_ptr().cast::<u8>(), chunk.len() as i32) };
if ret != 0 {
return Err(last_os_error());
}
2 changes: 1 addition & 1 deletion src/wasi.rs
Original file line number Diff line number Diff line change
@@ -7,7 +7,7 @@ use core::{
use wasi::random_get;

pub fn getrandom_inner(dest: &mut [MaybeUninit<u8>]) -> Result<(), Error> {
unsafe { random_get(dest.as_mut_ptr() as *mut u8, dest.len()) }.map_err(|e| {
unsafe { random_get(dest.as_mut_ptr().cast::<u8>(), dest.len()) }.map_err(|e| {
// The WASI errno will always be non-zero, but we check just in case.
match NonZeroU16::new(e.raw()) {
Some(r) => Error::from(NonZeroU32::from(r)),
7 changes: 4 additions & 3 deletions src/windows.rs
Original file line number Diff line number Diff line change
@@ -29,7 +29,7 @@ pub fn getrandom_inner(dest: &mut [MaybeUninit<u8>]) -> Result<(), Error> {
let ret = unsafe {
BCryptGenRandom(
ptr::null_mut(),
chunk.as_mut_ptr() as *mut u8,
chunk.as_mut_ptr().cast::<u8>(),
chunk.len() as u32,
BCRYPT_USE_SYSTEM_PREFERRED_RNG,
)
@@ -39,8 +39,9 @@ pub fn getrandom_inner(dest: &mut [MaybeUninit<u8>]) -> Result<(), Error> {
// Failed. Try RtlGenRandom as a fallback.
#[cfg(not(target_vendor = "uwp"))]
{
let ret =
unsafe { RtlGenRandom(chunk.as_mut_ptr() as *mut c_void, chunk.len() as u32) };
let ret = unsafe {
RtlGenRandom(chunk.as_mut_ptr().cast::<c_void>(), chunk.len() as u32)
};
if ret != 0 {
continue;
}