-
Notifications
You must be signed in to change notification settings - Fork 13.6k
Add lint against dangling pointers from local variables #144322
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
base: master
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
The Miri subtree was changed cc @rust-lang/miri |
Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns. |
@rfcbot reviewed |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
I would like us as lang to get a a world where we approve intent of lints, then the details are up to the diagnostics team, and under that framework I think the indent of "catch when people return pointers that are fundamentally useless to the caller" is clearly a good one. |
53a1ff8
to
bec4fc5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is useful/general enough as it is, but I have nothing against it and the diagnostics look fine.
I think to generalize it we would have to write it as a lint that runs dataflow on MIR
// verify that we have a pointer type | ||
let inner_ty = match ty.kind() { | ||
ty::RawPtr(inner_ty, _) => *inner_ty, | ||
_ => return, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a bit overprecise on just pointer return types and will not catch even tuples that contain raw pointers or generic Adts like Option
. Ok as a first version, but I feel like it should be more general to be really useful.
bec4fc5
to
ecb89bc
Compare
I think this is an empirical question that could be answered with crater. We can always just search crater for the warning when this lands in beta. |
Agree. I wanted to have a first simple version, happy to try to switch the lint to run on the MIR dataflow framework after this one is merged. @rustbot ready |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, I think merging as is is fine, and it obviously never has to be perfect, but I'm curious how far we can take it.
r=me with FCP finished
ecb89bc
to
21ec0d5
Compare
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
@bors r=oli-obk |
…oli-obk Add lint against dangling pointers from local variables ## `dangling_pointers_from_locals` *warn-by-default* The `dangling_pointers_from_locals` lint detects getting a pointer to data of a local that will be dropped at the end of the function. ### Example ```rust fn f() -> *const u8 { let x = 0; &x // returns a dangling ptr to `x` } ``` ```text warning: a dangling pointer will be produced because the local variable `x` will be dropped --> $DIR/dangling-pointers-from-locals.rs:10:5 | LL | fn simple() -> *const u8 { | --------- return type of the function is `*const u8` LL | let x = 0; | - `x` is defined inside the function and will be drop at the end of the function LL | &x | ^^ | = note: pointers do not have a lifetime; after returning, the `u8` will be deallocated at the end of the function because nothing is referencing it as far as the type system is concerned = note: `#[warn(dangling_pointers_from_locals)]` on by default ``` ### Explanation Returning a pointer from a local value will not prolong its lifetime, which means that the value can be dropped and the allocation freed while the pointer still exists, making the pointer dangling. If you need stronger guarantees, consider using references instead, as they are statically verified by the borrow-checker to never dangle. ------ This is related to GitHub codeql [CWE-825](https://github.com/github/codeql/blob/main/rust/ql/src/queries/security/CWE-825/AccessAfterLifetimeBad.rs) which shows examples of such simple miss-use. It should be noted that C compilers warns against such patterns even without `-Wall`, https://godbolt.org/z/P7z98arrc. ------ `@rustbot` labels +I-lang-nominated +T-lang cc `@traviscross` r? compiler
Rollup of 14 pull requests Successful merges: - #143857 (Port #[macro_export] to the new attribute parsing infrastructure) - #143900 ([rustdoc] Correctly handle `should_panic` doctest attribute and fix `--no-run` test flag on the 2024 edition) - #144070 (Implement `hash_map` macro ) - #144322 (Add lint against dangling pointers from local variables) - #144443 (Make target pointer width in target json an integer) - #144667 (`AlignmentEnum` should just be `repr(usize)` now) - #144779 (Implement debugging output of the bootstrap Step graph into a DOT file) - #144790 (Multiple bounds checking elision failures) - #144794 (Port `#[coroutine]` to the new attribute system) - #144808 (`Interner` arg to `EarlyBinder` does not affect auto traits) - #144816 (Update E0562 to account for the new impl trait positions) - #144822 (Return a struct with named fields from `hash_owner_nodes`) - #144824 (Updated test links in compiler) - #144829 (Use full flag name in strip command for Darwin) r? `@ghost` `@rustbot` modify labels: rollup
…oli-obk Add lint against dangling pointers from local variables ## `dangling_pointers_from_locals` *warn-by-default* The `dangling_pointers_from_locals` lint detects getting a pointer to data of a local that will be dropped at the end of the function. ### Example ```rust fn f() -> *const u8 { let x = 0; &x // returns a dangling ptr to `x` } ``` ```text warning: a dangling pointer will be produced because the local variable `x` will be dropped --> $DIR/dangling-pointers-from-locals.rs:10:5 | LL | fn simple() -> *const u8 { | --------- return type of the function is `*const u8` LL | let x = 0; | - `x` is defined inside the function and will be drop at the end of the function LL | &x | ^^ | = note: pointers do not have a lifetime; after returning, the `u8` will be deallocated at the end of the function because nothing is referencing it as far as the type system is concerned = note: `#[warn(dangling_pointers_from_locals)]` on by default ``` ### Explanation Returning a pointer from a local value will not prolong its lifetime, which means that the value can be dropped and the allocation freed while the pointer still exists, making the pointer dangling. If you need stronger guarantees, consider using references instead, as they are statically verified by the borrow-checker to never dangle. ------ This is related to GitHub codeql [CWE-825](https://github.com/github/codeql/blob/main/rust/ql/src/queries/security/CWE-825/AccessAfterLifetimeBad.rs) which shows examples of such simple miss-use. It should be noted that C compilers warns against such patterns even without `-Wall`, https://godbolt.org/z/P7z98arrc. ------ ``@rustbot`` labels +I-lang-nominated +T-lang cc ``@traviscross`` r? compiler
Rollup of 13 pull requests Successful merges: - #143857 (Port #[macro_export] to the new attribute parsing infrastructure) - #143900 ([rustdoc] Correctly handle `should_panic` doctest attribute and fix `--no-run` test flag on the 2024 edition) - #144070 (Implement `hash_map` macro ) - #144322 (Add lint against dangling pointers from local variables) - #144443 (Make target pointer width in target json an integer) - #144667 (`AlignmentEnum` should just be `repr(usize)` now) - #144790 (Multiple bounds checking elision failures) - #144794 (Port `#[coroutine]` to the new attribute system) - #144808 (`Interner` arg to `EarlyBinder` does not affect auto traits) - #144816 (Update E0562 to account for the new impl trait positions) - #144822 (Return a struct with named fields from `hash_owner_nodes`) - #144824 (Updated test links in compiler) - #144829 (Use full flag name in strip command for Darwin) r? `@ghost` `@rustbot` modify labels: rollup
dangling_pointers_from_locals
warn-by-default
The
dangling_pointers_from_locals
lint detects getting a pointer to data of a local that will be dropped at the end of the function.Example
Explanation
Returning a pointer from a local value will not prolong its lifetime, which means that the value can be dropped and the allocation freed while the pointer still exists, making the pointer dangling.
If you need stronger guarantees, consider using references instead, as they are statically verified by the borrow-checker to never dangle.
This is related to GitHub codeql CWE-825 which shows examples of such simple miss-use.
It should be noted that C compilers warns against such patterns even without
-Wall
, https://godbolt.org/z/P7z98arrc.@rustbot labels +I-lang-nominated +T-lang
cc @traviscross
r? compiler