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

Uplift clippy::invalid_null_ptr_usage lint #119220

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented Dec 22, 2023

This PR aims at uplifting the clippy::invalid_null_ptr_usage lint into rustc, this is similar to the clippy::invalid_utf8_in_unchecked uplift a few months ago, in the sense that those two lints lint on invalid parameter(s), here a null pointer where it is unexpected and UB to pass one.

For context: GitHub Search reveals that just for slice::from_raw_parts{_mut} ~20 invalid usages with ptr::null and an additional 4 invalid usages with 0 as *const ...-ish casts.


invalid_null_ptr_usages

(deny-by-default)

The invalid_null_ptr_usages lint checks for invalid usage of null pointers.

Example

// Undefined behavior
unsafe { std::slice::from_raw_parts(ptr::null(), 0); }
// Not Undefined behavior
unsafe { std::slice::from_raw_parts(NonNull::dangling().as_ptr(), 0); }

Produces:

error: calling this function with a null pointer is undefined behavior, even if the result of the function is unused, consider using a dangling pointer instead
  --> $DIR/invalid_null_ptr_usages.rs:14:23
   |
LL |     let _: &[usize] = std::slice::from_raw_parts(ptr::null(), 0);
   |                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^-----------^^^^
   |                                                  |
   |                                                  help: use a dangling pointer instead: `core::ptr::NonNull::dangling().as_ptr()`

Explanation

Calling methods who's safety invariants requires non-null pointer with a null pointer is undefined behavior.


The lint use a list of functions to know which functions and arguments to checks, this could be improved in the future with a rustc attribute, or maybe even with a #[diagnostic] attribute.

This PR also includes some small refactoring to avoid some ambiguities in naming, those can be done in another PR is desired.

@rustbot label: +I-lang-nominated
r? compiler

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 22, 2023
@rustbot
Copy link
Collaborator

rustbot commented Dec 22, 2023

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@rustbot rustbot added the I-lang-nominated Nominated for discussion during a lang team meeting. label Dec 22, 2023
@rust-log-analyzer

This comment has been minimized.

@Urgau Urgau force-pushed the uplift-invalid_null_ptr_usage branch 2 times, most recently from b25c44c to 7308419 Compare December 22, 2023 16:07
@rust-log-analyzer

This comment has been minimized.

@Urgau Urgau force-pushed the uplift-invalid_null_ptr_usage branch from 7308419 to 05aabc1 Compare December 22, 2023 16:44
@Noratrieb
Copy link
Member

I think @saethlin has also found plenty of code that hits this in the wild, so this seems like a good lint.

@saethlin
Copy link
Member

You're probably referring to PyO3/pyo3#2687 and servo/font-kit#197

I don't think either of these would be detected by the lint, but I agree that this is a reasonable lint target.

@apiraino apiraino added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 1, 2024
@apiraino
Copy link
Contributor

apiraino commented Feb 1, 2024

I'll flag this as S-waiting-on-team to signal that T-lang nomination

@bors
Copy link
Contributor

bors commented Feb 13, 2024

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

@scottmcm
Copy link
Member

(Just me, with my lang hat on but not talking for the team.)

I wish there was an easier way for these to get added without waiting on us.

Would you be interested in making a proposal of some sort for making maybe a diagnostics::something_or_other that could be used on functions or parameters to let warnings like this be something that functions can opt into themselves.

It'd be great if, rather than lang needing to approve stuff, it'd just be T-libs PR approving a #[diagnostics::ub_if(ptr.is_null())] or something like that. And eventually we could stabilize that to let crates use them too, to get built-in lints on stuff without needing rustc or clippy needing to know that the random crates even need to know exist.

@Centri3
Copy link
Member

Centri3 commented Jun 26, 2024

That feels similar to what flux and verus are doing but at a smaller scale. Unsure about allowing downstream users to access it but it'd be great to make these kinds of checks more prevalent (and not just for null ptr cases)

@Centri3
Copy link
Member

Centri3 commented Jun 26, 2024

I do like refinement types. We should discuss this further, I love the idea of rust expanding its sights of correctness to this, rather than a devtool (what flux does atm)

@Urgau
Copy link
Member Author

Urgau commented Jun 28, 2024

I would if there was an easier way to add those kind of lints. Using the diagnostics namespace is also something I alluded in my top comment, and I think that it's something that should probably be pushed forward, the only thing that is a bit weird is that I had the impression that the diagnostics attribute would not by it-self create a diagnostic but is only here to accompany one.

It'd be great if, rather than lang needing to approve stuff, it'd just be T-libs PR approving a #[diagnostics::ub_if(ptr.is_null())] or something like that.

That would be awesome, I would love if it were this "simple".

I can write something akin to an MCP (for t-lang) but I don't have the time for a full RFC :-)

@jieyouxu
Copy link
Member

jieyouxu commented Aug 5, 2024

It'd be great if, rather than lang needing to approve stuff, it'd just be T-libs PR approving a #[diagnostics::ub_if(ptr.is_null())] or something like that. And eventually we could stabilize that to let crates use them too, to get built-in lints on stuff without needing rustc or clippy needing to know that the random crates even need to know exist.

Using the diagnostics namespace is also something I alluded in my top comment, and I think that it's something that should probably be pushed forward

Just a remark: note that there is currently a limitation on the compiler side with attribute handling of "namespaced attributes" and abitrary inner expressions within attributes.

In particular, we don't currently have support for arbitrary inner expressions (we can probably teach the compiler about arbitrary inner expressions but would need to be very careful about the grammar):

  • we do currently have support for arbitrary expressions in the form of #[name = expr]
  • but not #[name(expr)] AFAIK
  • we certainly do not currently have support for #[namespace::name(expr)] where expr is not just a literal or other primitive.

(Purely FYI, not intended to derail this PR, we should continue discussion for these ideas on a dedicated zulip thread.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-lang-nominated Nominated for discussion during a lang team meeting. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.