`#[deny(unsafe_op_in_unsafe_fn)]` in sys/wasm #74477

chansuke · 2020-07-18T13:30:51Z

This is part of #73904.

This encloses unsafe operations in unsafe fn in libstd/sys/wasm.

@rustbot modify labels: F-unsafe-block-in-unsafe-fn

rust-highfive · 2020-07-18T13:30:54Z

r? @joshtriplett

(rust_highfive has picked a reviewer for you, use r? to override)

Mark-Simulacrum · 2020-07-18T13:37:02Z

src/libstd/sys/wasm/condvar_atomics.rs

+        // SAFETY: The caller must gurantee that the argument of `fetch_add()` is valid
+        // to describe memory ordering.
+        unsafe {
+            self.cnt.fetch_add(1, SeqCst);


This is not an unsafe operation.

Mark-Simulacrum · 2020-07-18T13:39:00Z

src/libstd/sys/wasm/condvar_atomics.rs

+            self.cnt.fetch_add(1, SeqCst);
+        }
+        unsafe {
+            wasm32::atomic_notify(self.ptr(), 1);


My guess is that the unsafety here is basically just from the raw pointer, so we can say something like "ptr() is always valid"

Mark-Simulacrum · 2020-07-18T13:39:38Z

src/libstd/sys/wasm/condvar_atomics.rs

@@ -60,7 +72,7 @@ impl Condvar {
        // thread. Incrementing the counter after we unlock the mutex will
        // prevent us from sleeping and otherwise the call to `wake` will
        // wake us up once we're asleep.
-        let ticket = self.cnt.load(SeqCst) as i32;
+        let ticket = unsafe { self.cnt.load(SeqCst) as i32 };


This is also not an unsafe operation.

Mark-Simulacrum · 2020-07-18T13:41:26Z

src/libstd/sys/wasm/mutex_atomics.rs

-                1,  // we expect our mutex is locked
-                -1, // wait infinitely
-            );
+            // SAFETY: the caller must uphold the safety contract for `i32_atomic_wait`.


This seems... odd. @alexcrichton, can you recall why lock() here is unsafe? It seems like it could be safe.

The intrinsic has a *mut i32 in the signature which it modifies, so the intrinsic is unsafe.

Mark-Simulacrum · 2020-07-18T13:41:41Z

src/libstd/sys/wasm/mutex_atomics.rs

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

    pub unsafe fn unlock(&self) {
-        let prev = self.locked.swap(0, SeqCst);
+        // SAFETY: the caller must uphold the safety contract for `swap`.
+        let prev = unsafe { self.locked.swap(0, SeqCst) };


Atomic operations are safe.

Thank you for the review

bors · 2020-07-21T01:43:33Z

☔ The latest upstream changes (presumably #74569) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2020-07-28T03:13:14Z

☔ The latest upstream changes (presumably #73265) made this pull request unmergeable. Please resolve the merge conflicts.

JohnTitor · 2020-08-06T22:31:08Z

This hasn't got any response for a while, r? @Mark-Simulacrum as you left some reviews.

Mark-Simulacrum · 2020-08-06T22:48:27Z

library/std/src/sys/wasm/condvar_atomics.rs

+        // to describe memory ordering.
+        unsafe {
+            self.cnt.fetch_add(1, SeqCst);
+        }


Not an unsafe operation (please also fix the other atomics).

crlf0710 · 2020-09-04T16:13:33Z

@chansuke Ping from triage! What's the current status on this? i assume this is ready for review again?

Mark-Simulacrum · 2020-09-09T13:04:37Z

library/std/src/sys/wasm/mutex_atomics.rs

-            );
+            // SAFETY: the caller must uphold the safety contract for `i32_atomic_wait`.
+            let val = unsafe {
+                wasm32::i32_atomic_wait(


I think this is a rebase failure -- this function seems to have been renamed to memory_atomic_wait32 on master. (I think there's one or two more cases where you have that).

Mark-Simulacrum · 2020-09-09T13:05:12Z

library/std/src/sys/wasm/mutex_atomics.rs

    }

    #[inline]
    unsafe fn _try_lock(&self, id: u32) -> Result<(), u32> {
-        let id = id.checked_add(1).unwrap(); // make sure `id` isn't 0
+        // SAFETY: the caller must gurantee that `id` isn't 0
+        let id = unsafe { id.checked_add(1).unwrap() };


This is also not an unsafe operation.

Mark-Simulacrum · 2020-09-09T13:05:21Z

library/std/src/sys/wasm/mutex_atomics.rs

-                self.owner.swap(0, SeqCst);
-                wasm32::memory_atomic_notify(self.ptr() as *mut i32, 1); // wake up one waiter, if any
+                unsafe {
+                    self.owner.swap(0, SeqCst);


This is not an unsafe operation.

Mark-Simulacrum · 2020-09-09T13:05:53Z

library/std/src/sys/wasm/condvar_atomics.rs

-        self.cnt.fetch_add(1, SeqCst);
-        wasm32::memory_atomic_notify(self.ptr(), u32::MAX); // -1 == "wake everyone"
+        unsafe {
+            wasm32::atomic_notify(self.ptr(), u32::MAX); // -1 == "wake everyone"


This also lost the right name.

Sorry, I fixed...

chansuke · 2020-09-09T13:41:55Z

@crlf0710 Sorry for the late reply. I fixed and now it's ready for the review.

Mark-Simulacrum · 2020-09-10T20:42:27Z

library/std/src/sys/wasm/condvar_atomics.rs

-        wasm32::memory_atomic_notify(self.ptr(), 1);
+        // SAFETY: ptr() is always valid
+        unsafe {
+            wasm32::atomic_notify(self.ptr(), 1);


I think this is still the wrong name, it should be memory_atomic_notify.

library/std/src/sys/wasm/condvar_atomics.rs

Mark-Simulacrum · 2020-09-10T20:43:15Z

library/std/src/sys/wasm/mutex_atomics.rs

-                wasm32::memory_atomic_notify(self.ptr() as *mut i32, 1); // wake up one waiter, if any
+                unsafe {
+                    wasm32::atomic_notify(self.ptr() as *mut i32, 1);
+                } // wake up one waiter, if any


Can you put this comment above the unsafe block instead?

Mark-Simulacrum · 2020-10-19T21:38:03Z

I think I likely approved the SGX changes a bit earlier than was warranted (sgx is also less critical in some sense as it's not to my knowledge something readily used by consumers, though I could be wrong).

lock::lock() is defined in the alloc.rs file and is just taking a global lock if std is compiled with atomics enabled.

We need the locks with multithreading because the allocator is called with &self but the malloc/calloc/free/realloc on dlmalloc are defined to take &mut self, and that can only be safe if we ensure safety somehow (in this case with a lock).

Mark-Simulacrum · 2020-10-22T23:42:41Z

library/std/src/sys/wasm/alloc.rs

        let _lock = lock::lock();
-        DLMALLOC.malloc(layout.size(), layout.align())
+        unsafe { DLMALLOC.malloc(layout.size(), layout.align()) }


There are two preconditions that we need to document for these calls:

DLMALLOC access is safe: this is true because the lock gives us unique and non-reentrant access.

Calling the function itself is safe: Preconditions on these functions match the trait method preconditions, so this is safe.

The comments so far only sort of talk about the first, but should address both. I think basically copy/pasting my two bullet points makes sense.

Mark-Simulacrum · 2020-10-22T23:47:27Z

library/std/src/sys/wasm/condvar_atomics.rs

-        self.cnt.fetch_add(1, SeqCst);
-        wasm32::memory_atomic_notify(self.ptr(), u32::MAX); // -1 == "wake everyone"
+        unsafe {
+            self.cnt.fetch_add(1, SeqCst);


This is not an unsafe operation and should not be inside the unsafe block. It also looks like are missing a safety comment about the first argument to memory_atomic_notify being a valid pointer.

library/std/src/sys/wasm/mutex_atomics.rs

Mark-Simulacrum · 2020-10-22T23:49:49Z

library/std/src/sys/wasm/mutex_atomics.rs

@@ -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() }


This is not an unsafe operation.

Mark-Simulacrum · 2020-10-22T23:50:02Z

library/std/src/sys/wasm/mutex_atomics.rs

@@ -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) } }


I don't believe there are any unsafe operations here.

chansuke · 2020-10-24T10:11:12Z

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

Mark-Simulacrum · 2020-10-25T21:38:15Z

@bors r+ rollup

bors · 2020-10-25T21:38:17Z

📌 Commit d37b8cf has been approved by Mark-Simulacrum

@rustbot

…fe-fn, r=Mark-Simulacrum `#[deny(unsafe_op_in_unsafe_fn)]` in sys/wasm This is part of rust-lang#73904. This encloses unsafe operations in unsafe fn in `libstd/sys/wasm`. @rustbot modify labels: F-unsafe-block-in-unsafe-fn

@rustbot

…fe-fn, r=Mark-Simulacrum `#[deny(unsafe_op_in_unsafe_fn)]` in sys/wasm This is part of rust-lang#73904. This encloses unsafe operations in unsafe fn in `libstd/sys/wasm`. @rustbot modify labels: F-unsafe-block-in-unsafe-fn

Rollup of 10 pull requests Successful merges: - rust-lang#74477 (`#[deny(unsafe_op_in_unsafe_fn)]` in sys/wasm) - rust-lang#77836 (transmute_copy: explain that alignment is handled correctly) - rust-lang#78126 (Properly define va_arg and va_list for aarch64-apple-darwin) - rust-lang#78137 (Initialize tracing subscriber in compiletest tool) - rust-lang#78161 (Add issue template link to IRLO) - rust-lang#78214 (Tweak match arm semicolon removal suggestion to account for futures) - rust-lang#78247 (Fix rust-lang#78192) - rust-lang#78252 (Add codegen test for rust-lang#45964) - rust-lang#78268 (Do not try to report on closures to avoid ICE) - rust-lang#78295 (Add some regression tests) Failed merges: r? `@ghost`

rustbot added the F-unsafe-block-in-unsafe-fn RFC #2585 label Jul 18, 2020

rust-highfive assigned joshtriplett Jul 18, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 18, 2020

Mark-Simulacrum reviewed Jul 18, 2020

View reviewed changes

LeSeulArtichaut mentioned this pull request Jul 18, 2020

#![deny(unsafe_op_in_unsafe_fn)] in libstd #73904

Open

22 tasks

chansuke force-pushed the sys-wasm-unsafe-op-in-unsafe-fn branch from 0054eb3 to 90d4625 Compare July 22, 2020 12:03

chansuke force-pushed the sys-wasm-unsafe-op-in-unsafe-fn branch 2 times, most recently from 440f06c to c0accca Compare August 3, 2020 14:51

rust-highfive assigned Mark-Simulacrum and unassigned joshtriplett Aug 6, 2020

Mark-Simulacrum reviewed Aug 6, 2020

View reviewed changes

Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 13, 2020

chansuke force-pushed the sys-wasm-unsafe-op-in-unsafe-fn branch from c0accca to fa379b4 Compare August 14, 2020 13:23

crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 4, 2020

Mark-Simulacrum reviewed Sep 9, 2020

View reviewed changes

Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 9, 2020

chansuke force-pushed the sys-wasm-unsafe-op-in-unsafe-fn branch from fa379b4 to 353727b Compare September 9, 2020 13:23

crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 10, 2020

Mark-Simulacrum reviewed Sep 10, 2020

View reviewed changes

Mark-Simulacrum removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 10, 2020

Mark-Simulacrum added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 19, 2020

chansuke force-pushed the sys-wasm-unsafe-op-in-unsafe-fn branch from 4a17d19 to 2ff5965 Compare October 20, 2020 23:28

Mark-Simulacrum reviewed Oct 22, 2020

View reviewed changes

Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 22, 2020

chansuke added 5 commits October 24, 2020 11:50

#[deny(unsafe_op_in_unsafe_fn)] in sys/wasm

d413bb6

Add some description for (malloc/calloc/free/realloc)

eed4510

Add documents for DLMALLOC

de87ae7

Fix unsafe operation of wasm32::memory_atomic_notify

d147f78

Remove unnecessary unsafe block from condvar_atomics & mutex_atomics

d37b8cf

chansuke force-pushed the sys-wasm-unsafe-op-in-unsafe-fn branch from 2ff5965 to d37b8cf Compare October 24, 2020 09:39

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 24, 2020

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 25, 2020

Dylan-DPC-zz mentioned this pull request Oct 26, 2020

Rollup of 10 pull requests #78383

Closed

Dylan-DPC-zz mentioned this pull request Oct 26, 2020

Rollup of 9 pull requests #78386

Closed

Dylan-DPC-zz mentioned this pull request Oct 26, 2020

Rollup of 10 pull requests #78387

Merged

bors merged commit e0c08ae into rust-lang:master Oct 26, 2020

rustbot added this to the 1.49.0 milestone Oct 26, 2020

Mark-Simulacrum mentioned this pull request Nov 5, 2020

Apply #![deny(unsafe_op_in_unsafe_fn)] to sys/windows #76676

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`#[deny(unsafe_op_in_unsafe_fn)]` in sys/wasm #74477

`#[deny(unsafe_op_in_unsafe_fn)]` in sys/wasm #74477

chansuke commented Jul 18, 2020

rust-highfive commented Jul 18, 2020

Mark-Simulacrum Jul 18, 2020

Mark-Simulacrum Jul 18, 2020

Mark-Simulacrum Jul 18, 2020

Mark-Simulacrum Jul 18, 2020

alexcrichton Jul 20, 2020

Mark-Simulacrum Jul 18, 2020

chansuke Jul 20, 2020

bors commented Jul 21, 2020

bors commented Jul 28, 2020

JohnTitor commented Aug 6, 2020

Mark-Simulacrum Aug 6, 2020

chansuke Aug 14, 2020

crlf0710 commented Sep 4, 2020

Mark-Simulacrum Sep 9, 2020

Mark-Simulacrum Sep 9, 2020

Mark-Simulacrum Sep 9, 2020

Mark-Simulacrum Sep 9, 2020

chansuke Sep 9, 2020

chansuke commented Sep 9, 2020

Mark-Simulacrum Sep 10, 2020

Mark-Simulacrum Sep 10, 2020

Mark-Simulacrum commented Oct 19, 2020

Mark-Simulacrum Oct 22, 2020

Mark-Simulacrum Oct 22, 2020

Mark-Simulacrum Oct 22, 2020

Mark-Simulacrum Oct 22, 2020

chansuke commented Oct 24, 2020

Mark-Simulacrum commented Oct 25, 2020

bors commented Oct 25, 2020

#[deny(unsafe_op_in_unsafe_fn)] in sys/wasm #74477

#[deny(unsafe_op_in_unsafe_fn)] in sys/wasm #74477

Conversation

chansuke commented Jul 18, 2020

rust-highfive commented Jul 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Jul 21, 2020

bors commented Jul 28, 2020

JohnTitor commented Aug 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crlf0710 commented Sep 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chansuke commented Sep 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mark-Simulacrum commented Oct 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chansuke commented Oct 24, 2020

Mark-Simulacrum commented Oct 25, 2020

bors commented Oct 25, 2020

`#[deny(unsafe_op_in_unsafe_fn)]` in sys/wasm #74477

`#[deny(unsafe_op_in_unsafe_fn)]` in sys/wasm #74477