Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix capacity comparison in reserve #44802

Merged
merged 2 commits into from
Sep 27, 2017
Merged

Conversation

sfackler
Copy link
Member

You can otherwise end up in a situation where you don't actually resize
but still call into handle_cap_increase which then corrupts head/tail.

Closes #44800

Not totally sure the right way to write a test for this - there are some debug asserts the old bad behavior will hit but we don't build the stdlib with debug assertions by default.

r? @gankro

You can otherwise end up in a situation where you don't actually resize
but still call into handle_cap_increase which then corrupts head/tail.

Closes rust-lang#44800
@Gankra
Copy link
Contributor

Gankra commented Sep 24, 2017

If the VecDeque tests are still in-module, you can assert on internal invariants in tests. iirc we do that a lot.

@alexcrichton
Copy link
Member

I also believe we test with debug assertions on CI

@sfackler
Copy link
Member Author

I added a run-pass-valgrind test, but unless I'm missing something I don't think we're actually running those in valgrind. compiletest needs a --valgrind-path to be passed to it and it doesn't look like anything actually does that.

@sfackler sfackler added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Sep 24, 2017
@Gankra
Copy link
Contributor

Gankra commented Sep 25, 2017

Oh, r=me on the actual fix.

y'all can figure out the actual test.

(I think it's fine to land without a test)

@sfackler
Copy link
Member Author

I've verified that the test used to fail when run in valgrind and now no longer does, and I don't think this should block on #44816.

@bors r=Gankro

@bors
Copy link
Contributor

bors commented Sep 25, 2017

📌 Commit 81bac74 has been approved by Gankro

@est31
Copy link
Member

est31 commented Sep 25, 2017

Alternatively, what about adding a test based on the example given by the reporter of #44800? It only depends on libstd, no? And most importantly, it has output you can compare.

@carols10cents carols10cents added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 25, 2017
@sfackler
Copy link
Member Author

It depends on the allocator getting messed up in a very specific way which is pretty fragile. Directly looking for out of bounds reads should be more reliable.

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Sep 26, 2017
@alexcrichton
Copy link
Member

@bors: p=1

fixing a segfault and also beta accepted

@alexcrichton alexcrichton added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Sep 26, 2017
@bors
Copy link
Contributor

bors commented Sep 27, 2017

⌛ Testing commit 81bac74 with merge f71b37b...

bors added a commit that referenced this pull request Sep 27, 2017
Fix capacity comparison in reserve

You can otherwise end up in a situation where you don't actually resize
but still call into handle_cap_increase which then corrupts head/tail.

Closes #44800

Not totally sure the right way to write a test for this - there are some debug asserts the old bad behavior will hit but we don't build the stdlib with debug assertions by default.

r? @gankro
@bors
Copy link
Contributor

bors commented Sep 27, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: Gankro
Pushing f71b37b to master...

@bors bors merged commit 81bac74 into rust-lang:master Sep 27, 2017
bors added a commit that referenced this pull request Sep 30, 2017
@sfackler sfackler deleted the vecdeque-oob branch September 30, 2017 03:51
@alexcrichton alexcrichton removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants