Skip to content
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

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

Closed
19 of 22 tasks
LeSeulArtichaut opened this issue Jun 30, 2020 · 38 comments
Closed
19 of 22 tasks

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

LeSeulArtichaut opened this issue Jun 30, 2020 · 38 comments
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. F-unsafe-block-in-unsafe-fn RFC #2585 T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@LeSeulArtichaut
Copy link
Contributor

LeSeulArtichaut commented Jun 30, 2020

The goal of this effort is to #![deny(unsafe_op_in_unsafe_fn)] in all of libstd, as proposed in rust-lang/compiler-team#317. This means enclosing unsafe operations in unsafe functions inside unsafe blocks, and documenting them as much as possible.
However, as libstd contains more than 100,000 lines (!!!) and 600 unsafe functions, this should be done step by step, and by multiple people. This issue is meant as a way of tracking and synchronizing progress.

cc @nikomatsakis @RalfJung

Mentoring instructions (or rather, suggested workflow)

Please first leave a comment here stating that you want to work on file xxx.rs or module xxx, to make sure that this implements Sync.

You'll first want to add a #![deny(unsafe_op_in_unsafe_fn)] attribute in the scope you'll be working on.
Then, add unsafe blocks around unsafe operations in unsafe functions. These unsafe operations can be found either by searching for unsafe fns, or by running ./x.py check src/libstd and looking at the errors.
When adding an unsafe block, try to explain why it is safe in a safety comment before the unsafe block. This should look like:

// SAFETY: explain why `unsf` is safe here...
unsafe {
    unsf();
}

Example PRs: #72709 (for liballoc) and #73622 (for libcore)

TODO list

@LeSeulArtichaut LeSeulArtichaut self-assigned this Jun 30, 2020
@LeSeulArtichaut LeSeulArtichaut added C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. F-unsafe-block-in-unsafe-fn RFC #2585 T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 30, 2020
@euclio
Copy link
Contributor

euclio commented Jun 30, 2020

I'd like to work on sys/unix.

@eltonlaw
Copy link
Contributor

eltonlaw commented Jul 1, 2020

I'd like to work on fs.rs

@ryr3
Copy link
Contributor

ryr3 commented Jul 1, 2020

I'd like to take up net/tcp.rs

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Jul 1, 2020
…ckler

`#[deny(unsafe_op_in_unsafe_fn)]` in libstd/fs.rs

The `libstd/fs.rs` part of rust-lang#73904 . Wraps the two calls to an unsafe fn `Initializer::nop()` in an `unsafe` block.

Followed instructions in parent issue, ran `./x.py check src/libstd/` after adding the lint and two warnings were given. After adding these changes, those disappear.
Manishearth added a commit to Manishearth/rust that referenced this issue Jul 2, 2020
…ckler

`#[deny(unsafe_op_in_unsafe_fn)]` in libstd/fs.rs

The `libstd/fs.rs` part of rust-lang#73904 . Wraps the two calls to an unsafe fn `Initializer::nop()` in an `unsafe` block.

Followed instructions in parent issue, ran `./x.py check src/libstd/` after adding the lint and two warnings were given. After adding these changes, those disappear.
Manishearth added a commit to Manishearth/rust that referenced this issue Jul 2, 2020
…ckler

`#[deny(unsafe_op_in_unsafe_fn)]` in libstd/fs.rs

The `libstd/fs.rs` part of rust-lang#73904 . Wraps the two calls to an unsafe fn `Initializer::nop()` in an `unsafe` block.

Followed instructions in parent issue, ran `./x.py check src/libstd/` after adding the lint and two warnings were given. After adding these changes, those disappear.
Manishearth added a commit to Manishearth/rust that referenced this issue Jul 2, 2020
…ckler

`#[deny(unsafe_op_in_unsafe_fn)]` in libstd/fs.rs

The `libstd/fs.rs` part of rust-lang#73904 . Wraps the two calls to an unsafe fn `Initializer::nop()` in an `unsafe` block.

