Add invalid null pointer usage lint. #6192

boxdot · 2020-10-18T16:28:07Z

I am not entirely sure about the lint name. On one hand I would like to use something short, on other hand it should be descriptive enough. Another name proposal is appreciated.
I checked another functions that might be candidates for this lint. There are NonNull::new_unchecked and drop_in_place. It might make sense to add the lint to the former case, however the suggestion is then slightly convoluted: NonNull::new_unchecked(NonNull::dangling().as_ptr()). I guess I could replace it by just NonNull::dangling(). The drop_in_place case seem to be not really applicable, since I can't imagine a situation where somebody drops a constant nullptr. Throughts?

fixes #1703
changelog: none

rust-highfive · 2020-10-18T16:28:10Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Manishearth (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Manishearth · 2020-10-25T16:23:39Z

I'm in the middle of a move and may cut back on reviewing for a few weeks. I'm going to reassign my PRs for now, though i'll still try to review smaller things.

r? @ebroto

ebroto

LGTM modulo nits and a test to add.

I am not entirely sure about the lint name

I think the name is fine, we could maybe be a bit more explicit with invalid_null_ptr_usage?

I checked another functions that might be candidates for this lint

I agree with both of your points. In addition, I think this lint may be relevant for:

std::slice::from_raw_parts_mut
Most of the functions in std::ptr module? See the safety section.

tests/ui/invalid_null_usage.rs

src/lintlist/mod.rs

clippy_lints/src/ptr.rs

giraffate · 2020-11-11T23:44:32Z

ping from triage @boxdot. There seem to be some fixes left to be done. Do you have any questions on how to proceed here?

boxdot · 2020-11-12T10:56:01Z

ping from triage @boxdot. There seem to be some fixes left to be done. Do you have any questions on how to proceed here?

Thank you for pinging. Will address the comments today evening.

boxdot · 2020-11-23T19:24:49Z

I hope it is ok to rebase on master during the review.

boxdot · 2020-11-23T19:27:42Z

clippy_lints/src/utils/paths.rs

 pub const PTR_EQ: [&str; 3] = ["core", "ptr", "eq"];
 pub const PTR_NULL: [&str; 3] = ["core", "ptr", "null"];
 pub const PTR_NULL_MUT: [&str; 3] = ["core", "ptr", "null_mut"];
+pub const PTR_READ: [&str; 3] = ["core", "ptr", "read"];


Seems like this is a perfect lint for #5393. Should we try to add diagnostics to the functions instead of hardcoding them here? I would need some hints how to do this though. :)

I think you are right. I'm not familiar with the process of adding diagnostic items, but looking at this commit it seems that just adding the attribute should be enough?

This should be done in the rust-lang/rust repo, and we can sync that change afterwards and finish the lint here. As a heads-up we may try to pin to a nightly soon in this repo, so the aforementioned sync process could change. I apologize in advance if that creates any problem, we still have to discover what the new way of working with the pinned nightly will look like :)

EDIT: GitHub inlined this comment in the review, but not the whole conversation. Take a look at the additional comment there. TL;DR this is not needed for merging.

FYI, there are other lints being currently implemented that will add paths (e.g. #6394)

Even though the right thing to do would be probably to turn those into diagnostic items, I will not make a blocker out of this, so it's fine if you leave the paths here. I understand it would make merging this way longer, and it's not a problem introduced in your PR.

Ideally, you can address #5393 after this one if you want :)

You were right about first merging this and then adding diagnostic items to the compiler. I got stuck in the latter step.

bors · 2020-11-24T23:54:50Z

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

ebroto · 2020-11-28T21:56:31Z

tests/ui/invalid_null_ptr_usage.fixed

+        std::ptr::copy::<usize>(std::ptr::null(), std::ptr::NonNull::dangling().as_ptr(), 0);
+        std::ptr::copy::<usize>(std::ptr::NonNull::dangling().as_ptr(), std::ptr::null_mut(), 0);
+
+        std::ptr::copy_nonoverlapping::<usize>(std::ptr::null(), std::ptr::NonNull::dangling().as_ptr(), 0);
+        std::ptr::copy_nonoverlapping::<usize>(std::ptr::NonNull::dangling().as_ptr(), std::ptr::null_mut(), 0);


These cases do not seem to be detected

Resolved in #7023

ebroto · 2020-11-28T21:57:24Z

tests/ui/invalid_null_usage.fixed

@@ -0,0 +1,6 @@
+// run-rustfix


I think this file can be removed now

ebroto · 2020-11-28T22:03:38Z

clippy_lints/src/utils/paths.rs

 pub const PTR_EQ: [&str; 3] = ["core", "ptr", "eq"];
 pub const PTR_NULL: [&str; 3] = ["core", "ptr", "null"];
 pub const PTR_NULL_MUT: [&str; 3] = ["core", "ptr", "null_mut"];
+pub const PTR_READ: [&str; 3] = ["core", "ptr", "read"];


I think you are right. I'm not familiar with the process of adding diagnostic items, but looking at this commit it seems that just adding the attribute should be enough?

This should be done in the rust-lang/rust repo, and we can sync that change afterwards and finish the lint here. As a heads-up we may try to pin to a nightly soon in this repo, so the aforementioned sync process could change. I apologize in advance if that creates any problem, we still have to discover what the new way of working with the pinned nightly will look like :)

