Insert null checks for pointer dereferences when debug assertions are enabled #134424

1c3t3a · 2024-12-17T13:05:40Z

Similar to how the alignment is already checked, this adds a check
for null pointer dereferences in debug mode. It is implemented similarly
to the alignment check as a MirPass.

This inserts checks in the same places as the CheckAlignment pass and additionally
also inserts checks for Borrows, so code like

let ptr: *const u32 = std::ptr::null();
let val: &u32 = unsafe { &*ptr };

will have a check inserted on dereference. This is done because null references
are UB. The alignment check doesn't cover these places, because in &(*ptr).field,
the exact requirement is that the final reference must be aligned. This is something to
consider further enhancements of the alignment check.

For now this is implemented as a separate MirPass, to make it easy to disable
this check if necessary.

This is related to a 2025H1 project goal for better UB checks in debug
mode: rust-lang/rust-project-goals#177.

r? @saethlin

rustbot · 2024-12-17T13:05:51Z

This PR changes Stable MIR

cc @oli-obk, @celinval, @ouz-a

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @davidtwco, @celinval, @vakaras

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.

cc @rust-lang/rust-analyzer

oli-obk · 2024-12-17T13:06:44Z

@bors try @rust-timer queue

Insert null checks for pointer dereferences when debug assertions are enabled Similar to how the alignment is already checked, this adds a check for null pointer dereferences in debug mode. It is implemented similarly to the alignment check as a MirPass. This is related to a 2025H1 project goal for better UB checks in debug mode: rust-lang/rust-project-goals#177. r? `@saethlin`

bors · 2024-12-17T13:07:59Z

⌛ Trying commit 52b1360 with merge 61e98dc...

bors · 2024-12-17T14:52:30Z

☀️ Try build successful - checks-actions
Build commit: 61e98dc (61e98dc17f0786f1c120ea5366e1680772b3aa14)

compiler/rustc_mir_transform/src/check_pointers.rs

compiler/rustc_mir_transform/src/check_null.rs

rust-timer · 2024-12-17T18:11:10Z

Finished benchmarking commit (61e98dc): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary 3.0%, secondary 3.8%)

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)	3.0%	[2.6%, 3.5%]	2
Regressions ❌ (secondary)	3.8%	[2.4%, 5.1%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.0%	[2.6%, 3.5%]	2

Cycles

Results (secondary -3.4%)

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.4%	[-3.4%, -3.4%]	1
All ❌✅ (primary)	-	-	0

Binary size

Results (primary -0.1%, secondary -0.1%)

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.1%	[0.0%, 0.3%]	10
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.2%	[-0.6%, -0.1%]	9
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.1%]	1
All ❌✅ (primary)	-0.1%	[-0.6%, 0.3%]	19

Bootstrap: 768.397s -> 772.491s (0.53%)
Artifact size: 330.36 MiB -> 330.37 MiB (0.00%)

compiler/rustc_mir_transform/src/check_null.rs

saethlin · 2024-12-21T01:57:16Z

For PRs like this that change codegen, x test ui is almost always better to work off than trying to look at CI, because running the whole UI test suite will exercise your change on a lot of small programs, so if something in your change is broken it's usually pretty easy to narrow it down.

CI always tests using a stage2 build, so if you break codegen, you'll often get a CI failure while using your new compiler to build itself, which is hard to debug from.

compiler/rustc_mir_transform/src/check_pointers.rs

RalfJung · 2024-12-21T07:27:28Z

Is there a specific reason that this is a separate MIR pass from the null ptr check? Together they form the pointer validity checks, so I don't quite see why those would be checked separately.

1c3t3a · 2024-12-27T15:23:02Z

Is there a specific reason that this is a separate MIR pass from the null ptr check? Together they form the pointer validity checks, so I don't quite see why those would be checked separately.