Followed instructions in parent issue, ran `./x.py check src/libstd/` after adding the lint and two warnings were given. After adding these changes, those disappear.
@hellow554
Copy link
Contributor

Took care of process.rs in #73955

What do you expect for primitive_docs.rs?

@LeSeulArtichaut
Copy link
Contributor Author

What do you expect for primitive_docs.rs?

I built that list by running ripgrep to find occurrences of "unsafe fn" and blindly copied it. You’re right, there’s nothing to do, obviously since everything is comments :D

@retep998
Copy link
Member

retep998 commented Jul 2, 2020

I'd like to be pinged on any PRs that touch windows bits.

@hellow554
Copy link
Contributor

FYI: #73909 has been merged and now #![feature(unsafe_block_in_unsafe_fn)] is activated in libstd/lib.rs

@ryr3
Copy link
Contributor

ryr3 commented Jul 2, 2020

Can I try working on the io part once this is merged?
This one-liner to open the files that contain unsafe fns might be useful to someone working on this. Just thought I'd share!

grep -l -d recurse 'unsafe fn' io | xargs vim # Or your editor

@LeSeulArtichaut
Copy link
Contributor Author

Can I try working on the io part once this is merged?

@ryr3 please go ahead!

@ryr3
Copy link
Contributor

ryr3 commented Jul 2, 2020

I started to work on it and saw that some files required other crates which need to be changed first.
For example, stdio imports thread/local.rs which has many unsafe fns. This leads to an error when running ./x.py check src/libstd
Is there any way to get around this, or will thread/local.rs have to be fixed first?

@LeSeulArtichaut
Copy link
Contributor Author

I started to work on it and saw that some files required other crates which need to be changed first.
For example, stdio imports thread/local.rs which has many unsafe fns. This leads to an error when running ./x.py check src/libstd
Is there any way to get around this, or will thread/local.rs have to be fixed first?

I think that's only macros being problematic. You cannot put unsafe blocks in the macro, because it will make use sites which aren't using #[deny(unsafe_op_in_unsafe_fn)] fail to compile because of the "unnecessary unsafe block" warning. And there's no quick fix, because the macros defined in thread/local.rs are also used in other crates so you can't just slap an #[allow(unsafe_op_in_unsafe_fn)] in the macro. I think this is definitely an issue, but which is going to disappear when we stabilize #![feature(unsafe_block_in_unsafe_fn)]. cc @nikomatsakis for advice on this.

@LeSeulArtichaut
Copy link
Contributor Author

Also cc @RalfJung as this problem might have an impact on the design of the feature.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Sep 15, 2020
…mulacrum

deny(unsafe_op_in_unsafe_fn) in libstd/process.rs

The libstd/process.rs part of rust-lang#73904 . Wraps the two calls to an unsafe fn Initializer::nop() in an unsafe block.

Will have to wait for rust-lang#73909 to be merged, because of the feature in the libstd/lib.rs
RalfJung added a commit to RalfJung/rust that referenced this issue Sep 19, 2020
…acrum

deny(unsafe_op_in_unsafe_fn) in libstd/path.rs

The libstd/path.rs part of rust-lang#73904 . Wraps the two calls to an unsafe fn Initializer::nop() in an unsafe block.
RalfJung added a commit to RalfJung/rust that referenced this issue Sep 19, 2020
…acrum

deny(unsafe_op_in_unsafe_fn) in libstd/path.rs

The libstd/path.rs part of rust-lang#73904 . Wraps the two calls to an unsafe fn Initializer::nop() in an unsafe block.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Sep 21, 2020
…n-unsafe-fn, r=joshtriplett

Std/thread: deny unsafe op in unsafe fn

Partial fix of rust-lang#73904.

This encloses `unsafe` operations in `unsafe fn` in `libstd/thread`.
@rustbot modify labels: F-unsafe-block-in-unsafe-fn
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Sep 21, 2020
…n-unsafe-fn, r=joshtriplett

