Add precondition checks to ptr::offset, ptr::add, ptr::sub #130251

saethlin · 2024-09-11T22:13:52Z

All of offset, add, and sub (currently) have the trivial preconditions that the offset in bytes must be <= isize::MAX, and the computation of the new address must not wrap. This adds precondition checks for these, and like in slice indexing, we use intrinsics directly to implement unsafe APIs that have explicit checks, because we get a clearer error message that mentions the misused API not an implementation detail.

Experimentation indicates these checks have 1-2% compile time overhead, due primarily to adding the checks for add.

A crater run (#130251 (comment)) indicates some people currently have buggy calls to ptr::offset that apply a negative offset to a null pointer, but the crater run does not hit the ptr::add or ptr::sub checks, which seems like an argument for cfg'ing out those checks on account of their overhead.

saethlin · 2024-09-12T04:04:34Z

@bors try @rust-timer queue

bors · 2024-09-12T04:05:45Z

⌛ Trying commit 557d607 with merge 243a55f...

…=<try> Add precondition checks to ptr::offset, ptr::add, ptr::sub r? `@ghost`

bors · 2024-09-12T05:49:11Z

☀️ Try build successful - checks-actions
Build commit: 243a55f (243a55fc1226811e67a58c1e10f51ac7bca5304a)

rust-timer · 2024-09-12T07:34:40Z

Finished benchmarking commit (243a55f): comparison URL.

Overall result: ❌ regressions - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.7%	[0.2%, 3.1%]	63
Regressions ❌ (secondary)	0.4%	[0.2%, 0.9%]	6
Improvements ✅ (primary)	-0.3%	[-0.4%, -0.2%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.6%	[-0.4%, 3.1%]	66

Max RSS (memory usage)

Results (primary -0.2%, secondary -0.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)	5.8%	[1.4%, 12.5%]	4
Regressions ❌ (secondary)	1.7%	[1.7%, 1.7%]	1
Improvements ✅ (primary)	-4.3%	[-10.7%, -1.5%]	6
Improvements ✅ (secondary)	-2.2%	[-2.2%, -2.2%]	1
All ❌✅ (primary)	-0.2%	[-10.7%, 12.5%]	10

Cycles

Results (primary 1.8%, 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)	1.8%	[1.0%, 3.7%]	14
Regressions ❌ (secondary)	2.4%	[2.4%, 2.4%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.8%	[1.0%, 3.7%]	14

Binary size

Results (primary 0.3%, secondary 0.2%)

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.6%	[0.0%, 5.6%]	84
Regressions ❌ (secondary)	0.2%	[0.0%, 2.4%]	46
Improvements ✅ (primary)	-0.5%	[-1.5%, -0.0%]	30
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[-1.5%, 5.6%]	114

Bootstrap: 756.901s -> 758.397s (0.20%)
Artifact size: 341.35 MiB -> 341.24 MiB (-0.03%)

saethlin · 2024-09-12T15:46:04Z

@bors try @rust-timer queue

bors · 2024-09-12T15:47:15Z

⌛ Trying commit f879fcb with merge 2dd09a2...

…=<try> Add precondition checks to ptr::offset, ptr::add, ptr::sub r? `@ghost`

bors · 2024-09-12T17:33:30Z

☀️ Try build successful - checks-actions
Build commit: 2dd09a2 (2dd09a27cf858a1f1afe04dcb20f0c908c6b2305)

rust-timer · 2024-09-12T18:48:26Z

Finished benchmarking commit (2dd09a2): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.6%	[0.2%, 2.5%]	56
Regressions ❌ (secondary)	2.9%	[0.6%, 5.2%]	2
Improvements ✅ (primary)	-0.2%	[-0.3%, -0.2%]	2
Improvements ✅ (secondary)	-0.2%	[-0.2%, -0.1%]	2
All ❌✅ (primary)	0.5%	[-0.3%, 2.5%]	58

Max RSS (memory usage)

Results (primary 2.7%, secondary -0.6%)

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)	5.7%	[2.9%, 8.1%]	6
Regressions ❌ (secondary)	1.9%	[1.9%, 1.9%]	1
Improvements ✅ (primary)	-3.5%	[-7.6%, -1.5%]	3
Improvements ✅ (secondary)	-1.9%	[-2.9%, -0.9%]	2
All ❌✅ (primary)	2.7%	[-7.6%, 8.1%]	9

Cycles

Results (primary 1.7%, 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)	1.7%	[1.2%, 3.0%]	10
Regressions ❌ (secondary)	4.1%	[4.1%, 4.1%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.2%	[-2.3%, -2.1%]	2
All ❌✅ (primary)	1.7%	[1.2%, 3.0%]	10

Binary size

Results (primary 0.4%, 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.6%	[0.0%, 6.3%]	86
Regressions ❌ (secondary)	0.1%	[0.0%, 2.3%]	38
Improvements ✅ (primary)	-0.5%	[-1.5%, -0.0%]	20
Improvements ✅ (secondary)	-0.1%	[-0.7%, -0.0%]	12
All ❌✅ (primary)	0.4%	[-1.5%, 6.3%]	106

Bootstrap: 758.82s -> 760.456s (0.22%)
Artifact size: 341.24 MiB -> 341.34 MiB (0.03%)

library/core/src/ptr/const_ptr.rs

...because the checks in offset found bugs in a crater run.

saethlin · 2024-10-07T15:37:34Z

Rebased and blessed the mir-opt tests.
@bors r=Amanieu

bors · 2024-10-07T15:37:38Z

📌 Commit 128ccc3 has been approved by Amanieu

It is now in the queue for this repository.

bors · 2024-10-07T15:41:56Z

⌛ Testing commit 128ccc3 with merge 02544e3...

…=Amanieu Add precondition checks to ptr::offset, ptr::add, ptr::sub All of `offset`, `add`, and `sub` (currently) have the trivial preconditions that the offset in bytes must be <= isize::MAX, and the computation of the new address must not wrap. This adds precondition checks for these, and like in slice indexing, we use intrinsics directly to implement unsafe APIs that have explicit checks, because we get a clearer error message that mentions the misused API not an implementation detail. Experimentation indicates these checks have 1-2% compile time overhead, due primarily to adding the checks for `add`. A crater run (rust-lang#130251 (comment)) indicates some people currently have buggy calls to `ptr::offset` that apply a negative offset to a null pointer, but the crater run does not hit the `ptr::add` or `ptr::sub` checks, which seems like an argument for cfg'ing out those checks on account of their overhead.

saethlin · 2024-10-07T22:31:00Z

Once more, with feeling
@bors r=Amanieu

bors · 2024-10-07T22:31:02Z

📌 Commit 8d562f6 has been approved by Amanieu

It is now in the queue for this repository.

saethlin · 2024-10-07T23:04:58Z

apu_pecengines_hal is on GitLab so there's no automatic backlink. So here's a manual one: https://gitlab.com/dns2utf8/apu_pcengines_hal/-/issues/1

bors · 2024-10-08T01:57:01Z

⌛ Testing commit 8d562f6 with merge b8495e5...

bors · 2024-10-08T04:36:58Z

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing b8495e5 to master...

rust-timer · 2024-10-08T08:05:36Z

Finished benchmarking commit (b8495e5): comparison URL.

Overall result: ❌✅ regressions and improvements - 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
Regressions ❌ (secondary)	0.4%	[0.3%, 0.5%]	2
Improvements ✅ (primary)	-3.2%	[-3.2%, -3.2%]	1
Improvements ✅ (secondary)	-0.2%	[-0.2%, -0.2%]	1
All ❌✅ (primary)	-3.2%	[-3.2%, -3.2%]	1

Max RSS (memory usage)

Results (primary -0.6%, secondary -2.6%)

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.7%	[1.8%, 7.5%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-3.9%	[-9.9%, -1.3%]	4
Improvements ✅ (secondary)	-2.6%	[-2.6%, -2.6%]	1
All ❌✅ (primary)	-0.6%	[-9.9%, 7.5%]	7

Cycles

Results (primary -0.9%)

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)	1.1%	[1.1%, 1.1%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.9%	[-3.2%, -0.5%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.9%	[-3.2%, 1.1%]	3

Binary size

Results (primary -0.2%)

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.3%	[0.0%, 0.8%]	8
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.4%	[-1.2%, -0.0%]	38
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.2%	[-1.2%, 0.8%]	46

Bootstrap: 775.103s -> 774.269s (-0.11%)
Artifact size: 329.57 MiB -> 329.45 MiB (-0.04%)

saethlin · 2024-10-08T13:32:52Z

@rustbot label: +perf-regression-triaged Results are net neutral and not significant enough to worry about. Alternatively, the slowdown is justified by finding bugs in real code.

Rust 1.83 introduces some additional out-of-bound checks [0], making it illegal to attempt to load at an out-of-bound access when trying to load/store values from/to register in rbpf's interpreter, and causing the program to panick even before we reach the safety checks from check_mem(). I understand we need to use wrapping_offset() rather than offset() in that case, which causes the operation itself (but not the resulting poitner) to be safe, and the checked to be deferred. See also the related GitHub issue [1]. [0] rust-lang/rust#130251 [1] #115 Reported-by: Ben Kimock <kimockb@gmail.com> Signed-off-by: Quentin Monnet <qmo@qmon.net>

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Sep 11, 2024

This comment has been minimized.

Sign in to view

saethlin force-pushed the ptr-offset-preconditions branch from fe99c6b to 557d607 Compare September 12, 2024 04:00

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 Sep 12, 2024

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 12, 2024

Auto merge of rust-lang#130251 - saethlin:ptr-offset-preconditions, r…

243a55f

…=<try> Add precondition checks to ptr::offset, ptr::add, ptr::sub r? `@ghost`

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Sep 12, 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 Sep 12, 2024

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 12, 2024

Auto merge of rust-lang#130251 - saethlin:ptr-offset-preconditions, r…

2dd09a2

…=<try> Add precondition checks to ptr::offset, ptr::add, ptr::sub r? `@ghost`

This comment has been minimized.

Sign in to view

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

saethlin mentioned this pull request Sep 18, 2024

ptr::add/sub: do not claim equivalence with offset(c as isize) #130229

Merged

RalfJung reviewed Sep 18, 2024

View reviewed changes

library/core/src/ptr/const_ptr.rs Show resolved Hide resolved

RalfJung reviewed Sep 18, 2024

View reviewed changes

library/core/src/ptr/const_ptr.rs Outdated Show resolved Hide resolved

RalfJung reviewed Sep 18, 2024

View reviewed changes

library/core/src/ptr/const_ptr.rs Show resolved Hide resolved

RalfJung reviewed Sep 18, 2024

View reviewed changes

library/core/src/ptr/const_ptr.rs Outdated Show resolved Hide resolved

saethlin added 3 commits October 7, 2024 11:12

Add precondition checks to ptr::offset, ptr::add, ptr::sub

6d246e4

cfg out checks in add and sub but not offset

9d5c961

...because the checks in offset found bugs in a crater run.

Bless mir-opt tests

128ccc3

saethlin force-pushed the ptr-offset-preconditions branch from ee9b057 to 128ccc3 Compare October 7, 2024 15:18

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 Oct 7, 2024

This comment has been minimized.

Sign in to view

Disable slice_iter mir-opt test in debug builds

8d562f6

This was referenced Oct 7, 2024

test_vm_stdw_add_overflow encounters UB, will fail in Rust 1.83 qmonnet/rbpf#115

Closed

arc::tests::zero_sized::clone encounters UB, will fail in Rust 1.83 pen-lang/pen#2519

Open

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 8, 2024

bors merged commit b8495e5 into rust-lang:master Oct 8, 2024
7 checks passed

rustbot added this to the 1.83.0 milestone Oct 8, 2024

saethlin deleted the ptr-offset-preconditions branch October 8, 2024 04:52

saethlin added the perf-regression-triaged The performance regression has been triaged. label Oct 8, 2024

qmonnet mentioned this pull request Oct 29, 2024

src/interpreter.rs: Use wrapping_offset() for load/store operations qmonnet/rbpf#117

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add precondition checks to ptr::offset, ptr::add, ptr::sub #130251

Add precondition checks to ptr::offset, ptr::add, ptr::sub #130251

saethlin commented Sep 11, 2024 •

edited

Loading

This comment has been minimized.

saethlin commented Sep 12, 2024

This comment has been minimized.

bors commented Sep 12, 2024

This comment has been minimized.

bors commented Sep 12, 2024

This comment has been minimized.

rust-timer commented Sep 12, 2024

saethlin commented Sep 12, 2024

This comment has been minimized.

bors commented Sep 12, 2024

This comment has been minimized.

bors commented Sep 12, 2024

This comment has been minimized.

rust-timer commented Sep 12, 2024

saethlin commented Oct 7, 2024

bors commented Oct 7, 2024

bors commented Oct 7, 2024

This comment has been minimized.

This comment has been minimized.

saethlin commented Oct 7, 2024

bors commented Oct 7, 2024

saethlin commented Oct 7, 2024

bors commented Oct 8, 2024

bors commented Oct 8, 2024

rust-timer commented Oct 8, 2024

saethlin commented Oct 8, 2024 •

edited

Loading

Add precondition checks to ptr::offset, ptr::add, ptr::sub #130251

Add precondition checks to ptr::offset, ptr::add, ptr::sub #130251

Conversation

saethlin commented Sep 11, 2024 • edited Loading

This comment has been minimized.

saethlin commented Sep 12, 2024

This comment has been minimized.

bors commented Sep 12, 2024

This comment has been minimized.

bors commented Sep 12, 2024

This comment has been minimized.

rust-timer commented Sep 12, 2024

Overall result: ❌ regressions - ACTION NEEDED

saethlin commented Sep 12, 2024

This comment has been minimized.

bors commented Sep 12, 2024

This comment has been minimized.

bors commented Sep 12, 2024

This comment has been minimized.

rust-timer commented Sep 12, 2024

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

saethlin commented Oct 7, 2024

bors commented Oct 7, 2024

bors commented Oct 7, 2024

This comment has been minimized.

This comment has been minimized.

saethlin commented Oct 7, 2024

bors commented Oct 7, 2024

saethlin commented Oct 7, 2024

bors commented Oct 8, 2024

bors commented Oct 8, 2024

rust-timer commented Oct 8, 2024

Overall result: ❌✅ regressions and improvements - please read the text below

saethlin commented Oct 8, 2024 • edited Loading

saethlin commented Sep 11, 2024 •

edited

Loading

saethlin commented Oct 8, 2024 •

edited

Loading