My reasoning here was that this is two separate checks that people maybe want to enable or disable separately (lets say they only like to pay the overhead for alignment and don't care about null). My thinking was that two separate MIR passes solve this problem the most straightforward way, but I am happy to discuss this.

1c3t3a · 2025-01-31T11:14:45Z

Should be fixed!

saethlin · 2025-01-31T13:50:13Z

@bors r=saethlin

bors · 2025-01-31T13:50:16Z

📌 Commit b151b51 has been approved by saethlin

It is now in the queue for this repository.

1c3t3a · 2025-01-31T14:00:26Z

Ahh just noticed that I can see this crashing with #135994! I can see how this plays out and then rebase the one that was not merged?

saethlin · 2025-01-31T14:10:31Z

Yup. One of them has to land first. This should let you be a bit more efficient, though I'll still be watching:

@bors delegate=1c3t3a

bors · 2025-01-31T14:10:35Z

✌️ @1c3t3a, you can now approve this pull request!

If @saethlin told you to "r=me" after making some further change, please make that change, then do @bors r=@saethlin

bors · 2025-01-31T15:56:56Z

⌛ Testing commit b151b51 with merge aa4cfd0...

bors · 2025-01-31T20:15:39Z

☀️ Test successful - checks-actions
Approved by: saethlin
Pushing aa4cfd0 to master...

The wording unsafe pointer is less common and not mentioned in a lot of places, instead this is usually called a "raw pointer". For the sake of uniformity, we rename this method. This came up during the review of rust-lang#134424.

rust-timer · 2025-02-01T02:10:53Z

Finished benchmarking commit (aa4cfd0): comparison URL.

Overall result: ❌ regressions - please read the text below

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

If the regression was expected or you think it can be justified,
please write a comment with sufficient written justification, and add
@rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
If you think that you know of a way to resolve the regression, try to create
a new PR with a fix for the regression.
If you do not understand the regression or you think that it is just noise,
you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
were already notified of this PR).

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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.3%	[0.2%, 0.5%]	7
Regressions ❌ (secondary)	0.3%	[0.2%, 0.5%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[0.2%, 0.5%]	7

Max RSS (memory usage)

Results (primary -1.3%)

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)	3.9%	[2.2%, 5.1%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-4.5%	[-8.4%, -2.2%]	5
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.3%	[-8.4%, 5.1%]	8

Cycles

Results (primary 2.7%, secondary 2.4%)

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.7%	[0.7%, 4.8%]	2
Regressions ❌ (secondary)	2.4%	[2.0%, 2.9%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.7%	[0.7%, 4.8%]	2

Binary size

Results (primary 0.1%)

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.2%	[0.0%, 0.5%]	43
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.4%	[-0.6%, -0.1%]	4
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[-0.6%, 0.5%]	47

Bootstrap: 777.68s -> 777.108s (-0.07%)
Artifact size: 328.84 MiB -> 328.80 MiB (-0.01%)

saethlin · 2025-02-01T02:19:23Z

@rustbot label: +perf-regression-triaged

Slight regression is expected, we're generating more MIR to insert new checks. We have evidence from a crater run that these checks find UB in the wild, which justifies the very small compile time slowdown.

Insert null checks for pointer dereferences when debug assertions are enabled Similar to how the alignment is already checked, this adds a check for null pointer dereferences in debug mode. It is implemented similarly to the alignment check as a `MirPass`. This inserts checks in the same places as the `CheckAlignment` pass and additionally also inserts checks for `Borrows`, so code like ```rust let ptr: *const u32 = std::ptr::null(); let val: &u32 = unsafe { &*ptr }; ``` will have a check inserted on dereference. This is done because null references are UB. The alignment check doesn't cover these places, because in `&(*ptr).field`, the exact requirement is that the final reference must be aligned. This is something to consider further enhancements of the alignment check. For now this is implemented as a separate `MirPass`, to make it easy to disable this check if necessary. This is related to a 2025H1 project goal for better UB checks in debug mode: rust-lang/rust-project-goals#177. r? `@saethlin`

The wording unsafe pointer is less common and not mentioned in a lot of places, instead this is usually called a "raw pointer". For the sake of uniformity, we rename this method. This came up during the review of rust-lang#134424.

- vtable changes: rust-lang/rust#135318 - NullPointerDereference changes: rust-lang/rust#134424

Upgrade toolchain to 2/10. I **highly recommend** reviewing this PR commit-by-commit. The description in each commit message links to the upstream PRs that prompted those particular changes. ## Callouts - 2/1 had a lot of formatting changes. I split the commits for that day into formatting changes and functionality changes accordingly. - 2/5 introduced a regression in our delayed UB instrumentation, so I made a new fixme test. See #3881 for details. ## Culprit PRs: rust-lang/rust#134424 rust-lang/rust#130514 rust-lang/rust#135748 rust-lang/rust#136590 rust-lang/rust#135318 rust-lang/rust#135265 rust-lang/rust@bcb8565 rust-lang/rust#136471 rust-lang/rust#136645 Resolves #3863 By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.

Rename rustc_middle::Ty::is_unsafe_ptr to is_raw_ptr The wording unsafe pointer is less common and not mentioned in a lot of places, instead this is usually called a "raw pointer". For the sake of uniformity, we rename this method. This came up during the review of rust-lang#134424. r? `@Noratrieb`

Rename rustc_middle::Ty::is_unsafe_ptr to is_raw_ptr The wording unsafe pointer is less common and not mentioned in a lot of places, instead this is usually called a "raw pointer". For the sake of uniformity, we rename this method. This came up during the review of rust-lang/rust#134424. r? `@Noratrieb`

rustbot assigned saethlin Dec 17, 2024

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. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 17, 2024

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 17, 2024

This comment has been minimized.

Sign in to view

1c3t3a force-pushed the null-checks branch from 52b1360 to 8b8ffa1 Compare December 17, 2024 16:37

This comment has been minimized.

Sign in to view

RalfJung reviewed Dec 17, 2024

View reviewed changes

compiler/rustc_mir_transform/src/check_pointers.rs Show resolved Hide resolved

RalfJung reviewed Dec 17, 2024

View reviewed changes

compiler/rustc_mir_transform/src/check_null.rs Outdated Show resolved Hide resolved

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 17, 2024

This comment has been minimized.

Sign in to view

1c3t3a requested review from RalfJung and saethlin December 20, 2024 22:41

This comment has been minimized.

Sign in to view

saethlin reviewed Dec 21, 2024

View reviewed changes

compiler/rustc_mir_transform/src/check_null.rs Outdated Show resolved Hide resolved

saethlin reviewed Dec 21, 2024

View reviewed changes

compiler/rustc_mir_transform/src/check_null.rs Outdated Show resolved Hide resolved

RalfJung reviewed Dec 21, 2024

View reviewed changes

compiler/rustc_mir_transform/src/check_pointers.rs Show resolved Hide resolved

compiler/rustc_mir_transform/src/check_pointers.rs Outdated Show resolved Hide resolved

compiler/rustc_mir_transform/src/check_pointers.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 31, 2025

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 31, 2025

bors merged commit aa4cfd0 into rust-lang:master Jan 31, 2025
7 checks passed

rustbot added this to the 1.86.0 milestone Jan 31, 2025

rustbot added the perf-regression-triaged The performance regression has been triaged. label Feb 1, 2025

zjp-CN mentioned this pull request Feb 5, 2025

Null ptr check doesn't catch null reference to ZSTs #136568

Closed

Xenira mentioned this pull request Feb 6, 2025

Null pointer dereference on rust nightly davidcole1340/ext-php-rs#357

Closed

carolynzech added a commit to carolynzech/kani that referenced this pull request Feb 10, 2025

Update toolchain to 2/1 - functionality changes

d77bb0f

- vtable changes: rust-lang/rust#135318 - NullPointerDereference changes: rust-lang/rust#134424

carolynzech mentioned this pull request Feb 10, 2025

Upgrade toolchain to 2/10 model-checking/kani#3883

Merged

carolynzech added a commit to carolynzech/kani that referenced this pull request Feb 10, 2025

Update toolchain to 2/1 - functionality changes

cc35317

- vtable changes: rust-lang/rust#135318 - NullPointerDereference changes: rust-lang/rust#134424

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Insert null checks for pointer dereferences when debug assertions are enabled #134424

Insert null checks for pointer dereferences when debug assertions are enabled #134424

1c3t3a commented Dec 17, 2024 •

edited

Loading

rustbot commented Dec 17, 2024

oli-obk commented Dec 17, 2024

This comment has been minimized.

bors commented Dec 17, 2024

This comment has been minimized.

bors commented Dec 17, 2024

This comment has been minimized.

This comment has been minimized.

rust-timer commented Dec 17, 2024

This comment has been minimized.

This comment has been minimized.

saethlin commented Dec 21, 2024 •

edited

Loading

RalfJung commented Dec 21, 2024

1c3t3a commented Dec 27, 2024

This comment has been minimized.

1c3t3a commented Jan 31, 2025

saethlin commented Jan 31, 2025

bors commented Jan 31, 2025

1c3t3a commented Jan 31, 2025

saethlin commented Jan 31, 2025

bors commented Jan 31, 2025

bors commented Jan 31, 2025

bors commented Jan 31, 2025

rust-timer commented Feb 1, 2025

saethlin commented Feb 1, 2025

Insert null checks for pointer dereferences when debug assertions are enabled #134424

Insert null checks for pointer dereferences when debug assertions are enabled #134424

Conversation

1c3t3a commented Dec 17, 2024 • edited Loading

rustbot commented Dec 17, 2024

oli-obk commented Dec 17, 2024

This comment has been minimized.

bors commented Dec 17, 2024

This comment has been minimized.

bors commented Dec 17, 2024

This comment has been minimized.

This comment has been minimized.

rust-timer commented Dec 17, 2024

Overall result: no relevant changes - no action needed

This comment has been minimized.

This comment has been minimized.

saethlin commented Dec 21, 2024 • edited Loading

RalfJung commented Dec 21, 2024

1c3t3a commented Dec 27, 2024

This comment has been minimized.

1c3t3a commented Jan 31, 2025

saethlin commented Jan 31, 2025

bors commented Jan 31, 2025

1c3t3a commented Jan 31, 2025

saethlin commented Jan 31, 2025

bors commented Jan 31, 2025

bors commented Jan 31, 2025

bors commented Jan 31, 2025

rust-timer commented Feb 1, 2025

Overall result: ❌ regressions - please read the text below

saethlin commented Feb 1, 2025

1c3t3a commented Dec 17, 2024 •

edited

Loading

saethlin commented Dec 21, 2024 •

edited

Loading