From d413bb6f5716261f2740eb67540df1da1469ce12 Mon Sep 17 00:00:00 2001 From: chansuke Date: Sat, 18 Jul 2020 21:59:53 +0900 Subject: [PATCH 1/5] `#[deny(unsafe_op_in_unsafe_fn)]` in sys/wasm --- library/std/src/sys/wasm/alloc.rs | 8 +++--- library/std/src/sys/wasm/condvar_atomics.rs | 11 +++++--- library/std/src/sys/wasm/mod.rs | 2 ++ library/std/src/sys/wasm/mutex_atomics.rs | 29 +++++++++++++-------- 4 files changed, 32 insertions(+), 18 deletions(-) diff --git a/library/std/src/sys/wasm/alloc.rs b/library/std/src/sys/wasm/alloc.rs index 32b8b5bdaea7a..e12af19718a7c 100644 --- a/library/std/src/sys/wasm/alloc.rs +++ b/library/std/src/sys/wasm/alloc.rs @@ -25,25 +25,25 @@ unsafe impl GlobalAlloc for System { #[inline] unsafe fn alloc(&self, layout: Layout) -> *mut u8 { let _lock = lock::lock(); - DLMALLOC.malloc(layout.size(), layout.align()) + unsafe { DLMALLOC.malloc(layout.size(), layout.align()) } } #[inline] unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 { let _lock = lock::lock(); - DLMALLOC.calloc(layout.size(), layout.align()) + unsafe { DLMALLOC.calloc(layout.size(), layout.align()) } } #[inline] unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) { let _lock = lock::lock(); - DLMALLOC.free(ptr, layout.size(), layout.align()) + unsafe { DLMALLOC.free(ptr, layout.size(), layout.align()) } } #[inline] unsafe fn realloc(&self, ptr: *mut u8, layout: Layout, new_size: usize) -> *mut u8 { let _lock = lock::lock(); - DLMALLOC.realloc(ptr, layout.size(), layout.align(), new_size) + unsafe { DLMALLOC.realloc(ptr, layout.size(), layout.align(), new_size) } } } diff --git a/library/std/src/sys/wasm/condvar_atomics.rs b/library/std/src/sys/wasm/condvar_atomics.rs index a96bb18e6ef1a..b9133e9fb7dbc 100644 --- a/library/std/src/sys/wasm/condvar_atomics.rs +++ b/library/std/src/sys/wasm/condvar_atomics.rs @@ -44,13 +44,18 @@ impl Condvar { pub unsafe fn notify_one(&self) { self.cnt.fetch_add(1, SeqCst); - wasm32::memory_atomic_notify(self.ptr(), 1); + // SAFETY: ptr() is always valid + unsafe { + wasm32::memory_atomic_notify(self.ptr(), 1); + } } #[inline] pub unsafe fn notify_all(&self) { - self.cnt.fetch_add(1, SeqCst); - wasm32::memory_atomic_notify(self.ptr(), u32::MAX); // -1 == "wake everyone" + unsafe { + self.cnt.fetch_add(1, SeqCst); + wasm32::memory_atomic_notify(self.ptr(), u32::MAX); // -1 == "wake everyone" + } } pub unsafe fn wait(&self, mutex: &Mutex) { diff --git a/library/std/src/sys/wasm/mod.rs b/library/std/src/sys/wasm/mod.rs index 11c6896f050b2..82683c0f624cf 100644 --- a/library/std/src/sys/wasm/mod.rs +++ b/library/std/src/sys/wasm/mod.rs @@ -14,6 +14,8 @@ //! compiling for wasm. That way it's a compile time error for something that's //! guaranteed to be a runtime error! +#![deny(unsafe_op_in_unsafe_fn)] + pub mod alloc; pub mod args; #[path = "../unsupported/cmath.rs"] diff --git a/library/std/src/sys/wasm/mutex_atomics.rs b/library/std/src/sys/wasm/mutex_atomics.rs index 2970fcf806cbf..11d7f4c389dec 100644 --- a/library/std/src/sys/wasm/mutex_atomics.rs +++ b/library/std/src/sys/wasm/mutex_atomics.rs @@ -28,11 +28,14 @@ impl Mutex { pub unsafe fn lock(&self) { while !self.try_lock() { - let val = wasm32::memory_atomic_wait32( - self.ptr(), - 1, // we expect our mutex is locked - -1, // wait infinitely - ); + // SAFETY: the caller must uphold the safety contract for `memory_atomic_wait32`. + let val = unsafe { + wasm32::memory_atomic_wait32( + self.ptr(), + 1, // we expect our mutex is locked + -1, // wait infinitely + ) + }; // we should have either woke up (0) or got a not-equal due to a // race (1). We should never time out (2) debug_assert!(val == 0 || val == 1); @@ -47,7 +50,7 @@ impl Mutex { #[inline] pub unsafe fn try_lock(&self) -> bool { - self.locked.compare_exchange(0, 1, SeqCst, SeqCst).is_ok() + unsafe { self.locked.compare_exchange(0, 1, SeqCst, SeqCst).is_ok() } } #[inline] @@ -83,7 +86,7 @@ unsafe impl Sync for ReentrantMutex {} impl ReentrantMutex { pub const unsafe fn uninitialized() -> ReentrantMutex { - ReentrantMutex { owner: AtomicU32::new(0), recursions: UnsafeCell::new(0) } + unsafe { ReentrantMutex { owner: AtomicU32::new(0), recursions: UnsafeCell::new(0) } } } pub unsafe fn init(&self) { @@ -93,19 +96,20 @@ impl ReentrantMutex { pub unsafe fn lock(&self) { let me = thread::my_id(); while let Err(owner) = self._try_lock(me) { - let val = wasm32::memory_atomic_wait32(self.ptr(), owner as i32, -1); + // SAFETY: the caller must gurantee that `self.ptr()` and `owner` are valid i32. + let val = unsafe { wasm32::memory_atomic_wait32(self.ptr(), owner as i32, -1) }; debug_assert!(val == 0 || val == 1); } } #[inline] pub unsafe fn try_lock(&self) -> bool { - self._try_lock(thread::my_id()).is_ok() + unsafe { self._try_lock(thread::my_id()).is_ok() } } #[inline] unsafe fn _try_lock(&self, id: u32) -> Result<(), u32> { - let id = id.checked_add(1).unwrap(); // make sure `id` isn't 0 + let id = id.checked_add(1).unwrap(); match self.owner.compare_exchange(0, id, SeqCst, SeqCst) { // we transitioned from unlocked to locked Ok(_) => { @@ -132,7 +136,10 @@ impl ReentrantMutex { match *self.recursions.get() { 0 => { self.owner.swap(0, SeqCst); - wasm32::memory_atomic_notify(self.ptr() as *mut i32, 1); // wake up one waiter, if any + // SAFETY: the caller must gurantee that `self.ptr()` is valid i32. + unsafe { + wasm32::atomic_notify(self.ptr() as *mut i32, 1); + } // wake up one waiter, if any } ref mut n => *n -= 1, } From eed45107da8da6293d9cecc302ad3b9870848b51 Mon Sep 17 00:00:00 2001 From: chansuke Date: Wed, 21 Oct 2020 08:15:28 +0900 Subject: [PATCH 2/5] Add some description for (malloc/calloc/free/realloc) --- library/std/src/sys/wasm/alloc.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/library/std/src/sys/wasm/alloc.rs b/library/std/src/sys/wasm/alloc.rs index e12af19718a7c..b85f4d50021e3 100644 --- a/library/std/src/sys/wasm/alloc.rs +++ b/library/std/src/sys/wasm/alloc.rs @@ -24,24 +24,28 @@ static mut DLMALLOC: dlmalloc::Dlmalloc = dlmalloc::DLMALLOC_INIT; unsafe impl GlobalAlloc for System { #[inline] unsafe fn alloc(&self, layout: Layout) -> *mut u8 { + // SAFETY: DLMALLOC.malloc() is guranteed to be safe since lock::lock() aqcuire a globl lock let _lock = lock::lock(); unsafe { DLMALLOC.malloc(layout.size(), layout.align()) } } #[inline] unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 { + // SAFETY: DLMALLOC.calloc() is guranteed to be safe since lock::lock() aqcuire a globl lock let _lock = lock::lock(); unsafe { DLMALLOC.calloc(layout.size(), layout.align()) } } #[inline] unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) { + // SAFETY: DLMALLOC.free() is guranteed to be safe since lock::lock() aqcuire a globl lock let _lock = lock::lock(); unsafe { DLMALLOC.free(ptr, layout.size(), layout.align()) } } #[inline] unsafe fn realloc(&self, ptr: *mut u8, layout: Layout, new_size: usize) -> *mut u8 { + // SAFETY: DLMALLOC.realloc() is guranteed to be safe since lock::lock() aqcuire a globl lock let _lock = lock::lock(); unsafe { DLMALLOC.realloc(ptr, layout.size(), layout.align(), new_size) } } From de87ae79610925502f45ec07cf24bac51e037ed1 Mon Sep 17 00:00:00 2001 From: chansuke Date: Sat, 24 Oct 2020 17:59:58 +0900 Subject: [PATCH 3/5] Add documents for DLMALLOC --- library/std/src/sys/wasm/alloc.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/library/std/src/sys/wasm/alloc.rs b/library/std/src/sys/wasm/alloc.rs index b85f4d50021e3..b61a7872265f3 100644 --- a/library/std/src/sys/wasm/alloc.rs +++ b/library/std/src/sys/wasm/alloc.rs @@ -24,28 +24,32 @@ static mut DLMALLOC: dlmalloc::Dlmalloc = dlmalloc::DLMALLOC_INIT; unsafe impl GlobalAlloc for System { #[inline] unsafe fn alloc(&self, layout: Layout) -> *mut u8 { - // SAFETY: DLMALLOC.malloc() is guranteed to be safe since lock::lock() aqcuire a globl lock + // SAFETY: DLMALLOC access is guranteed to be safe because the lock gives us unique and non-reentrant access. + // Calling malloc() is safe because preconditions on this function match the trait method preconditions. let _lock = lock::lock(); unsafe { DLMALLOC.malloc(layout.size(), layout.align()) } } #[inline] unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 { - // SAFETY: DLMALLOC.calloc() is guranteed to be safe since lock::lock() aqcuire a globl lock + // SAFETY: DLMALLOC access is guranteed to be safe because the lock gives us unique and non-reentrant access. + // Calling calloc() is safe because preconditions on this function match the trait method preconditions. let _lock = lock::lock(); unsafe { DLMALLOC.calloc(layout.size(), layout.align()) } } #[inline] unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) { - // SAFETY: DLMALLOC.free() is guranteed to be safe since lock::lock() aqcuire a globl lock + // SAFETY: DLMALLOC access is guranteed to be safe because the lock gives us unique and non-reentrant access. + // Calling free() is safe because preconditions on this function match the trait method preconditions. let _lock = lock::lock(); unsafe { DLMALLOC.free(ptr, layout.size(), layout.align()) } } #[inline] unsafe fn realloc(&self, ptr: *mut u8, layout: Layout, new_size: usize) -> *mut u8 { - // SAFETY: DLMALLOC.realloc() is guranteed to be safe since lock::lock() aqcuire a globl lock + // SAFETY: DLMALLOC access is guranteed to be safe because the lock gives us unique and non-reentrant access. + // Calling realloc() is safe because preconditions on this function match the trait method preconditions. let _lock = lock::lock(); unsafe { DLMALLOC.realloc(ptr, layout.size(), layout.align(), new_size) } } From d147f78e367386bf63ccb03d453e151e37cfdd81 Mon Sep 17 00:00:00 2001 From: chansuke Date: Sat, 24 Oct 2020 18:14:17 +0900 Subject: [PATCH 4/5] Fix unsafe operation of wasm32::memory_atomic_notify --- library/std/src/sys/wasm/condvar_atomics.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/library/std/src/sys/wasm/condvar_atomics.rs b/library/std/src/sys/wasm/condvar_atomics.rs index b9133e9fb7dbc..c2c47910582a0 100644 --- a/library/std/src/sys/wasm/condvar_atomics.rs +++ b/library/std/src/sys/wasm/condvar_atomics.rs @@ -52,8 +52,9 @@ impl Condvar { #[inline] pub unsafe fn notify_all(&self) { + self.cnt.fetch_add(1, SeqCst); + // SAFETY: memory_atomic_notify()is always valid unsafe { - self.cnt.fetch_add(1, SeqCst); wasm32::memory_atomic_notify(self.ptr(), u32::MAX); // -1 == "wake everyone" } } From d37b8cf729187e5dcabb3650031eb806d1f79770 Mon Sep 17 00:00:00 2001 From: chansuke Date: Sat, 24 Oct 2020 18:22:18 +0900 Subject: [PATCH 5/5] Remove unnecessary unsafe block from condvar_atomics & mutex_atomics --- library/std/src/sys/wasm/condvar_atomics.rs | 2 +- library/std/src/sys/wasm/mutex_atomics.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/library/std/src/sys/wasm/condvar_atomics.rs b/library/std/src/sys/wasm/condvar_atomics.rs index c2c47910582a0..0c1c076cc9142 100644 --- a/library/std/src/sys/wasm/condvar_atomics.rs +++ b/library/std/src/sys/wasm/condvar_atomics.rs @@ -53,7 +53,7 @@ impl Condvar { #[inline] pub unsafe fn notify_all(&self) { self.cnt.fetch_add(1, SeqCst); - // SAFETY: memory_atomic_notify()is always valid + // SAFETY: ptr() is always valid unsafe { wasm32::memory_atomic_notify(self.ptr(), u32::MAX); // -1 == "wake everyone" } diff --git a/library/std/src/sys/wasm/mutex_atomics.rs b/library/std/src/sys/wasm/mutex_atomics.rs index 11d7f4c389dec..479182ffa44d5 100644 --- a/library/std/src/sys/wasm/mutex_atomics.rs +++ b/library/std/src/sys/wasm/mutex_atomics.rs @@ -50,7 +50,7 @@ impl Mutex { #[inline] pub unsafe fn try_lock(&self) -> bool { - unsafe { self.locked.compare_exchange(0, 1, SeqCst, SeqCst).is_ok() } + self.locked.compare_exchange(0, 1, SeqCst, SeqCst).is_ok() } #[inline] @@ -86,7 +86,7 @@ unsafe impl Sync for ReentrantMutex {} impl ReentrantMutex { pub const unsafe fn uninitialized() -> ReentrantMutex { - unsafe { ReentrantMutex { owner: AtomicU32::new(0), recursions: UnsafeCell::new(0) } } + ReentrantMutex { owner: AtomicU32::new(0), recursions: UnsafeCell::new(0) } } pub unsafe fn init(&self) {