Std/thread: deny unsafe op in unsafe fn

Partial fix of rust-lang#73904.

This encloses `unsafe` operations in `unsafe fn` in `libstd/thread`.
@rustbot modify labels: F-unsafe-block-in-unsafe-fn
RalfJung added a commit to RalfJung/rust that referenced this issue Sep 21, 2020
…n-unsafe-fn, r=joshtriplett

Std/thread: deny unsafe op in unsafe fn

Partial fix of rust-lang#73904.

This encloses `unsafe` operations in `unsafe fn` in `libstd/thread`.
@rustbot modify labels: F-unsafe-block-in-unsafe-fn
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 26, 2020
…unsafe-fn, r=joshtriplett

Std/thread: deny unsafe op in unsafe fn

Partial fix of rust-lang#73904.

This encloses `unsafe` operations in `unsafe fn` in `libstd/thread`.
`@rustbot` modify labels: F-unsafe-block-in-unsafe-fn
@ghost
Copy link

ghost commented Sep 29, 2020

I would like to take sys/sgx.

bors added a commit to rust-lang-ci/rust that referenced this issue Oct 8, 2020
`#[deny(unsafe_op_in_unsafe_fn)]` in sys/sgx

This is part of rust-lang#73904.

Enclose unsafe operations in unsafe blocks in `libstd/sys/sgx`.
@m-ou-se
Copy link
Member

m-ou-se commented Oct 8, 2020

sys/unsupported is missing in the list. That one was not much work: #77722

sys/vxworks will be mostly fixed by #77666

@ghost
Copy link

ghost commented Oct 8, 2020

sys/sgx merged in #77346.

@Mark-Simulacrum Mark-Simulacrum added E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. and removed E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. labels Oct 22, 2020
@Mark-Simulacrum
Copy link
Member

I have adjusted the E- tag here to E-hard, because for the most part, correctly checking the requirements and determining the appropriate safety comment to write is not an easy task. It might be a medium task (unclear), but it feels hard to me.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Oct 26, 2020
…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
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Oct 26, 2020
…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
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Oct 26, 2020
…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
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Nov 6, 2020
`#![deny(unsafe_op_in_unsafe_fn)]` in sys/hermit

Partial fix of rust-lang#73904.

This encloses ``unsafe`` operations in ``unsafe fn`` in ``sys/hermit``.
Some unsafe blocks are not well documented because some system-based functions lack documents.
JohnTitor added a commit to JohnTitor/rust that referenced this issue Nov 6, 2020
`#![deny(unsafe_op_in_unsafe_fn)]` in sys/hermit

Partial fix of rust-lang#73904.

This encloses ``unsafe`` operations in ``unsafe fn`` in ``sys/hermit``.
Some unsafe blocks are not well documented because some system-based functions lack documents.
JohnTitor added a commit to JohnTitor/rust that referenced this issue Nov 6, 2020
`#![deny(unsafe_op_in_unsafe_fn)]` in sys/hermit

Partial fix of rust-lang#73904.

This encloses ``unsafe`` operations in ``unsafe fn`` in ``sys/hermit``.
Some unsafe blocks are not well documented because some system-based functions lack documents.
@LeSeulArtichaut LeSeulArtichaut removed their assignment Jun 2, 2021
@RalfJung
Copy link
Member

@LeSeulArtichaut @ryr3 @euclio @carbotaniuman you are currently "registered" at the top of this issue for various parts of std to be ported to unsafe_op_in_unsafe_fn. However, there doesn't seem to have been progress in a while -- do you still plan to pick this up, or can I remove your names from the issue so others can pick it up if they want to?

@carbotaniuman
Copy link
Contributor

Feel free to take my name off, I think I put my name on before a hiatus and I don't remember this anymore.

@workingjubilee
Copy link
Member

The initial task list described in this issue has been completed or made out-of-date.

The rest is tracked in #127747

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. F-unsafe-block-in-unsafe-fn RFC #2585 T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests