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

Add lint to detect dereferencing of NULL pointers #83856

Closed
RalfJung opened this issue Apr 4, 2021 · 9 comments · Fixed by #83948
Closed

Add lint to detect dereferencing of NULL pointers #83856

RalfJung opened this issue Apr 4, 2021 · 9 comments · Fixed by #83948
Assignees
Labels
A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Apr 4, 2021

I propose we add a lint that detects code like

*(0 as *const i32)
*ptr::null()
*ptr::null_mut()

Some people seem to think that this is okay in contexts like &*(0 as *const i32) or addr_of!(*(0 as *const i32)), but that is not the case -- * on a NULL pointer is UB even as a place expression.

To implement this, you can use the invalid_value lint as a template:

declare_lint_pass!(InvalidValue => [INVALID_VALUE]);

check_expr should check for Unary expressions with a Deref operator, and then check the operand to be either a cast of 0 to a pointer type, or a call to one of the null methods. To detect the methods, make them "diagnostic items"; the invalid_value lint does that e.g. for mem::zeroed so it can again serve as a template here.

@RalfJung RalfJung added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Apr 4, 2021
@RalfJung RalfJung changed the title Ad lint to detect dereferencing of NULL pointers Add lint to detect dereferencing of NULL pointers Apr 4, 2021
@RalfJung
Copy link
Member Author

RalfJung commented Apr 4, 2021

Ah, turns out clippy already has a lint for this: zero_ptr. So maybe we should just move that to rustc? Cc @rust-lang/clippy

@jonas-schievink jonas-schievink added A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Apr 4, 2021
@ABouttefeux
Copy link
Contributor

@rustbot claim

@RalfJung
Copy link
Member Author

RalfJung commented Apr 4, 2021

@ABouttefeux awesome. :) Let me know if you need any help.

@ABouttefeux
Copy link
Contributor

Thanks :)

@slightlyoutofphase
Copy link
Contributor

slightlyoutofphase commented Apr 4, 2021

@RalfJung

Did you mean something slightly different with your "okay in contexts like" examples, perhaps? Neither of those two actually compile as written, since they're just attempts to deference something that isn't even a pointer. You get:

error[E0614]: type `usize` cannot be dereferenced

in both cases.

@RalfJung
Copy link
Member Author

RalfJung commented Apr 4, 2021

oops I screwed up the casts, should be fixed now.

@Manishearth
Copy link
Member

Yeah, happy to have Clippy lints upstreamed to rust, let us know when it's done so we can deprecate it on our end

@ABouttefeux
Copy link
Contributor

@RalfJung for detecting a call like ptr::null() you said to use TyCtxt::is_diagnostic_item. However I do not know what to use for the symbol argument. There is compiler\rustc_span\src\symbol.rs that defines symbol but nothing looks like creating a null ptr. Should I add one what should it look like ?

@RalfJung
Copy link
Member Author

RalfJung commented Apr 5, 2021

You first need to add the diagnostic item attribute to the relevant function inside library, like so:

#[rustc_diagnostic_item = "mem_zeroed"]

This name must be globally unique, e.g. ptr_null and ptr_null_mut.

Then you add those names to the list in compiler\rustc_span\src\symbol.rs (please keep it alphabetically sorted), and then you can use them for is_diagnostic_item.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Apr 14, 2021
…alfJung

add lint deref_nullptr detecting when a null ptr is dereferenced

fixes rust-lang#83856
changelog: add lint that detect code like
```rust
unsafe {
      &*core::ptr::null::<i32>()
 };
unsafe {
     addr_of!(std::ptr::null::<i32>())
};
let x: i32 = unsafe {*core::ptr::null()};
let x: i32 = unsafe {*core::ptr::null_mut()};
unsafe {*(0 as *const i32)};
unsafe {*(core::ptr::null() as *const i32)};
```
```
warning: Dereferencing a null pointer causes undefined behavior
 --> src\main.rs:5:26
  |
5 |     let x: i32 = unsafe {*core::ptr::null()};
  |                          ^^^^^^^^^^^^^^^^^^
  |                          |
  |                          a null pointer is dereferenced
  |                          this code causes undefined behavior when executed
  |
  = note: `#[warn(deref_nullptr)]` on by default
```

Limitation:
It does not detect code like
```rust
const ZERO: usize = 0;
unsafe {*(ZERO as *const i32)};
```
or code where `0` is not directly a literal
@bors bors closed this as completed in 7537b20 Apr 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants