memrchr: Correct aligned offset computation by bluss · Pull Request #35969 · rust-lang/rust

bluss · 2016-08-24T17:01:59Z

The memrchr fallback did not compute the offset correctly. It was
intentioned to land on usize-aligned addresses but did not.
This was suspected to have resulted in a crash on ARMv7!

This bug affected non-linux platforms.

I think like this, if we have a slice with pointer ptr and length
len, we want to find the last usize-aligned offset in the slice.
The correct computation should be:

For example if ptr = 1 and len = 6, and size_of::<usize>() is 4:

[ x x x x x x ]
1 2 3 4 5 6
^-- last aligned address at offset 3 from the start.

The last aligned address is ptr + len - (ptr + len) % usize_size.

Compute offset from the start as:

offset = len - (ptr + len) % usize_size = 6 - (1 + 6) % 4 = 6 - 3 = 3.

I believe the function's return value was always correct previously, if
the platform supported unaligned addresses.

Fixes#35967

rust-highfive · 2016-08-24T17:02:04Z

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

The memrchr fallback did not compute the offset correctly. It was intentioned to land on usize-aligned addresses but did not. This was suspected to resulted in a crash on ARMv7 platform! This bug affected non-linux platforms. I think like this, if we have a slice with pointer `ptr` and length `len`, we want to find the last usize-aligned offset in the slice. The correct computation should be: For example if ptr = 1 and len = 6, and size_of::<usize>() is 4: [ x x x x x x ] 1 2 3 4 5 6 ^-- last aligned address at offset 3 from the start. The last aligned address is ptr + len - (ptr + len) % usize_size. Compute offset from the start as: offset = len - (ptr + len) % usize_size = 6 - (1 + 6) % 4 = 6 - 3 = 3. I believe the function's return value was always correct previously, if the platform supported unaligned addresses.

nagisa · 2016-08-24T17:54:48Z

@@ -209,7 +209,7 @@ mod fallback {
 let end_align = (ptr as usize + len) & (usize_bytes - 1);


Preexisting, but this uses size_of to calculate usize_bytes, but alignment of usize on a system is not size_of::<usize> but rather align_of::<usize>? Seems like a pessimisation?

It doesn't seem like a big deal. I prefer a separate PR to deal with this. Let's have the fix here be minimal, especially if we backport it.

nagisa · 2016-08-24T19:31:22Z

LGTM, but it took me some 10 mins in a distraction-ful environment to figure out the offset calculation. I think a bug like this would’ve been obvious with a plain conditional as I proposed above.

bluss · 2016-08-24T19:37:45Z

Thank you for contributing the concentration! I agree that the conditional is better that way, will amend.

This makes the critical calculation easier to understand.

bluss · 2016-08-24T20:15:41Z

@bors r=nagisa

bors · 2016-08-24T20:15:43Z

📌 Commit 8295c50 has been approved by nagisa

bors · 2016-08-26T09:15:43Z

⌛ Testing commit 8295c50 with merge 0f1f6ce...

bors · 2016-08-26T10:29:44Z

💔 Test failed - auto-win-gnu-32-opt-rustbuild

bluss · 2016-08-26T10:33:07Z

failures:
---- [incremental] incremental\ich_method_call_trait_scope.rs stdout ----
thread '[incremental] incremental\ich_method_call_trait_scope.rs' panicked at 'called `Result::unwrap()` on an `Err` value: Error { repr: Os { code: 3, message: "The system cannot find the path specified." } }', C:\bot\slave\auto-win-gnu-32-opt-rustbuild\build\src\libcore\result.rs:783
note: Run with `RUST_BACKTRACE=1` for a backtrace.
failures:
[incremental] incremental\ich_method_call_trait_scope.rs

sophiajt · 2016-08-26T19:17:24Z

@bors retry

bors · 2016-08-27T14:52:20Z

⌛ Testing commit 8295c50 with merge 1194695...

memrchr: Correct aligned offset computation The memrchr fallback did not compute the offset correctly. It was intentioned to land on usize-aligned addresses but did not. This was suspected to have resulted in a crash on ARMv7! This bug affected non-linux platforms. I think like this, if we have a slice with pointer `ptr` and length `len`, we want to find the last usize-aligned offset in the slice. The correct computation should be: For example if ptr = 1 and len = 6, and `size_of::<usize>()` is 4: ``` [ x x x x x x ] 1 2 3 4 5 6 ^-- last aligned address at offset 3 from the start. ``` The last aligned address is ptr + len - (ptr + len) % usize_size. Compute offset from the start as: offset = len - (ptr + len) % usize_size = 6 - (1 + 6) % 4 = 6 - 3 = 3. I believe the function's return value was always correct previously, if the platform supported unaligned addresses. Fixes#35967

bors · 2016-08-27T17:44:01Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-cargotest, auto-linux-64-cross-freebsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-no-mir, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-no-mir, auto-win-msvc-64-opt-rustbuild
Approved by: nagisa
Pushing 1194695 to master...

bluss · 2016-08-27T19:20:31Z

Nominating for beta: It's a crashing bug affecting std::io::stdout (common libstd feature) on a minority platform (discovered on ARMv7 on android). Neither x86, x86-64, nor cfg(linux) platforms are affected.

bluss · 2016-08-29T12:29:11Z

Fix was verified by #35967 (comment)

alexcrichton · 2016-09-12T22:20:54Z

Libs decided to accept for backport.

rust-highfive assigned aturon Aug 24, 2016

bluss force-pushed the memrchr-alignment branch from fed896d to d1ecee9Compare August 24, 2016 17:05

nagisa reviewed Aug 24, 2016
View reviewed changes

memrchr: Use a conditional instead of subtracting a complicated min
8295c50
This makes the critical calculation easier to understand.

bluss mentioned this pull request Aug 24, 2016
memrchr uses unaligned loads #35967
Closed

bors merged commit 8295c50 into rust-lang:masterAug 27, 2016

bluss deleted the memrchr-alignment branch August 27, 2016 18:47

bluss added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Aug 27, 2016

nikomatsakis added the T-libs-api [DEPRECATED; DO NOT USE] label Sep 1, 2016

alexcrichton added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Sep 12, 2016

alexcrichton mentioned this pull request Sep 13, 2016
Backport PRs to beta #36433
Merged

brson removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Sep 16, 2016

		@@ -209,7 +209,7 @@ mod fallback {
		let end_align = (ptr as usize + len) & (usize_bytes - 1);

Uh oh!

Uh oh!

Conversation

bluss commented Aug 24, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Aug 24, 2016

Uh oh!

nagisaAug 24, 2016

Choose a reason for hiding this comment

Uh oh!

blussAug 25, 2016

Choose a reason for hiding this comment

Uh oh!

nagisa commented Aug 24, 2016

Uh oh!

bluss commented Aug 24, 2016

Uh oh!

bluss commented Aug 24, 2016

Uh oh!

bors commented Aug 24, 2016

Uh oh!

bors commented Aug 26, 2016

Uh oh!

bors commented Aug 26, 2016

Uh oh!

bluss commented Aug 26, 2016

Uh oh!

sophiajt commented Aug 26, 2016

Uh oh!

bors commented Aug 27, 2016

Uh oh!

bors commented Aug 27, 2016

Uh oh!

bluss commented Aug 27, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bluss commented Aug 29, 2016

Uh oh!

alexcrichton commented Sep 12, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

bluss commented Aug 24, 2016•
edited
Loading

bluss commented Aug 27, 2016•
edited
Loading