Fix some aliasing issues in Vec #70558

RalfJung · 2020-03-30T09:40:21Z

Vec::extend and Vec::truncate invalidated references into the vector even without reallocation, because they (implicitly) created a mutable reference covering the entire initialized part of the vector.

Fixes #70301
I verified the fix by adding some new tests here that I ran in Miri.

rust-highfive · 2020-03-30T09:40:25Z

r? @shepmaster

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

RalfJung · 2020-03-30T09:43:53Z

src/liballoc/vec.rs

            self.set_len(len + slice.len());
-            self.get_unchecked_mut(len..).copy_from_slice(slice);


Unfortunately there is no good way to grep for all places where the implicit DerefMut coercion is used, like here. I grepped for get_unchecked_mut and get_unchecked and got rid of those.

You could maybe print the backtrace in the DerefMut implementation and then run the tests and it will print what functions call DerefMut?(locally obviously hehe)

Since DerefMut is called for every mutable indexing operation (&mut v[i]), I expect that to show tons of stacktraces and not be very useful.

…rator

also add smoke test to detect relocation even in rustc runs

Centril · 2020-03-30T11:26:04Z

@bors try @rust-timer queue

rust-timer · 2020-03-30T11:26:05Z

Awaiting bors try build completion

bors · 2020-03-30T11:26:14Z

⌛ Trying commit 4eacf45 with merge fd566c8d8da22c80354a2256fdce4fa836e1f906...

RalfJung · 2020-03-30T11:45:08Z

Let's re-try that with the latest changes. I think now I have all &mut self methods that are not expected to change the capacity covered in the test (and found and fixed issues in remove and swap_remove).

@bors try @rust-timer queue

rust-timer · 2020-03-30T11:45:09Z

Awaiting bors try build completion

bors · 2020-03-30T11:45:19Z

⌛ Trying commit 5bbaac3 with merge c5fdda00c8e50fa1bacea8d98dd211fcac521372...

bors · 2020-03-30T14:21:49Z

☀️ Try build successful - checks-azure
Build commit: c5fdda00c8e50fa1bacea8d98dd211fcac521372 (c5fdda00c8e50fa1bacea8d98dd211fcac521372)

rust-timer · 2020-03-30T14:21:51Z

Queued c5fdda00c8e50fa1bacea8d98dd211fcac521372 with parent 8926bb4, future comparison URL.

llogiq · 2020-03-30T18:41:23Z

Would a lint for implicit deref(_mut) help?

rust-timer · 2020-03-30T19:30:45Z

Finished benchmarking try commit c5fdda00c8e50fa1bacea8d98dd211fcac521372, comparison URL.

RalfJung · 2020-03-31T09:52:51Z

Perf looks green, except for "syn-opt" where some runs get faster by 3% and some slower by 2% -- but they have a ? so I suppose those are pretty noisy anyway.

@llogiq hm, good idea. I am not sure, there might be tons of harmless implicit derefs in that file. The problematic ones are the ones for Vec itself.

But it might be worth a shot.

RalfJung · 2020-04-01T16:54:52Z

src/liballoc/vec.rs