EDIT: GitHub inlined this comment in the review, but not the whole conversation. Take a look at the additional comment there. TL;DR this is not needed for merging.

ebroto

Thanks for making this move forward!

bors · 2020-12-04T01:05:31Z

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

giraffate · 2020-12-18T14:37:50Z

ping from triage @boxdot. Could you have any update on this?

ebroto · 2021-01-03T00:05:57Z

According to the triage procedure, I should close this PR as there has been no activity and two weeks have passed since the last ping.

@boxdot do not hesitate to reopen this in case you plan to finish it.

Invalid null usage v2 This is continuation of #6192 after inactivity. I plan to move paths into the compiler as diagnostic items after this is merged. fixes #1703 changelog: none

HKalbasi · 2021-07-29T22:08:56Z

finished in #5831
@rustbot label -S-inactive-closed

rust-highfive assigned Manishearth Oct 18, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 18, 2020

rust-highfive assigned ebroto and unassigned Manishearth Oct 25, 2020

ebroto suggested changes Oct 26, 2020

View reviewed changes

tests/ui/invalid_null_usage.rs Outdated Show resolved Hide resolved

src/lintlist/mod.rs Outdated Show resolved Hide resolved

clippy_lints/src/ptr.rs Outdated Show resolved Hide resolved

ebroto added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Oct 26, 2020

boxdot force-pushed the invalid-null-usage branch from 550c114 to 302d171 Compare November 23, 2020 19:23

boxdot commented Nov 23, 2020

View reviewed changes

boxdot force-pushed the invalid-null-usage branch from f28c1fe to 791227c Compare November 23, 2020 19:33

boxdot requested a review from ebroto November 23, 2020 22:21

ebroto 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 from the author. (Use `@rustbot ready` to update this status) labels Nov 23, 2020

boxdot added 8 commits November 27, 2020 15:58

Add invalid null pointer usage lint.

446acc7

Check also for std::ptr::{null, null_mut}.

8dcc446

Rename lint to invalid_null_ptr_usage.

b0844cf

Add test for ptr::null_mut.

d76f8e5

Address review comments: remove dot (also in CMP_NULL).

8ef5a6d

Address review comment: extend lint on std::slice::from_raw_parts_mut

e15d0ce

Add remaining functions from std::ptr.

79ed1c7

Make clippy happy.

4c56ef0

boxdot force-pushed the invalid-null-usage branch from 791227c to 4c56ef0 Compare November 27, 2020 15:05

ebroto reviewed Nov 28, 2020

View reviewed changes

ebroto suggested changes Nov 28, 2020

View reviewed changes

ebroto added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Nov 28, 2020

ebroto closed this Jan 3, 2021

ebroto added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jan 3, 2021

boxdot mentioned this pull request Apr 3, 2021

Invalid null usage v2 #7023

Merged

rustbot removed the S-inactive-closed Status: Closed due to inactivity label Jul 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add invalid null pointer usage lint. #6192

Add invalid null pointer usage lint. #6192

boxdot commented Oct 18, 2020

rust-highfive commented Oct 18, 2020

Manishearth commented Oct 25, 2020

ebroto left a comment

giraffate commented Nov 11, 2020

boxdot commented Nov 12, 2020

boxdot commented Nov 23, 2020 •

edited

Loading

boxdot Nov 23, 2020

ebroto Nov 28, 2020 •

edited

Loading

ebroto Nov 29, 2020 •

edited

Loading

boxdot Apr 3, 2021

bors commented Nov 24, 2020

ebroto Nov 28, 2020

boxdot Apr 3, 2021

ebroto Nov 28, 2020

ebroto Nov 28, 2020 •

edited

Loading

ebroto left a comment

bors commented Dec 4, 2020

giraffate commented Dec 18, 2020

ebroto commented Jan 3, 2021

HKalbasi commented Jul 29, 2021

Add invalid null pointer usage lint. #6192

Add invalid null pointer usage lint. #6192

Conversation

boxdot commented Oct 18, 2020

rust-highfive commented Oct 18, 2020

Manishearth commented Oct 25, 2020

ebroto left a comment

Choose a reason for hiding this comment

giraffate commented Nov 11, 2020

boxdot commented Nov 12, 2020

boxdot commented Nov 23, 2020 • edited Loading

boxdot Nov 23, 2020

Choose a reason for hiding this comment

ebroto Nov 28, 2020 • edited Loading

Choose a reason for hiding this comment

ebroto Nov 29, 2020 • edited Loading

Choose a reason for hiding this comment

boxdot Apr 3, 2021

Choose a reason for hiding this comment

bors commented Nov 24, 2020

ebroto Nov 28, 2020

Choose a reason for hiding this comment

boxdot Apr 3, 2021

Choose a reason for hiding this comment

ebroto Nov 28, 2020

Choose a reason for hiding this comment

ebroto Nov 28, 2020 • edited Loading

Choose a reason for hiding this comment

ebroto left a comment

Choose a reason for hiding this comment

bors commented Dec 4, 2020

giraffate commented Dec 18, 2020

ebroto commented Jan 3, 2021

HKalbasi commented Jul 29, 2021

boxdot commented Nov 23, 2020 •

edited

Loading

ebroto Nov 28, 2020 •

edited

Loading

ebroto Nov 29, 2020 •

edited

Loading

ebroto Nov 28, 2020 •

edited

Loading