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

Replace more casts with safer conversions & enable cast-related lints. #445

Closed
wants to merge 7 commits into from
16 changes: 7 additions & 9 deletions src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,18 +94,16 @@ impl Error {
/// [1]: https://doc.rust-lang.org/std/io/struct.Error.html#method.raw_os_error
#[inline]
pub fn raw_os_error(self) -> Option<i32> {
if self.0.get() < Self::INTERNAL_START {
match () {
#[cfg(target_os = "solid_asp3")]
const _: () = assert!(i32::MAX.unsigned_abs() == Error::INTERNAL_START - 1);
i32::try_from(self.0.get()).ok().map(|errno| {
if cfg!(target_os = "solid_asp3") {
// On SOLID, negate the error code again to obtain the original
// error code.
() => Some(-(self.0.get() as i32)),
#[cfg(not(target_os = "solid_asp3"))]
() => Some(self.0.get() as i32),
-errno
} else {
errno
}
} else {
None
}
})
}

/// Extract the bare error code.
Expand Down
18 changes: 7 additions & 11 deletions src/hermit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,18 +9,14 @@ 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().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..];
} else {
let err = if res < 0 {
u32::try_from(res.unsigned_abs())
match usize::try_from(res) {
Ok(res) if res > 0 => dest = dest.get_mut(res..).ok_or(Error::UNEXPECTED)?,
_ => {
let err = u32::try_from(res.unsigned_abs())
.ok()
.map_or(Error::UNEXPECTED, Error::from_os_error)
} else {
Error::UNEXPECTED
};
return Err(err);
.map_or(Error::UNEXPECTED, Error::from_os_error);
return Err(err);
}
}
}
Ok(())
Expand Down
7 changes: 4 additions & 3 deletions src/js.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use wasm_bindgen::{prelude::wasm_bindgen, JsCast, JsValue};

// Size of our temporary Uint8Array buffer used with WebCrypto methods
// Maximum is 65536 bytes see https://developer.mozilla.org/en-US/docs/Web/API/Crypto/getRandomValues
const WEB_CRYPTO_BUFFER_SIZE: usize = 256;
const WEB_CRYPTO_BUFFER_SIZE: u16 = 256;
// Node.js's crypto.randomFillSync requires the size to be less than 2**31.
const NODE_MAX_BUFFER_SIZE: usize = (1 << 31) - 1;

Expand Down Expand Up @@ -50,9 +50,10 @@ pub(crate) fn getrandom_inner(dest: &mut [MaybeUninit<u8>]) -> Result<(), Error>
RngSource::Web(crypto, buf) => {
// getRandomValues does not work with all types of WASM memory,
// so we initially write to browser memory to avoid exceptions.
for chunk in dest.chunks_mut(WEB_CRYPTO_BUFFER_SIZE) {
for chunk in dest.chunks_mut(WEB_CRYPTO_BUFFER_SIZE.into()) {
// The chunk can be smaller than buf's length, so we call to
// JS to create a smaller view of buf without allocation.
#[allow(clippy::cast_possible_truncation)]
Copy link
Member

Choose a reason for hiding this comment

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

You can use .try_into().expect("..") here same as in windows7.

let sub_buf = buf.subarray(0, chunk.len() as u32);

if crypto.get_random_values(&sub_buf).is_err() {
Expand Down Expand Up @@ -95,7 +96,7 @@ fn getrandom_init() -> Result<RngSource, Error> {
},
};

let buf = Uint8Array::new_with_length(WEB_CRYPTO_BUFFER_SIZE as u32);
let buf = Uint8Array::new_with_length(WEB_CRYPTO_BUFFER_SIZE.into());
Ok(RngSource::Web(crypto, buf))
}

Expand Down
18 changes: 18 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,24 @@
#![no_std]
#![warn(rust_2018_idioms, unused_lifetimes, missing_docs)]
#![cfg_attr(docsrs, feature(doc_auto_cfg))]
#![deny(
clippy::cast_lossless,
clippy::cast_possible_truncation,
clippy::cast_possible_wrap,
clippy::cast_precision_loss,
clippy::cast_ptr_alignment,
clippy::cast_sign_loss,
clippy::char_lit_as_u8,
clippy::checked_conversions,
clippy::fn_to_numeric_cast,
clippy::fn_to_numeric_cast_with_truncation,
clippy::ptr_as_ptr,
clippy::unnecessary_cast,
clippy::useless_conversion
)]
Comment on lines +211 to +225
Copy link
Member

@josephlr josephlr Jun 21, 2024

Choose a reason for hiding this comment

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

Would it be possible to deny entire categories of lints (pedantic, correctness, complexity, etc...) rather than specific ones?

Copy link
Member

Choose a reason for hiding this comment

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

Also, with the number of additional lints it may be better to enable them in clippy.toml.

// `clippy::cast_ref_to_mut` was replaced by `invalid_reference_casting` in 1.73.
#![allow(renamed_and_removed_lints)]
#![deny(clippy::cast_ref_to_mut)]

#[macro_use]
extern crate cfg_if;
Expand Down
14 changes: 11 additions & 3 deletions src/linux_android.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,20 @@ pub fn getrandom_inner(dest: &mut [MaybeUninit<u8>]) -> Result<(), Error> {

// Also used by linux_android_with_fallback to check if the syscall is available.
pub fn getrandom_syscall(buf: &mut [MaybeUninit<u8>]) -> libc::ssize_t {
unsafe {
let res: libc::c_long = unsafe {
libc::syscall(
libc::SYS_getrandom,
buf.as_mut_ptr().cast::<core::ffi::c_void>(),
buf.len(),
0,
) as libc::ssize_t
}
)
};

// c_long to ssize_t conversion is lossless.
const _: () =
assert!(core::mem::size_of::<libc::c_long>() == core::mem::size_of::<libc::ssize_t>());
#[allow(clippy::cast_possible_truncation)]
let res = res as libc::ssize_t;

res
}
6 changes: 3 additions & 3 deletions src/util_libc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,9 @@ pub fn sys_fill_exact(
) -> Result<(), Error> {
while !buf.is_empty() {
let res = sys_fill(buf);
match res {
res if res > 0 => buf = buf.get_mut(res as usize..).ok_or(Error::UNEXPECTED)?,
-1 => {
match usize::try_from(res) {
Ok(res) if res > 0 => buf = buf.get_mut(res..).ok_or(Error::UNEXPECTED)?,
Err(_) if res == -1 => {
let err = last_os_error();
// We should try again if the call was interrupted.
if err.raw_os_error() != Some(libc::EINTR) {
Expand Down
4 changes: 3 additions & 1 deletion src/windows7.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@ pub fn getrandom_inner(dest: &mut [MaybeUninit<u8>]) -> Result<(), Error> {
// Prevent overflow of u32
let chunk_size = usize::try_from(i32::MAX).expect("Windows does not support 16-bit targets");
for chunk in dest.chunks_mut(chunk_size) {
let ret = unsafe { RtlGenRandom(chunk.as_mut_ptr().cast::<c_void>(), chunk.len() as u32) };
#[allow(clippy::cast_possible_truncation)]
let chunk_len = chunk.len() as u32;
Copy link
Member

Choose a reason for hiding this comment

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

You can write it as chunk.len().try_into().expect("chunk size is bounded by i32::MAX"). Compiler is able to properly remove such panic.

let ret = unsafe { RtlGenRandom(chunk.as_mut_ptr().cast::<c_void>(), chunk_len) };
if ret != TRUE {
return Err(Error::WINDOWS_RTL_GEN_RANDOM);
}
Expand Down
Loading