@@ -962,13 +963,15 @@ impl<T> Vec<T> {
    #[inline]
    #[stable(feature = "rust1", since = "1.0.0")]
    pub fn swap_remove(&mut self, index: usize) -> T {
+        let len = self.len();
+        assert!(index < len, "index out of bounds: the len is {} but the index is {}", len, index);


Using formatting in the error seems to cause performance issues; let me know if you'd prefer an unformatted message.

Amanieu · 2020-04-04T20:16:41Z

The changes look fine, and there doesn't seem to be any perf impact.

@bors r+

bors · 2020-04-04T20:16:42Z

📌 Commit 3e2ac9c000cefd624d0324f7ffb16457d6246b72 has been approved by Amanieu

bors · 2020-04-04T20:16:43Z

🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened

bors · 2020-04-05T02:05:45Z

⌛ Testing commit 3e2ac9c000cefd624d0324f7ffb16457d6246b72 with merge e094a03952477c60b97b8fc3535872714749bec2...

RalfJung · 2020-04-05T06:37:04Z

##[error]An error occurred while provisioning resources (Error Type: Disconnect).

@bors retry

RalfJung · 2020-04-05T06:38:49Z

The changes look fine, and there doesn't seem to be any perf impact.

That perf measurement was from before I added the panic message.
I'll make this an assertion now and once #70573 lands, make swap_remove match remove.

RalfJung · 2020-04-05T06:41:27Z

r? @Amanieu @bors r=Amanieu

bors · 2020-04-05T06:41:28Z

📌 Commit 7e81c11 has been approved by Amanieu

@ghost

Rollup of 5 pull requests Successful merges: - rust-lang#70558 (Fix some aliasing issues in Vec) - rust-lang#70760 (docs: make the description of Result::map_or more clear) - rust-lang#70769 (Miri: remove an outdated FIXME) - rust-lang#70776 (clarify comment in RawVec::into_box) - rust-lang#70806 (fix Miri assignment sanity check) Failed merges: r? @ghost

rust-highfive assigned shepmaster Mar 30, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 30, 2020

RalfJung commented Mar 30, 2020

View reviewed changes

RalfJung mentioned this pull request Mar 30, 2020

String::push_str invalidates interior references even when it does not reallocate #70301

Closed

RalfJung added 4 commits March 30, 2020 11:58

fix Vec::extend invalidating unrelated pointers

6556549

fix pointer invalidation when extnding a vector from an untrusted ite…

86c1c43

…rator

fix ptr invalidation in Vec::truncate

fa6c883

add some tests

4393923

RalfJung force-pushed the vec-extend-aliasing branch 2 times, most recently from a5aea87 to d959640 Compare March 30, 2020 10:10

test more mutating vector methods

3411ade

RalfJung force-pushed the vec-extend-aliasing branch from d959640 to 3411ade Compare March 30, 2020 10:24

RalfJung added 2 commits March 30, 2020 13:01

also cover next() path of draining iterators

4eacf45

fix aliasing in remove()

8f479e3

also add smoke test to detect relocation even in rustc runs

RalfJung force-pushed the vec-extend-aliasing branch from f853bca to deaa348 Compare March 30, 2020 11:33

fix and test aliasing in swap_remove

5bbaac3

RalfJung force-pushed the vec-extend-aliasing branch from deaa348 to 5bbaac3 Compare March 30, 2020 11:34

RalfJung mentioned this pull request Mar 30, 2020

Improve panic messages for Vec methods #70524

Closed

RalfJung commented Apr 1, 2020

View reviewed changes

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 Apr 4, 2020

This comment has been minimized.

Sign in to view

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

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 Apr 5, 2020

tweak swap_remove

7e81c11

RalfJung force-pushed the vec-extend-aliasing branch from 3e2ac9c to 7e81c11 Compare April 5, 2020 06:41

rust-highfive assigned Amanieu and unassigned shepmaster Apr 5, 2020

Dylan-DPC-zz mentioned this pull request Apr 5, 2020

Rollup of 5 pull requests #70807

Merged

bors merged commit 7e4ed72 into rust-lang:master Apr 5, 2020

RalfJung deleted the vec-extend-aliasing branch April 5, 2020 16:29

RalfJung mentioned this pull request Apr 15, 2020

Vec drop and truncate: drop using raw slice *mut [T] #71148

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix some aliasing issues in Vec #70558

Fix some aliasing issues in Vec #70558

RalfJung commented Mar 30, 2020 •

edited

Loading

rust-highfive commented Mar 30, 2020

RalfJung Mar 30, 2020 •

edited

Loading

elichai Apr 2, 2020 •

edited

Loading

RalfJung Apr 2, 2020

Centril commented Mar 30, 2020

rust-timer commented Mar 30, 2020

bors commented Mar 30, 2020

RalfJung commented Mar 30, 2020

rust-timer commented Mar 30, 2020

bors commented Mar 30, 2020

bors commented Mar 30, 2020

rust-timer commented Mar 30, 2020

llogiq commented Mar 30, 2020

rust-timer commented Mar 30, 2020

RalfJung commented Mar 31, 2020 •

edited

Loading

RalfJung Apr 1, 2020

Amanieu commented Apr 4, 2020

bors commented Apr 4, 2020

bors commented Apr 4, 2020

bors commented Apr 5, 2020

This comment has been minimized.

RalfJung commented Apr 5, 2020

RalfJung commented Apr 5, 2020

RalfJung commented Apr 5, 2020

bors commented Apr 5, 2020

		self.set_len(len + slice.len());
		self.get_unchecked_mut(len..).copy_from_slice(slice);

Fix some aliasing issues in Vec #70558

Fix some aliasing issues in Vec #70558

Conversation

RalfJung commented Mar 30, 2020 • edited Loading

rust-highfive commented Mar 30, 2020

RalfJung Mar 30, 2020 • edited Loading

Choose a reason for hiding this comment

elichai Apr 2, 2020 • edited Loading

Choose a reason for hiding this comment

RalfJung Apr 2, 2020

Choose a reason for hiding this comment

Centril commented Mar 30, 2020

rust-timer commented Mar 30, 2020

bors commented Mar 30, 2020

RalfJung commented Mar 30, 2020

rust-timer commented Mar 30, 2020

bors commented Mar 30, 2020

bors commented Mar 30, 2020

rust-timer commented Mar 30, 2020

llogiq commented Mar 30, 2020

rust-timer commented Mar 30, 2020

RalfJung commented Mar 31, 2020 • edited Loading

RalfJung Apr 1, 2020

Choose a reason for hiding this comment

Amanieu commented Apr 4, 2020

bors commented Apr 4, 2020

bors commented Apr 4, 2020

bors commented Apr 5, 2020

This comment has been minimized.

RalfJung commented Apr 5, 2020

RalfJung commented Apr 5, 2020

RalfJung commented Apr 5, 2020

bors commented Apr 5, 2020

RalfJung commented Mar 30, 2020 •

edited

Loading

RalfJung Mar 30, 2020 •

edited

Loading

elichai Apr 2, 2020 •

edited

Loading

RalfJung commented Mar 31, 2020 •

edited

Loading