From 86618e9bad1af25916b4ca5b20f2109f0be83bf3 Mon Sep 17 00:00:00 2001 From: Luca Casonato Date: Sun, 26 Jun 2022 00:13:24 +0200 Subject: [PATCH] build: require safety comments on unsafe code (#13870) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bartek Iwańczuk Co-authored-by: Divy Srivastava --- cli/bench/http.rs | 10 ++- cli/deno_dir.rs | 8 +- cli/lsp/logging.rs | 6 +- cli/lsp/parent_process_checker.rs | 2 + cli/unix_util.rs | 2 + core/async_cancel.rs | 25 ++++++ core/async_cell.rs | 34 +++++++- core/bindings.rs | 4 + core/inspector.rs | 13 ++- core/modules.rs | 1 + core/ops.rs | 2 + core/ops_metrics.rs | 5 ++ core/resources.rs | 2 + core/runtime.rs | 5 ++ ext/ffi/lib.rs | 128 +++++++++++++++++++----------- ext/net/ops_tls.rs | 20 ++++- ext/webgpu/src/buffer.rs | 6 ++ runtime/ops/io.rs | 33 +++++--- runtime/ops/os.rs | 12 ++- runtime/ops/process.rs | 2 + runtime/ops/spawn.rs | 2 + runtime/ops/tty.rs | 6 ++ runtime/permissions.rs | 2 + serde_v8/examples/basic.rs | 2 + serde_v8/magic/rawbytes.rs | 2 + serde_v8/serializable.rs | 2 + serde_v8/utils.rs | 1 + test_ffi/src/lib.rs | 40 +++++++--- tools/lint.js | 4 + 29 files changed, 294 insertions(+), 87 deletions(-) diff --git a/cli/bench/http.rs b/cli/bench/http.rs index 83f8eef8b0..7706429071 100644 --- a/cli/bench/http.rs +++ b/cli/bench/http.rs @@ -153,11 +153,13 @@ fn run( fn get_port() -> u16 { static mut NEXT_PORT: u16 = 4544; - let port = unsafe { NEXT_PORT }; - - unsafe { + // TODO(bartlomieju): + #[allow(clippy::undocumented_unsafe_blocks)] + let port = unsafe { + let p = NEXT_PORT; NEXT_PORT += 1; - } + p + }; port } diff --git a/cli/deno_dir.rs b/cli/deno_dir.rs index ea46474d06..c6a5eca298 100644 --- a/cli/deno_dir.rs +++ b/cli/deno_dir.rs @@ -76,7 +76,13 @@ mod dirs { pub fn home_dir() -> Option { std::env::var_os("HOME") .and_then(|h| if h.is_empty() { None } else { Some(h) }) - .or_else(|| unsafe { fallback() }) + .or_else(|| { + // TODO(bartlomieju): + #[allow(clippy::undocumented_unsafe_blocks)] + unsafe { + fallback() + } + }) .map(PathBuf::from) } diff --git a/cli/lsp/logging.rs b/cli/lsp/logging.rs index b3fc06d896..88392dca22 100644 --- a/cli/lsp/logging.rs +++ b/cli/lsp/logging.rs @@ -21,7 +21,11 @@ pub fn set_lsp_log_level(level: log::Level) { pub fn lsp_log_level() -> log::Level { let level = LSP_LOG_LEVEL.load(Ordering::SeqCst); - unsafe { std::mem::transmute(level) } + // TODO(bartlomieju): + #[allow(clippy::undocumented_unsafe_blocks)] + unsafe { + std::mem::transmute(level) + } } /// Use this macro to do "info" logs in the lsp code. This allows diff --git a/cli/lsp/parent_process_checker.rs b/cli/lsp/parent_process_checker.rs index f4bf99d3e6..e4a359bd93 100644 --- a/cli/lsp/parent_process_checker.rs +++ b/cli/lsp/parent_process_checker.rs @@ -20,6 +20,8 @@ pub fn start(parent_process_id: u32) { #[cfg(unix)] fn is_process_active(process_id: u32) -> bool { + // TODO(bartlomieju): + #[allow(clippy::undocumented_unsafe_blocks)] unsafe { // signal of 0 checks for the existence of the process id libc::kill(process_id as i32, 0) == 0 diff --git a/cli/unix_util.rs b/cli/unix_util.rs index 927b99d64b..f282f6cfec 100644 --- a/cli/unix_util.rs +++ b/cli/unix_util.rs @@ -4,6 +4,8 @@ /// This is the difference between `ulimit -n` and `ulimit -n -H`. pub fn raise_fd_limit() { #[cfg(unix)] + // TODO(bartlomieju): + #[allow(clippy::undocumented_unsafe_blocks)] unsafe { let mut limits = libc::rlimit { rlim_cur: 0, diff --git a/core/async_cancel.rs b/core/async_cancel.rs index cf338174da..55ab8f4d14 100644 --- a/core/async_cancel.rs +++ b/core/async_cancel.rs @@ -302,13 +302,19 @@ mod internal { Some((head, rc)) => { // Register this `Cancelable` node with a `CancelHandle` head node. assert_ne!(self, head); + // TODO(piscisaureus): safety comment + #[allow(clippy::undocumented_unsafe_blocks)] let self_inner = unsafe { &mut *self.inner.get() }; + // TODO(piscisaureus): safety comment + #[allow(clippy::undocumented_unsafe_blocks)] let head_inner = unsafe { &mut *head.inner.get() }; self_inner.link(waker, head_inner, rc) } None => { // This `Cancelable` has already been linked to a `CancelHandle` head // node; just update our stored `Waker` if necessary. + // TODO(piscisaureus): safety comment + #[allow(clippy::undocumented_unsafe_blocks)] let inner = unsafe { &mut *self.inner.get() }; inner.update_waker(waker) } @@ -316,11 +322,15 @@ mod internal { } pub fn cancel(&self) { + // TODO(piscisaureus): safety comment + #[allow(clippy::undocumented_unsafe_blocks)] let inner = unsafe { &mut *self.inner.get() }; inner.cancel(); } pub fn is_canceled(&self) -> bool { + // TODO(piscisaureus): safety comment + #[allow(clippy::undocumented_unsafe_blocks)] let inner = unsafe { &mut *self.inner.get() }; inner.is_canceled() } @@ -337,6 +347,8 @@ mod internal { impl Drop for Node { fn drop(&mut self) { + // TODO(piscisaureus): safety comment + #[allow(clippy::undocumented_unsafe_blocks)] let inner = unsafe { &mut *self.inner.get() }; inner.unlink(); } @@ -392,6 +404,8 @@ mod internal { prev: next_prev_nn, .. } => { + // TODO(piscisaureus): safety comment + #[allow(clippy::undocumented_unsafe_blocks)] let prev = unsafe { &mut *next_prev_nn.as_ptr() }; match prev { NodeInner::Linked { @@ -444,10 +458,14 @@ mod internal { if prev_nn == next_nn { // There were only two nodes in this chain; after unlinking ourselves // the other node is no longer linked. + // TODO(piscisaureus): safety comment + #[allow(clippy::undocumented_unsafe_blocks)] let other = unsafe { prev_nn.as_mut() }; *other = NodeInner::Unlinked; } else { // The chain had more than two nodes. + // TODO(piscisaureus): safety comment + #[allow(clippy::undocumented_unsafe_blocks)] match unsafe { prev_nn.as_mut() } { NodeInner::Linked { next: prev_next_nn, .. @@ -456,6 +474,8 @@ mod internal { } _ => unreachable!(), } + // TODO(piscisaureus): safety comment + #[allow(clippy::undocumented_unsafe_blocks)] match unsafe { next_nn.as_mut() } { NodeInner::Linked { prev: next_prev_nn, .. @@ -473,6 +493,8 @@ mod internal { fn cancel(&mut self) { let mut head_nn = NonNull::from(self); + // TODO(piscisaureus): safety comment + #[allow(clippy::undocumented_unsafe_blocks)] // Mark the head node as canceled. let mut item_nn = match replace(unsafe { head_nn.as_mut() }, NodeInner::Canceled) { @@ -487,6 +509,8 @@ mod internal { // Cancel all item nodes in the chain, waking each stored `Waker`. while item_nn != head_nn { + // TODO(piscisaureus): safety comment + #[allow(clippy::undocumented_unsafe_blocks)] match replace(unsafe { item_nn.as_mut() }, NodeInner::Canceled) { NodeInner::Linked { kind: NodeKind::Item { waker }, @@ -745,6 +769,7 @@ mod tests { assert!(Rc::get_mut(&mut cancel_handle).is_some()); let mut future = pending::().or_cancel(&cancel_handle); + // SAFETY: `Cancelable` pins the future let future = unsafe { Pin::new_unchecked(&mut future) }; // There are two `Rc` references now, so this fails. diff --git a/core/async_cell.rs b/core/async_cell.rs index 8798861cf3..ea6cac831d 100644 --- a/core/async_cell.rs +++ b/core/async_cell.rs @@ -157,12 +157,16 @@ impl RcRef { map_fn: F, ) -> RcRef { let RcRef:: { rc, value } = source.into(); + // TODO(piscisaureus): safety comment + #[allow(clippy::undocumented_unsafe_blocks)] let value = map_fn(unsafe { &*value }); RcRef { rc, value } } pub(crate) fn split(rc_ref: &Self) -> (&T, &Rc) { let &Self { ref rc, value } = rc_ref; + // TODO(piscisaureus): safety comment + #[allow(clippy::undocumented_unsafe_blocks)] (unsafe { &*value }, rc) } } @@ -206,7 +210,11 @@ impl From<&Rc> for RcRef { impl Deref for RcRef { type Target = T; fn deref(&self) -> &Self::Target { - unsafe { &*self.value } + // TODO(piscisaureus): safety comment + #[allow(clippy::undocumented_unsafe_blocks)] + unsafe { + &*self.value + } } } @@ -260,6 +268,8 @@ mod internal { // Don't allow synchronous borrows to cut in line; if there are any // enqueued waiters, return `None`, even if the current borrow is a shared // one and the requested borrow is too. + // TODO(piscisaureus): safety comment + #[allow(clippy::undocumented_unsafe_blocks)] let waiters = unsafe { &mut *cell_ref.waiters.as_ptr() }; if waiters.is_empty() { // There are no enqueued waiters, but it is still possible that the cell @@ -288,6 +298,8 @@ mod internal { let waiter = Waiter::new(M::borrow_mode()); let turn = self.turn.get(); let index = { + // TODO(piscisaureus): safety comment + #[allow(clippy::undocumented_unsafe_blocks)] let waiters = unsafe { &mut *self.waiters.as_ptr() }; waiters.push_back(Some(waiter)); waiters.len() - 1 @@ -315,6 +327,8 @@ mod internal { Poll::Ready(()) } else { // This waiter is still in line and has not yet been woken. + // TODO(piscisaureus): safety comment + #[allow(clippy::undocumented_unsafe_blocks)] let waiters = unsafe { &mut *self.waiters.as_ptr() }; // Sanity check: id cannot be higher than the last queue element. assert!(id < turn + waiters.len()); @@ -330,6 +344,8 @@ mod internal { fn wake_waiters(&self) { let mut borrow_count = self.borrow_count.get(); + // TODO(piscisaureus): safety comment + #[allow(clippy::undocumented_unsafe_blocks)] let waiters = unsafe { &mut *self.waiters.as_ptr() }; let mut turn = self.turn.get(); @@ -379,6 +395,8 @@ mod internal { self.drop_borrow::(); } else { // This waiter is still in the queue, take it out and leave a "hole". + // TODO(piscisaureus): safety comment + #[allow(clippy::undocumented_unsafe_blocks)] let waiters = unsafe { &mut *self.waiters.as_ptr() }; waiters[id - turn].take().unwrap(); } @@ -411,6 +429,8 @@ mod internal { type Output = AsyncBorrowImpl; fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll { ready!(self.cell.as_ref().unwrap().poll_waiter::(self.id, cx)); + // TODO(piscisaureus): safety comment + #[allow(clippy::undocumented_unsafe_blocks)] let self_mut = unsafe { Pin::get_unchecked_mut(self) }; let cell = self_mut.cell.take().unwrap(); Poll::Ready(AsyncBorrowImpl::::new(cell)) @@ -448,7 +468,11 @@ mod internal { impl Deref for AsyncBorrowImpl { type Target = T; fn deref(&self) -> &Self::Target { - unsafe { &*self.cell.as_ptr() } + // TODO(piscisaureus): safety comment + #[allow(clippy::undocumented_unsafe_blocks)] + unsafe { + &*self.cell.as_ptr() + } } } @@ -466,7 +490,11 @@ mod internal { impl DerefMut for AsyncBorrowImpl { fn deref_mut(&mut self) -> &mut Self::Target { - unsafe { &mut *self.cell.as_ptr() } + // TODO(piscisaureus): safety comment + #[allow(clippy::undocumented_unsafe_blocks)] + unsafe { + &mut *self.cell.as_ptr() + } } } diff --git a/core/bindings.rs b/core/bindings.rs index 5681ded31c..a88e54af70 100644 --- a/core/bindings.rs +++ b/core/bindings.rs @@ -147,6 +147,7 @@ pub extern "C" fn host_import_module_dynamically_callback( specifier: v8::Local, import_assertions: v8::Local, ) -> *mut v8::Promise { + // SAFETY: `CallbackScope` can be safely constructed from `Local` let scope = &mut unsafe { v8::CallbackScope::new(context) }; // NOTE(bartlomieju): will crash for non-UTF-8 specifier @@ -253,6 +254,7 @@ pub extern "C" fn host_initialize_import_meta_object_callback( module: v8::Local, meta: v8::Local, ) { + // SAFETY: `CallbackScope` can be safely constructed from `Local` let scope = &mut unsafe { v8::CallbackScope::new(context) }; let module_map_rc = JsRuntime::module_map(scope); let module_map = module_map_rc.borrow(); @@ -274,6 +276,7 @@ pub extern "C" fn host_initialize_import_meta_object_callback( pub extern "C" fn promise_reject_callback(message: v8::PromiseRejectMessage) { use v8::PromiseRejectEvent::*; + // SAFETY: `CallbackScope` can be safely constructed from `&PromiseRejectMessage` let scope = &mut unsafe { v8::CallbackScope::new(&message) }; let state_rc = JsRuntime::state(scope); @@ -418,6 +421,7 @@ pub fn module_resolve_callback<'s>( import_assertions: v8::Local<'s, v8::FixedArray>, referrer: v8::Local<'s, v8::Module>, ) -> Option> { + // SAFETY: `CallbackScope` can be safely constructed from `Local` let scope = &mut unsafe { v8::CallbackScope::new(context) }; let module_map_rc = JsRuntime::module_map(scope); diff --git a/core/inspector.rs b/core/inspector.rs index 26855acdfe..bec22d2573 100644 --- a/core/inspector.rs +++ b/core/inspector.rs @@ -485,6 +485,8 @@ impl task::ArcWake for InspectorWaker { _isolate: &mut v8::Isolate, arg: *mut c_void, ) { + // SAFETY: `InspectorWaker` is owned by `JsRuntimeInspector`, so the + // pointer to the latter is valid as long as waker is alive. let inspector = unsafe { &*(arg as *mut JsRuntimeInspector) }; let _ = inspector.poll_sessions(None); } @@ -521,6 +523,8 @@ impl InspectorSession { let v8_channel = v8::inspector::ChannelBase::new::(); let mut v8_inspector = v8_inspector_rc.borrow_mut(); let v8_inspector_ptr = v8_inspector.as_mut().unwrap(); + // TODO(piscisaureus): safety comment + #[allow(clippy::undocumented_unsafe_blocks)] let v8_session = v8_inspector_ptr.connect( Self::CONTEXT_GROUP_ID, // Todo(piscisaureus): V8Inspector::connect() should require that @@ -544,6 +548,8 @@ impl InspectorSession { msg: String, ) { let msg = v8::inspector::StringView::from(msg.as_bytes()); + // SAFETY: `InspectorSession` is the only owner of `v8_session_ptr`, so + // the pointer is valid for as long the struct. unsafe { (*v8_session_ptr).dispatch_protocol_message(msg); }; @@ -731,6 +737,9 @@ impl LocalInspectorSession { fn new_box_with(new_fn: impl FnOnce(*mut T) -> T) -> Box { let b = Box::new(MaybeUninit::::uninit()); let p = Box::into_raw(b) as *mut T; - unsafe { ptr::write(p, new_fn(p)) }; - unsafe { Box::from_raw(p) } + // SAFETY: memory layout for `T` is ensured on first line of this function + unsafe { + ptr::write(p, new_fn(p)); + Box::from_raw(p) + } } diff --git a/core/modules.rs b/core/modules.rs index e86fdbf73a..aec8c498ff 100644 --- a/core/modules.rs +++ b/core/modules.rs @@ -122,6 +122,7 @@ fn json_module_evaluation_steps<'a>( context: v8::Local<'a, v8::Context>, module: v8::Local, ) -> Option> { + // SAFETY: `CallbackScope` can be safely constructed from `Local` let scope = &mut unsafe { v8::CallbackScope::new(context) }; let tc_scope = &mut v8::TryCatch::new(scope); let module_map = tc_scope diff --git a/core/ops.rs b/core/ops.rs index a2833e8112..3d0d0ec706 100644 --- a/core/ops.rs +++ b/core/ops.rs @@ -63,6 +63,8 @@ impl Future for OpCall { self: std::pin::Pin<&mut Self>, cx: &mut std::task::Context<'_>, ) -> std::task::Poll { + // TODO(piscisaureus): safety comment + #[allow(clippy::undocumented_unsafe_blocks)] let inner = unsafe { &mut self.get_unchecked_mut().0 }; let mut pinned = Pin::new(inner); ready!(pinned.as_mut().poll(cx)); diff --git a/core/ops_metrics.rs b/core/ops_metrics.rs index b068aa0eec..aa3ff503b7 100644 --- a/core/ops_metrics.rs +++ b/core/ops_metrics.rs @@ -56,12 +56,17 @@ impl OpsTracker { #[allow(clippy::mut_from_ref)] #[inline] fn ops_mut(&self) -> &mut Vec { + // SAFETY: `OpsTracker` is created after registering ops so it is guaranteed + // that that `ops` will be initialized. unsafe { &mut *self.ops.get() } } #[allow(clippy::mut_from_ref)] #[inline] fn metrics_mut(&self, id: OpId) -> &mut OpMetrics { + // SAFETY: `OpsTracker` is created after registering ops, and ops + // cannot be unregistered during runtime, so it is guaranteed that `id` + // is not causing out-of-bound access. unsafe { self.ops_mut().get_unchecked_mut(id) } } diff --git a/core/resources.rs b/core/resources.rs index ae4ef73944..a4bb3607df 100644 --- a/core/resources.rs +++ b/core/resources.rs @@ -77,6 +77,8 @@ impl dyn Resource { pub fn downcast_rc<'a, T: Resource>(self: &'a Rc) -> Option<&'a Rc> { if self.is::() { let ptr = self as *const Rc<_> as *const Rc; + // TODO(piscisaureus): safety comment + #[allow(clippy::undocumented_unsafe_blocks)] Some(unsafe { &*ptr }) } else { None diff --git a/core/runtime.rs b/core/runtime.rs index f82f062070..7cb556fd3f 100644 --- a/core/runtime.rs +++ b/core/runtime.rs @@ -333,6 +333,9 @@ impl JsRuntime { assert!(options.startup_snapshot.is_none()); let mut creator = v8::SnapshotCreator::new(Some(&bindings::EXTERNAL_REFERENCES)); + // SAFETY: `get_owned_isolate` is unsafe because it may only be called + // once. This is the only place we call this function, so this call is + // safe. let isolate = unsafe { creator.get_owned_isolate() }; let mut isolate = JsRuntime::setup_isolate(isolate); { @@ -1028,6 +1031,8 @@ extern "C" fn near_heap_limit_callback( where F: FnMut(usize, usize) -> usize, { + // SAFETY: The data is a pointer to the Rust callback function. It is stored + // in `JsRuntime::allocations` and thus is guaranteed to outlive the isolate. let callback = unsafe { &mut *(data as *mut F) }; callback(current_heap_limit, initial_heap_limit) } diff --git a/ext/ffi/lib.rs b/ext/ffi/lib.rs index 4c306e7f4d..41b2f0a3b6 100644 --- a/ext/ffi/lib.rs +++ b/ext/ffi/lib.rs @@ -130,6 +130,8 @@ impl DynamicLibraryResource { // By default, Err returned by this function does not tell // which symbol wasn't exported. So we'll modify the error // message to include the name of symbol. + // + // SAFETY: The obtained T symbol is the size of a pointer. let fn_ptr = match unsafe { self.lib.symbol::<*const c_void>(symbol) } { Ok(value) => Ok(value), Err(err) => Err(generic_error(format!( @@ -164,6 +166,8 @@ impl DynamicLibraryResource { // By default, Err returned by this function does not tell // which symbol wasn't exported. So we'll modify the error // message to include the name of symbol. + // + // SAFETY: The obtained T symbol is the size of a pointer. match unsafe { self.lib.symbol::<*const c_void>(&symbol) } { Ok(value) => Ok(Ok(value)), Err(err) => Err(generic_error(format!( @@ -743,55 +747,60 @@ fn ffi_call( let call_args: Vec = call_args .iter() .enumerate() - .map(|(index, ffi_arg)| unsafe { - ffi_arg.as_arg(*parameter_types.get(index).unwrap()) + .map(|(index, ffi_arg)| { + // SAFETY: the union field is initialized + unsafe { ffi_arg.as_arg(*parameter_types.get(index).unwrap()) } }) .collect(); - Ok(match result_type { - NativeType::Void => NativeValue { - void_value: unsafe { cif.call::<()>(fun_ptr, &call_args) }, - }, - NativeType::U8 => NativeValue { - u8_value: unsafe { cif.call::(fun_ptr, &call_args) }, - }, - NativeType::I8 => NativeValue { - i8_value: unsafe { cif.call::(fun_ptr, &call_args) }, - }, - NativeType::U16 => NativeValue { - u16_value: unsafe { cif.call::(fun_ptr, &call_args) }, - }, - NativeType::I16 => NativeValue { - i16_value: unsafe { cif.call::(fun_ptr, &call_args) }, - }, - NativeType::U32 => NativeValue { - u32_value: unsafe { cif.call::(fun_ptr, &call_args) }, - }, - NativeType::I32 => NativeValue { - i32_value: unsafe { cif.call::(fun_ptr, &call_args) }, - }, - NativeType::U64 => NativeValue { - u64_value: unsafe { cif.call::(fun_ptr, &call_args) }, - }, - NativeType::I64 => NativeValue { - i64_value: unsafe { cif.call::(fun_ptr, &call_args) }, - }, - NativeType::USize => NativeValue { - usize_value: unsafe { cif.call::(fun_ptr, &call_args) }, - }, - NativeType::ISize => NativeValue { - isize_value: unsafe { cif.call::(fun_ptr, &call_args) }, - }, - NativeType::F32 => NativeValue { - f32_value: unsafe { cif.call::(fun_ptr, &call_args) }, - }, - NativeType::F64 => NativeValue { - f64_value: unsafe { cif.call::(fun_ptr, &call_args) }, - }, - NativeType::Pointer | NativeType::Function => NativeValue { - pointer: unsafe { cif.call::<*const u8>(fun_ptr, &call_args) }, - }, - }) + // SAFETY: types in the `Cif` match the actual calling convention and + // types of symbol. + unsafe { + Ok(match result_type { + NativeType::Void => NativeValue { + void_value: cif.call::<()>(fun_ptr, &call_args), + }, + NativeType::U8 => NativeValue { + u8_value: cif.call::(fun_ptr, &call_args), + }, + NativeType::I8 => NativeValue { + i8_value: cif.call::(fun_ptr, &call_args), + }, + NativeType::U16 => NativeValue { + u16_value: cif.call::(fun_ptr, &call_args), + }, + NativeType::I16 => NativeValue { + i16_value: cif.call::(fun_ptr, &call_args), + }, + NativeType::U32 => NativeValue { + u32_value: cif.call::(fun_ptr, &call_args), + }, + NativeType::I32 => NativeValue { + i32_value: cif.call::(fun_ptr, &call_args), + }, + NativeType::U64 => NativeValue { + u64_value: cif.call::(fun_ptr, &call_args), + }, + NativeType::I64 => NativeValue { + i64_value: cif.call::(fun_ptr, &call_args), + }, + NativeType::USize => NativeValue { + usize_value: cif.call::(fun_ptr, &call_args), + }, + NativeType::ISize => NativeValue { + isize_value: cif.call::(fun_ptr, &call_args), + }, + NativeType::F32 => NativeValue { + f32_value: cif.call::(fun_ptr, &call_args), + }, + NativeType::F64 => NativeValue { + f64_value: cif.call::(fun_ptr, &call_args), + }, + NativeType::Pointer | NativeType::Function => NativeValue { + pointer: cif.call::<*const u8>(fun_ptr, &call_args), + }, + }) + } } struct UnsafeCallbackResource { @@ -1201,71 +1210,83 @@ fn op_ffi_get_static<'scope>( return Err(type_error("Invalid FFI static type 'void'")); } NativeType::U8 => { + // SAFETY: ptr is user provided let result = unsafe { ptr::read_unaligned(data_ptr as *const u8) }; let number: v8::Local = v8::Integer::new_from_unsigned(scope, result as u32).into(); number.into() } NativeType::I8 => { + // SAFETY: ptr is user provided let result = unsafe { ptr::read_unaligned(data_ptr as *const i8) }; let number: v8::Local = v8::Integer::new(scope, result as i32).into(); number.into() } NativeType::U16 => { + // SAFETY: ptr is user provided let result = unsafe { ptr::read_unaligned(data_ptr as *const u16) }; let number: v8::Local = v8::Integer::new_from_unsigned(scope, result as u32).into(); number.into() } NativeType::I16 => { + // SAFETY: ptr is user provided let result = unsafe { ptr::read_unaligned(data_ptr as *const i16) }; let number: v8::Local = v8::Integer::new(scope, result as i32).into(); number.into() } NativeType::U32 => { + // SAFETY: ptr is user provided let result = unsafe { ptr::read_unaligned(data_ptr as *const u32) }; let number: v8::Local = v8::Integer::new_from_unsigned(scope, result).into(); number.into() } NativeType::I32 => { + // SAFETY: ptr is user provided let result = unsafe { ptr::read_unaligned(data_ptr as *const i32) }; let number: v8::Local = v8::Integer::new(scope, result).into(); number.into() } NativeType::U64 => { + // SAFETY: ptr is user provided let result = unsafe { ptr::read_unaligned(data_ptr as *const u64) }; let big_int: v8::Local = v8::BigInt::new_from_u64(scope, result).into(); big_int.into() } NativeType::I64 => { + // SAFETY: ptr is user provided let result = unsafe { ptr::read_unaligned(data_ptr as *const i64) }; let big_int: v8::Local = v8::BigInt::new_from_i64(scope, result).into(); big_int.into() } NativeType::USize => { + // SAFETY: ptr is user provided let result = unsafe { ptr::read_unaligned(data_ptr as *const usize) }; let big_int: v8::Local = v8::BigInt::new_from_u64(scope, result as u64).into(); big_int.into() } NativeType::ISize => { + // SAFETY: ptr is user provided let result = unsafe { ptr::read_unaligned(data_ptr as *const isize) }; let big_int: v8::Local = v8::BigInt::new_from_i64(scope, result as i64).into(); big_int.into() } NativeType::F32 => { + // SAFETY: ptr is user provided let result = unsafe { ptr::read_unaligned(data_ptr as *const f32) }; let number: v8::Local = v8::Number::new(scope, result as f64).into(); number.into() } NativeType::F64 => { + // SAFETY: ptr is user provided let result = unsafe { ptr::read_unaligned(data_ptr as *const f64) }; let number: v8::Local = v8::Number::new(scope, result).into(); number.into() @@ -1339,7 +1360,7 @@ fn op_ffi_call_nonblocking<'scope>( cif, ptr, parameter_types, - .. + result_type, } = symbol.clone(); ffi_call(call_args, &cif, ptr, ¶meter_types, result_type) }); @@ -1392,6 +1413,8 @@ where )) } else { let src = src as *const u8; + // SAFETY: src is user defined. + // dest is properly aligned and is valid for writes of len * size_of::() bytes. unsafe { ptr::copy(src, dst.as_mut_ptr(), len) }; Ok(()) } @@ -1411,6 +1434,8 @@ where permissions.check(None)?; let ptr = ptr as *const c_char; + // SAFETY: ptr is user provided + // lifetime validity is not an issue because we allocate a new string. Ok(unsafe { CStr::from_ptr(ptr) }.to_str()?.to_string()) } @@ -1427,6 +1452,7 @@ where let permissions = state.borrow_mut::(); permissions.check(None)?; + // SAFETY: ptr is user provided. Ok(unsafe { ptr::read_unaligned(ptr as *const u8) }) } @@ -1443,6 +1469,7 @@ where let permissions = state.borrow_mut::(); permissions.check(None)?; + // SAFETY: ptr is user provided. Ok(unsafe { ptr::read_unaligned(ptr as *const i8) }) } @@ -1459,6 +1486,7 @@ where let permissions = state.borrow_mut::(); permissions.check(None)?; + // SAFETY: ptr is user provided. Ok(unsafe { ptr::read_unaligned(ptr as *const u16) }) } @@ -1475,6 +1503,7 @@ where let permissions = state.borrow_mut::(); permissions.check(None)?; + // SAFETY: ptr is user provided. Ok(unsafe { ptr::read_unaligned(ptr as *const i16) }) } @@ -1491,6 +1520,7 @@ where let permissions = state.borrow_mut::(); permissions.check(None)?; + // SAFETY: ptr is user provided. Ok(unsafe { ptr::read_unaligned(ptr as *const u32) }) } @@ -1507,6 +1537,7 @@ where let permissions = state.borrow_mut::(); permissions.check(None)?; + // SAFETY: ptr is user provided. Ok(unsafe { ptr::read_unaligned(ptr as *const i32) }) } @@ -1525,6 +1556,7 @@ where let permissions = state.borrow_mut::(); permissions.check(None)?; + // SAFETY: ptr is user provided. let result = unsafe { ptr::read_unaligned(ptr as *const u64) }; let big_int: v8::Local = @@ -1545,6 +1577,7 @@ where let permissions = state.borrow_mut::(); permissions.check(None)?; + // SAFETY: ptr is user provided. Ok(unsafe { ptr::read_unaligned(ptr as *const f32) }) } @@ -1561,6 +1594,7 @@ where let permissions = state.borrow_mut::(); permissions.check(None)?; + // SAFETY: ptr is user provided. Ok(unsafe { ptr::read_unaligned(ptr as *const f64) }) } diff --git a/ext/net/ops_tls.rs b/ext/net/ops_tls.rs index 8892ea0ac2..1c91674df0 100644 --- a/ext/net/ops_tls.rs +++ b/ext/net/ops_tls.rs @@ -375,11 +375,17 @@ impl TlsStreamInner { ready!(self.poll_io(cx, Flow::Read))?; if self.rd_state == State::StreamOpen { + // TODO(bartlomieju): + #[allow(clippy::undocumented_unsafe_blocks)] let buf_slice = unsafe { &mut *(buf.unfilled_mut() as *mut [_] as *mut [u8]) }; let bytes_read = self.tls.reader().read(buf_slice)?; assert_ne!(bytes_read, 0); - unsafe { buf.assume_init(bytes_read) }; + // TODO(bartlomieju): + #[allow(clippy::undocumented_unsafe_blocks)] + unsafe { + buf.assume_init(bytes_read) + }; buf.advance(bytes_read); } @@ -581,10 +587,16 @@ impl Shared { let self_weak = Arc::downgrade(self); let self_ptr = self_weak.into_raw() as *const (); let raw_waker = RawWaker::new(self_ptr, &Self::SHARED_WAKER_VTABLE); - unsafe { Waker::from_raw(raw_waker) } + // TODO(bartlomieju): + #[allow(clippy::undocumented_unsafe_blocks)] + unsafe { + Waker::from_raw(raw_waker) + } } fn clone_shared_waker(self_ptr: *const ()) -> RawWaker { + // TODO(bartlomieju): + #[allow(clippy::undocumented_unsafe_blocks)] let self_weak = unsafe { Weak::from_raw(self_ptr as *const Self) }; let ptr1 = self_weak.clone().into_raw(); let ptr2 = self_weak.into_raw(); @@ -598,6 +610,8 @@ impl Shared { } fn wake_shared_waker_by_ref(self_ptr: *const ()) { + // TODO(bartlomieju): + #[allow(clippy::undocumented_unsafe_blocks)] let self_weak = unsafe { Weak::from_raw(self_ptr as *const Self) }; if let Some(self_arc) = Weak::upgrade(&self_weak) { self_arc.rd_waker.wake(); @@ -607,6 +621,8 @@ impl Shared { } fn drop_shared_waker(self_ptr: *const ()) { + // TODO(bartlomieju): + #[allow(clippy::undocumented_unsafe_blocks)] let _ = unsafe { Weak::from_raw(self_ptr as *const Self) }; } diff --git a/ext/webgpu/src/buffer.rs b/ext/webgpu/src/buffer.rs index 24bd98efb9..f8bc213a30 100644 --- a/ext/webgpu/src/buffer.rs +++ b/ext/webgpu/src/buffer.rs @@ -105,6 +105,8 @@ pub async fn op_webgpu_buffer_get_map_async( user_data: *mut u8, ) { let sender_ptr = user_data as *mut oneshot::Sender>; + // TODO(bartlomieju): + #[allow(clippy::undocumented_unsafe_blocks)] let boxed_sender = unsafe { Box::from_raw(sender_ptr) }; boxed_sender .send(match status { @@ -188,6 +190,8 @@ pub fn op_webgpu_buffer_get_mapped_range( )) .map_err(|e| DomExceptionOperationError::new(&e.to_string()))?; + // TODO(bartlomieju): + #[allow(clippy::undocumented_unsafe_blocks)] let slice = unsafe { std::slice::from_raw_parts_mut(slice_pointer, range_size as usize) }; @@ -225,6 +229,8 @@ pub fn op_webgpu_buffer_unmap( let size = mapped_resource.1; if let Some(buffer) = zero_copy { + // TODO(bartlomieju): + #[allow(clippy::undocumented_unsafe_blocks)] let slice = unsafe { std::slice::from_raw_parts_mut(slice_pointer, size) }; slice.copy_from_slice(&buffer); } diff --git a/runtime/ops/io.rs b/runtime/ops/io.rs index 78263ce281..bef33e9fad 100644 --- a/runtime/ops/io.rs +++ b/runtime/ops/io.rs @@ -42,26 +42,35 @@ use { // alive for the duration of the application since the last handle/fd // being dropped will close the corresponding pipe. #[cfg(unix)] -static STDIN_HANDLE: Lazy = - Lazy::new(|| unsafe { StdFile::from_raw_fd(0) }); +static STDIN_HANDLE: Lazy = Lazy::new(|| { + // SAFETY: corresponds to OS stdin + unsafe { StdFile::from_raw_fd(0) } +}); #[cfg(unix)] -static STDOUT_HANDLE: Lazy = - Lazy::new(|| unsafe { StdFile::from_raw_fd(1) }); +static STDOUT_HANDLE: Lazy = Lazy::new(|| { + // SAFETY: corresponds to OS stdout + unsafe { StdFile::from_raw_fd(1) } +}); #[cfg(unix)] -static STDERR_HANDLE: Lazy = - Lazy::new(|| unsafe { StdFile::from_raw_fd(2) }); +static STDERR_HANDLE: Lazy = Lazy::new(|| { + // SAFETY: corresponds to OS stderr + unsafe { StdFile::from_raw_fd(2) } +}); #[cfg(windows)] -static STDIN_HANDLE: Lazy = Lazy::new(|| unsafe { - StdFile::from_raw_handle(GetStdHandle(winbase::STD_INPUT_HANDLE)) +static STDIN_HANDLE: Lazy = Lazy::new(|| { + // SAFETY: corresponds to OS stdin + unsafe { StdFile::from_raw_handle(GetStdHandle(winbase::STD_INPUT_HANDLE)) } }); #[cfg(windows)] -static STDOUT_HANDLE: Lazy = Lazy::new(|| unsafe { - StdFile::from_raw_handle(GetStdHandle(winbase::STD_OUTPUT_HANDLE)) +static STDOUT_HANDLE: Lazy = Lazy::new(|| { + // SAFETY: corresponds to OS stdout + unsafe { StdFile::from_raw_handle(GetStdHandle(winbase::STD_OUTPUT_HANDLE)) } }); #[cfg(windows)] -static STDERR_HANDLE: Lazy = Lazy::new(|| unsafe { - StdFile::from_raw_handle(GetStdHandle(winbase::STD_ERROR_HANDLE)) +static STDERR_HANDLE: Lazy = Lazy::new(|| { + // SAFETY: corresponds to OS stderr + unsafe { StdFile::from_raw_handle(GetStdHandle(winbase::STD_ERROR_HANDLE)) } }); pub fn init() -> Extension { diff --git a/runtime/ops/os.rs b/runtime/ops/os.rs index 654bbede16..dbc87daab3 100644 --- a/runtime/ops/os.rs +++ b/runtime/ops/os.rs @@ -248,7 +248,11 @@ fn op_system_memory_info( fn op_getgid(state: &mut OpState) -> Result, AnyError> { super::check_unstable(state, "Deno.getGid"); state.borrow_mut::().env.check_all()?; - unsafe { Ok(Some(libc::getgid())) } + // TODO(bartlomieju): + #[allow(clippy::undocumented_unsafe_blocks)] + unsafe { + Ok(Some(libc::getgid())) + } } #[cfg(windows)] @@ -264,7 +268,11 @@ fn op_getgid(state: &mut OpState) -> Result, AnyError> { fn op_getuid(state: &mut OpState) -> Result, AnyError> { super::check_unstable(state, "Deno.getUid"); state.borrow_mut::().env.check_all()?; - unsafe { Ok(Some(libc::getuid())) } + // TODO(bartlomieju): + #[allow(clippy::undocumented_unsafe_blocks)] + unsafe { + Ok(Some(libc::getuid())) + } } #[cfg(windows)] diff --git a/runtime/ops/process.rs b/runtime/ops/process.rs index 49f26ade46..a48cd122d3 100644 --- a/runtime/ops/process.rs +++ b/runtime/ops/process.rs @@ -174,6 +174,8 @@ fn op_run(state: &mut OpState, run_args: RunArgs) -> Result { c.uid(uid); } #[cfg(unix)] + // TODO(bartlomieju): + #[allow(clippy::undocumented_unsafe_blocks)] unsafe { c.pre_exec(|| { libc::setgroups(0, std::ptr::null()); diff --git a/runtime/ops/spawn.rs b/runtime/ops/spawn.rs index 7e7e2d05ef..a6930b485e 100644 --- a/runtime/ops/spawn.rs +++ b/runtime/ops/spawn.rs @@ -149,6 +149,8 @@ fn create_command( command.uid(uid); } #[cfg(unix)] + // TODO(bartlomieju): + #[allow(clippy::undocumented_unsafe_blocks)] unsafe { command.pre_exec(|| { libc::setgroups(0, std::ptr::null()); diff --git a/runtime/ops/tty.rs b/runtime/ops/tty.rs index ad152e2dab..62a7717a62 100644 --- a/runtime/ops/tty.rs +++ b/runtime/ops/tty.rs @@ -172,12 +172,16 @@ fn op_isatty(state: &mut OpState, rid: ResourceId) -> Result { let handle = get_windows_handle(std_file)?; let mut test_mode: DWORD = 0; // If I cannot get mode out of console, it is not a console. + // TODO(bartlomieju): + #[allow(clippy::undocumented_unsafe_blocks)] Ok(unsafe { consoleapi::GetConsoleMode(handle, &mut test_mode) != FALSE }) } #[cfg(unix)] { use std::os::unix::io::AsRawFd; let raw_fd = std_file.as_raw_fd(); + // TODO(bartlomieju): + #[allow(clippy::undocumented_unsafe_blocks)] Ok(unsafe { libc::isatty(raw_fd as libc::c_int) == 1 }) } })?; @@ -225,6 +229,8 @@ fn op_console_size( use std::os::unix::io::AsRawFd; let fd = std_file.as_raw_fd(); + // TODO(bartlomieju): + #[allow(clippy::undocumented_unsafe_blocks)] unsafe { let mut size: libc::winsize = std::mem::zeroed(); if libc::ioctl(fd, libc::TIOCGWINSZ, &mut size as *mut _) != 0 { diff --git a/runtime/permissions.rs b/runtime/permissions.rs index be6c0c188d..ab8baec893 100644 --- a/runtime/permissions.rs +++ b/runtime/permissions.rs @@ -1886,6 +1886,8 @@ fn permission_prompt(message: &str, name: &str) -> bool { #[cfg(unix)] fn clear_stdin() -> Result<(), AnyError> { + // TODO(bartlomieju): + #[allow(clippy::undocumented_unsafe_blocks)] let r = unsafe { libc::tcflush(0, libc::TCIFLUSH) }; assert_eq!(r, 0); Ok(()) diff --git a/serde_v8/examples/basic.rs b/serde_v8/examples/basic.rs index 81a8e6e6ca..8f19e9405c 100644 --- a/serde_v8/examples/basic.rs +++ b/serde_v8/examples/basic.rs @@ -52,6 +52,8 @@ fn main() { println!("x = {}", x); } + // SAFETY: all isolates have been destroyed, so we can now safely let V8 clean + // up its resources. unsafe { v8::V8::dispose(); } diff --git a/serde_v8/magic/rawbytes.rs b/serde_v8/magic/rawbytes.rs index d82e687178..2189ebfc33 100644 --- a/serde_v8/magic/rawbytes.rs +++ b/serde_v8/magic/rawbytes.rs @@ -87,8 +87,10 @@ mod tests { #[test] fn bytes_layout() { + // SAFETY: ensuring layout is the same let u1: [usize; 4] = unsafe { mem::transmute(from_static(HELLO.as_bytes())) }; + // SAFETY: ensuring layout is the same let u2: [usize; 4] = unsafe { mem::transmute(bytes::Bytes::from_static(HELLO.as_bytes())) }; assert_eq!(u1[..3], u2[..3]); // Struct bytes are equal besides Vtables diff --git a/serde_v8/serializable.rs b/serde_v8/serializable.rs index a724f300e4..abde544c76 100644 --- a/serde_v8/serializable.rs +++ b/serde_v8/serializable.rs @@ -97,6 +97,8 @@ impl From for SerializablePkg { fn from(x: T) -> Self { #[inline(always)] fn tc(src: T) -> U { + // SAFETY: the caller has ensured via the TypeId that the T and U types + // are the same. let x = unsafe { transmute_copy(&src) }; std::mem::forget(src); x diff --git a/serde_v8/utils.rs b/serde_v8/utils.rs index 427ac5f2f4..d491ac093d 100644 --- a/serde_v8/utils.rs +++ b/serde_v8/utils.rs @@ -17,6 +17,7 @@ pub fn v8_init() { } pub fn v8_shutdown() { + // SAFETY: this is safe, because all isolates have been shut down already. unsafe { v8::V8::dispose(); } diff --git a/test_ffi/src/lib.rs b/test_ffi/src/lib.rs index 5b813cd012..b4908d9cd7 100644 --- a/test_ffi/src/lib.rs +++ b/test_ffi/src/lib.rs @@ -1,5 +1,7 @@ // Copyright 2018-2022 the Deno authors. All rights reserved. MIT license. +#![allow(clippy::undocumented_unsafe_blocks)] + use std::os::raw::c_void; use std::thread::sleep; use std::time::Duration; @@ -11,23 +13,29 @@ pub extern "C" fn print_something() { println!("something"); } -#[allow(clippy::not_unsafe_ptr_arg_deref)] +/// # Safety +/// +/// The pointer to the buffer must be valid and initalized, and the length must +/// not be longer than the buffer's allocation. #[no_mangle] -pub extern "C" fn print_buffer(ptr: *const u8, len: usize) { - let buf = unsafe { std::slice::from_raw_parts(ptr, len) }; +pub unsafe extern "C" fn print_buffer(ptr: *const u8, len: usize) { + let buf = std::slice::from_raw_parts(ptr, len); println!("{:?}", buf); } -#[allow(clippy::not_unsafe_ptr_arg_deref)] +/// # Safety +/// +/// The pointer to the buffer must be valid and initalized, and the length must +/// not be longer than the buffer's allocation. #[no_mangle] -pub extern "C" fn print_buffer2( +pub unsafe extern "C" fn print_buffer2( ptr1: *const u8, len1: usize, ptr2: *const u8, len2: usize, ) { - let buf1 = unsafe { std::slice::from_raw_parts(ptr1, len1) }; - let buf2 = unsafe { std::slice::from_raw_parts(ptr2, len2) }; + let buf1 = std::slice::from_raw_parts(ptr1, len1); + let buf2 = std::slice::from_raw_parts(ptr2, len2); println!("{:?} {:?}", buf1, buf2); } @@ -87,19 +95,25 @@ pub extern "C" fn sleep_blocking(ms: u64) { sleep(duration); } -#[allow(clippy::not_unsafe_ptr_arg_deref)] +/// # Safety +/// +/// The pointer to the buffer must be valid and initalized, and the length must +/// not be longer than the buffer's allocation. #[no_mangle] -pub extern "C" fn fill_buffer(value: u8, buf: *mut u8, len: usize) { - let buf = unsafe { std::slice::from_raw_parts_mut(buf, len) }; +pub unsafe extern "C" fn fill_buffer(value: u8, buf: *mut u8, len: usize) { + let buf = std::slice::from_raw_parts_mut(buf, len); for itm in buf.iter_mut() { *itm = value; } } -#[allow(clippy::not_unsafe_ptr_arg_deref)] +/// # Safety +/// +/// The pointer to the buffer must be valid and initalized, and the length must +/// not be longer than the buffer's allocation. #[no_mangle] -pub extern "C" fn nonblocking_buffer(ptr: *const u8, len: usize) { - let buf = unsafe { std::slice::from_raw_parts(ptr, len) }; +pub unsafe extern "C" fn nonblocking_buffer(ptr: *const u8, len: usize) { + let buf = std::slice::from_raw_parts(ptr, len); assert_eq!(buf, vec![1, 2, 3, 4, 5, 6, 7, 8]); } diff --git a/tools/lint.js b/tools/lint.js index 18be09c657..f1fa96a2cf 100755 --- a/tools/lint.js +++ b/tools/lint.js @@ -116,6 +116,10 @@ async function clippy() { "clippy::all", "-D", "clippy::await_holding_refcell_ref", + "-D", + "clippy::missing_safety_doc", + "-D", + "clippy::undocumented_unsafe_blocks", ], stdout: "inherit", stderr: "inherit",