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

Expose unsafe operations #190

Closed
gnzlbg opened this issue Nov 4, 2018 · 9 comments
Closed

Expose unsafe operations #190

gnzlbg opened this issue Nov 4, 2018 · 9 comments
Labels
E-hard E-has-instructions Issue has some instructions and pointers to code to get started fun A technically challenging issue with high impact

Comments

@gnzlbg
Copy link

gnzlbg commented Nov 4, 2018

It would be great if the rust-analyzer would expose which operations are unsafe, e.g., calling an unsafe fn, dereferencing a pointer, creating a reference to a field of a packed struct, reading from an union, etc. So that Rust IDEs can underline/highlight these operations (e.g. because operations inside an unsafe block are not all always unsafe).

@Shnatsel
Copy link
Member

Shnatsel commented Nov 4, 2018

To clarify, this is needed because e.g. the Rust standard library often wraps an entire function in an unsafe block to indicate that everything in that function, even the safe calls, need to be correct for the entire function to be safe.

The alternative - i.e. wrapping each individual call to an unsafe fn into its own unsafe block - would reduce readability, and the signalling that "all of this needs to be correct for the function to be safe" would be lost.

It is desirable to be able to tell apart safe and unsafe operations inside an unsafe block because it would simplify auditing. If unsafe calls are highlighted in some way, one could easily make a list of all the unsafe calls in the block, check their documentation to see what invariants they require to be upheld, and then verify that the rest of the block (i.e. the safe calls) uphold those invariants.

Without this it's easy to miss some unsafe call that actually requires some invariants to be upheld, and thus miss a security vulnerability.

@aochagavia
Copy link
Contributor

aochagavia commented Nov 4, 2018

Any ideas on how to visualize this? I can think of showing the names of unsafe functions in a different color (e.g. red) or underlining the unsafe expression with yellow.

@gnzlbg
Copy link
Author

gnzlbg commented Nov 4, 2018

I'd suggest consistently using the same styling for all unsafe operations and to not use this styling for anything else. All styling I can think of works for all unsafe operations (bold, underline, special fg/bg colors,...) so what to choose is up to whoever implements this. These are important things, so they should stand out and be readable, ideally without making the reader blind.

@matklad
Copy link
Member

matklad commented Nov 5, 2018

We definitely should do this, but it is definitely pretty far in the future: to highlight unsafe fn calls, we must resolve method invocation to the corresponding method definition, and that basically means implementing all of rust except for lifetimes. I've filed intellij-rust/intellij-rust#3013 for IntelliJ, which already has all the relevant infra in place for this feature.

@jonhoo
Copy link
Contributor

jonhoo commented Nov 1, 2019

Possibly a relevant RFC: rust-lang/rfcs#2585

@matklad matklad added E-hard fun A technically challenging issue with high impact labels Dec 15, 2019
@matklad
Copy link
Member

matklad commented Dec 15, 2019

We definitely should do this, but it is definitely pretty far in the future:

I think the far future has come? It seems like we have required infrastructure to implement this.

The solution consists of two big parts:

  • presenting unsafe operations to the user. This should be handled by highlighting module
  • finding out which operation are actually unsafe.

The second part should be done on hir (as it has semantic implications: we should flag unnecessary/missing unsafe blocks once analysis has no false positives). I think we should add

fn unsafe_expressions(db: &impl HIrdatabase, def: DefWithBody) -> Vec<ExprId>

function to hir/expr. It should work roughtly like ExprValidator: infer all types, then walk the tree and collect all unsafe expressions.

@matklad matklad added the E-has-instructions Issue has some instructions and pointers to code to get started label Dec 15, 2019
@matklad
Copy link
Member

matklad commented Mar 6, 2020

For the first part, we now can just slap TokenModifier::Unsafe onto unsafe ops.

@Nashenas88
Copy link
Contributor

I'm going to try taking this one on.

bors bot added a commit that referenced this issue Jun 2, 2020
4720: Add highlight support for unsafe fn calls and raw ptr deref r=matklad a=Nashenas88

Addresses ide highlighting for #190

Co-authored-by: Paul Daniel Faria <Nashenas88@users.noreply.github.com>
bors bot added a commit that referenced this issue Jun 27, 2020
4587: Add "missing unsafe" diagnostics r=matklad a=Nashenas88

Addresses #190 

Co-authored-by: Paul Daniel Faria <Nashenas88@users.noreply.github.com>
bors bot added a commit that referenced this issue Jun 27, 2020
4587: Add "missing unsafe" diagnostics r=Nashenas88 a=Nashenas88

Addresses #190 

Co-authored-by: Paul Daniel Faria <Nashenas88@users.noreply.github.com>
@matklad
Copy link
Member

matklad commented Jul 9, 2020

I think this is mostly done at the moment, set *.unsafe color in the color theme to highlight the ops.

@matklad matklad closed this as completed Jul 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-hard E-has-instructions Issue has some instructions and pointers to code to get started fun A technically challenging issue with high impact
Projects
None yet
Development

No branches or pull requests

6 participants