`#[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

This is not an unsafe operation.

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

src/libstd/sys/wasm/condvar_atomics.rs

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

This is also not an unsafe operation.

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

src/libstd/sys/wasm/mutex_atomics.rs

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

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

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

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

This is also not an unsafe operation.

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

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

This is not an unsafe operation.

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

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

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

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

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

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

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

This is not an unsafe operation.

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

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

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 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

Closed

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

Uh oh!

Conversation

chansuke commented Jul 18, 2020

Uh oh!

rust-highfive commented Jul 18, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors commented Jul 21, 2020

Uh oh!

bors commented Jul 28, 2020

Uh oh!

JohnTitor commented Aug 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crlf0710 commented Sep 4, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chansuke commented Sep 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mark-Simulacrum commented Oct 19, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chansuke commented Oct 24, 2020

Uh oh!

Mark-Simulacrum commented Oct 25, 2020

Uh oh!

bors commented Oct 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants