Skip to content

Replace as casts with safer conversions #510

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

Merged
merged 1 commit into from
Oct 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
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
1 change: 0 additions & 1 deletion .clippy.toml

This file was deleted.

2 changes: 1 addition & 1 deletion .github/workflows/workspace.yml
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ jobs:
# Fixed Nigthly version is used to prevent
# CI failures which are not relevant to PR changes
# on introduction of new Clippy lints.
toolchain: nightly-2024-09-04
toolchain: nightly-2024-10-08
components: clippy,rust-src
- name: std feature
run: cargo clippy --features std
Expand Down
18 changes: 7 additions & 11 deletions src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,18 +88,14 @@ 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")]
// 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),
i32::try_from(self.0.get()).ok().map(|errno| {
// On SOLID, negate the error code again to obtain the original error code.
if cfg!(target_os = "solid_asp3") {
-errno
} else {
errno
}
} else {
None
}
})
}

/// Creates a new instance of an `Error` from a particular custom error code.
Expand Down
21 changes: 10 additions & 11 deletions src/hermit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,18 +9,17 @@ 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 res {
res if res > 0 => {
let len = usize::try_from(res).map_err(|_| Error::UNEXPECTED)?;
dest = dest.get_mut(len..).ok_or(Error::UNEXPECTED)?;
}
code => {
let err = u32::try_from(code.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
12 changes: 8 additions & 4 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,10 +50,14 @@ 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()) {
let chunk_len: u32 = chunk
.len()
.try_into()
.expect("chunk length is bounded by WEB_CRYPTO_BUFFER_SIZE");
// The chunk can be smaller than buf's length, so we call to
// JS to create a smaller view of buf without allocation.
let sub_buf = buf.subarray(0, chunk.len() as u32);
let sub_buf = buf.subarray(0, chunk_len);

if crypto.get_random_values(&sub_buf).is_err() {
return Err(Error::WEB_GET_RANDOM_VALUES);
Expand Down Expand Up @@ -95,7 +99,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
15 changes: 15 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,21 @@
#![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
)]

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

// 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
}
)
};

const _: () =
assert!(core::mem::size_of::<libc::c_long>() == core::mem::size_of::<libc::ssize_t>());
res.try_into()
.expect("c_long to ssize_t conversion is lossless")
Copy link
Member Author

@newpavlov newpavlov Oct 9, 2024

Choose a reason for hiding this comment

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

This cast will be no longer needed if #508 lands.

}
16 changes: 4 additions & 12 deletions src/netbsd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,26 +19,18 @@ unsafe extern "C" fn polyfill_using_kern_arand(
) -> libc::ssize_t {
debug_assert_eq!(flags, 0);

static MIB: [libc::c_int; 2] = [libc::CTL_KERN, libc::KERN_ARND];
const MIB_LEN: libc::c_uint = 2;
static MIB: [libc::c_int; MIB_LEN as usize] = [libc::CTL_KERN, libc::KERN_ARND];

// NetBSD will only return up to 256 bytes at a time, and
// older NetBSD kernels will fail on longer buffers.
let mut len = cmp::min(buflen, 256);

let ret = unsafe {
libc::sysctl(
MIB.as_ptr(),
MIB.len() as libc::c_uint,
buf,
&mut len,
ptr::null(),
0,
)
};
let ret = unsafe { libc::sysctl(MIB.as_ptr(), MIB_LEN, buf, &mut len, ptr::null(), 0) };
if ret == -1 {
-1
} else {
len as libc::ssize_t
libc::ssize_t::try_from(len).expect("len is bounded by 256")
}
}

Expand Down
5 changes: 4 additions & 1 deletion src/util_libc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,10 @@ pub fn sys_fill_exact(
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)?,
res if res > 0 => {
let len = usize::try_from(res).map_err(|_| Error::UNEXPECTED)?;
buf = buf.get_mut(len..).ok_or(Error::UNEXPECTED)?;
}
-1 => {
let err = last_os_error();
// We should try again if the call was interrupted.
Expand Down
7 changes: 6 additions & 1 deletion src/vxworks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,12 @@ pub fn getrandom_inner(dest: &mut [MaybeUninit<u8>]) -> Result<(), Error> {
// Prevent overflow of i32
let chunk_size = usize::try_from(i32::MAX).expect("VxWorks does not support 16-bit targets");
for chunk in dest.chunks_mut(chunk_size) {
let ret = unsafe { libc::randABytes(chunk.as_mut_ptr().cast::<u8>(), chunk.len() as i32) };
let chunk_len: libc::c_int = chunk
.len()
.try_into()
.expect("chunk size is bounded by i32::MAX");
let p: *mut libc::c_uchar = chunk.as_mut_ptr().cast();
let ret = unsafe { libc::randABytes(p, chunk_len) };
if ret != 0 {
return Err(last_os_error());
}
Expand Down
8 changes: 7 additions & 1 deletion src/wasi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,16 @@ pub fn getrandom_inner(dest: &mut [MaybeUninit<u8>]) -> Result<(), Error> {
// https://docs.rs/wasi/0.11.0+wasi-snapshot-preview1/src/wasi/lib_generated.rs.html#2046-2062
// Note that size of an allocated object can not be bigger than isize::MAX bytes.
// WASI 0.1 supports only 32-bit WASM, so casting length to `i32` is safe.
#[allow(clippy::cast_possible_truncation, clippy::cast_possible_wrap)]
Copy link
Member Author

@newpavlov newpavlov Oct 9, 2024

Choose a reason for hiding this comment

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

This is the only place for which we have to disable the lints. Unfortunately, the compiler does not use the requirement that non-ZST allocations can not be bigger than isize::MAX during optimizations, so using i32::try_from(dest.len()).unwrap() will contain a panic. We could return Error::UNEXPECTED if dest.len() > i32::MAX, but I don't like such hack.

Also, IIRC WASI executors interprets i32 length in random_get as an unsigned integer, so we could've legitimately used cast_signed here, but unfortunately it's not stable.

let ret = unsafe { random_get(dest.as_mut_ptr() as i32, dest.len() as i32) };
match ret {
0 => Ok(()),
_ => Err(Error::from_os_error(ret as u32)),
code => {
let err = u32::try_from(code)
.map(Error::from_os_error)
.unwrap_or(Error::UNEXPECTED);
Err(err)
}
}
}

Expand Down
3 changes: 2 additions & 1 deletion src/windows7.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ 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) };
let chunk_len = u32::try_from(chunk.len()).expect("chunk size is bounded by i32::MAX");
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