Lint against getting pointers from immediately dropped temporaries by GrigorenkoPV · Pull Request #128985 · rust-lang/rust

GrigorenkoPV · 2024-08-11T19:36:14Z

Fixes#123613

Changes:

New lint: dangling_pointers_from_temporaries. Is a generalization of temporary_cstring_as_ptr for more types and more ways to get a temporary.
temporary_cstring_as_ptr is removed and marked as renamed to dangling_pointers_from_temporaries.
clippy::temporary_cstring_as_ptr is marked as renamed to dangling_pointers_from_temporaries.
Fixed a false positive¹ for when the pointer is not actually dangling because of lifetime extension for function/method call arguments.
core::cell::Cell is now rustc_diagnostic_item = "Cell"

Questions:

Instead of manually checking for a list of known methods and diagnostic items, maybe add some sort of annotation to those methods in library and check for the presence of that annotation? Lint against getting pointers from immediately dropped temporaries #128985 (comment)

Known limitations:

False negatives²:

See the comments in compiler/rustc_lint/src/dangling.rs

Method calls that are not checked for:
- temporary_unsafe_cell.get()
- temporary_sync_unsafe_cell.get()
Ways to get a temporary that are not recognized:
- owning_temporary.field
- owning_temporary[index]
No checks for ref-to-ptr conversions:
- &raw [mut] temporary
- &temporary as *(const|mut) _
- ptr::from_ref(&temporary) and friends

lint should not be emitted, but is↩
lint should be emitted, but is not↩

rustbot · 2024-08-11T19:36:23Z

r? @wesleywiser

rustbot has assigned @wesleywiser.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

jieyouxu · 2024-08-12T00:50:50Z

FYI this probably needs T-lang approval and a crater run

GrigorenkoPV · 2024-08-12T21:33:09Z

This probably needs a crater run. Check-only should suffice. However, the lint should probably be denied for the run, but I am not sure if crater has such an option. Maybe I will just temporary change the default level in code.

Anyways, neither do I have permissions for bors try, nor for a crater run.

Urgau · 2024-08-19T17:34:24Z

+
+const MAX_PATH: usize = 260;
+fn main() {
+ let str1 = String::with_capacity(MAX_PATH).as_mut_ptr();


I wonder if it would make sense to suggest introducing a second binding in those simple let cases, aka.
let str1 = String::with_capacity(MAX_PATH);let str1 = str1.as_mut_ptr();

Probably yes, but I would prefer to do this as a separate follow-up PR if possible

I wonder if it would make sense to suggest introducing a second binding in those simple let cases, aka.
let str1 = String::with_capacity(MAX_PATH);let str1 = str1.as_mut_ptr();
After looking at the "regressed" code from crater, I can say that a lot of the time this suggestion will not fix the problem, and will just result in code like
let vec = some_iter.collect();let ptr = vec.as_ptr();return ptr;
or similar where the pointer still dangles eventually, even though we binded the temporary for now.
Sometimes it would make sense to suggest things like .into_raw_parts() instead of .as_ptr(), but then the user has to remember to do ::from_raw_parts() if they want to avoid leaking...
Most of the places where this lint fires are not trivial to fix and actually require the person writing the code to be more cautious & knowledgeable about what they are doing, which can only be achieved through more experience of writing unsafe, as well as some punches from the compiler, miri, or sanitizers.
I think that not providing this suggestion in places where it would help is less bad than improperly providing it in places where it wouldn't help but would just further obscure and hide the issue.

I still feel like we should point users towards doing something, even if it's not a general solution, and not a a structured suggestion.
It's really a shame to tell users something but not how to fix it.
Opened #132283 to track this.

It's really a shame to tell users something but not how to fix it.
It's even worse to suggest something that is no a fix. So if you are sure you have a correct suggestion, then sure, please make it -- but never do this unless you are sure. People will trust the suggestion and then be worse off than before.
EDIT: Sorry, I should have posted in the issue. Here we go.

rustbot · 2024-08-20T22:24:03Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

GrigorenkoPV · 2024-08-21T13:13:35Z

Looks like this caught an actual mistake in miri tests. However, the test in question was added after this PR had been opened, so I will have to rebase now to be able to fix the test.

GrigorenkoPV · 2024-08-21T13:42:50Z

Rebased

rustbot · 2024-08-21T13:50:23Z

The Miri subtree was changed

cc @rust-lang/miri

RalfJung · 2024-08-21T14:21:53Z

Looks like this caught an actual mistake in miri tests. However, the test in question was added after this PR had been opened, so I will have to rebase now to be able to fix the test.

It did. That's a good demo for the lint being quite useful indeed. :)

rfcbot · 2024-10-26T16:43:47Z

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.

Urgau · 2024-10-27T11:33:19Z

+
+fn lint_expr(cx: &LateContext<'_>, expr: &Expr<'_>) {
+ if let ExprKind::MethodCall(method, receiver, _args, _span) = expr.kind
+ && matches!(method.ident.name, sym::as_ptr | sym::as_mut_ptr)


If you want to do it in this PR (otherwise in a follow-up), we could add an rustc attribute to those as_ptr/as_mut_ptr methods.
You can follow afcb09b and 2a930d3 (from a PR of mine) with check_applied_to_fn_or_method instead, and something similar to this to check the attribute
rust/compiler/rustc_lint/src/ptr_nulls.rs
Lines 49 to 50 in e454c45
&& letSome(def_id) = cx.typeck_results().type_dependent_def_id(e.hir_id)
&& cx.tcx.has_attr(def_id, sym::rustc_never_returns_null_ptr)

I would prefer to do this as a follow-up, just not to stall this one any longer

I've created #132281 to track the use of a rustc attr for this lint.
Feel free to assign it to your-self (@rustbot assign I think).

Urgau · 2024-10-28T19:12:18Z

+ .label_ptr = this pointer will immediately be invalid
+ .label_temporary = this `{$ty}` is deallocated at the end of the statement, bind it to a variable to extend its lifetime
+ .note = pointers do not have a lifetime; when calling `{$callee}` the `{$ty}` will be deallocated at the end of the statement because nothing is referencing it as far as the type system is concerned
+ .help = for more information, see <https://doc.rust-lang.org/reference/destructors.html>


It's too bad that we don't have place explaining what a "dangling pointer" is, we could have linked it here.
Opened #132286 about that.

Urgau · 2024-10-28T19:15:54Z

It's clear that some improvements could be made (#132281, #132283, #132286), but IMO these improvements do not block merging this PR, they are all pre-existing anyway.

Therefore, @bors r+

bors · 2024-10-28T19:15:56Z

📌 Commit c69894e has been approved by Urgau

It is now in the queue for this repository.

bors · 2024-10-29T00:24:10Z

⌛ Testing commit c69894e with merge a9d1762...

bors · 2024-10-29T03:29:20Z

☀️ Test successful - checks-actions
Approved by: Urgau
Pushing a9d1762 to master...

rust-timer · 2024-10-29T04:47:03Z

Finished benchmarking commit (a9d1762): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.2%	[0.1%, 0.2%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary 2.1%, secondary 4.0%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.1%	[1.3%, 3.4%]	3
Regressions ❌ (secondary)	4.0%	[2.3%, 5.6%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.1%	[1.3%, 3.4%]	3

Cycles

Results (secondary -3.0%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.0%	[-3.0%, -3.0%]	1
All ❌✅ (primary)	-	-	0