Moved value gets dropped if reassigned #42903
Comments
|
The |
|
Woh. I need to read about that tool, that's awesome! BTW, I tried to clarify in the bug report on the trust-dns repo; it's not that OpenSSL and ring are affected, it's that I reproduced the issue with both OpenSSL and/or ring dependencies in use. |
|
Thanks @bluejekyll ! I've changed my description to just reference your bug and not duplicate what your description used to be :) |
nagisa
added
I-wrong
regression-from-stable-to-nightly
|
Running the
Edit: Here are some valgrind logs, they look... worrisome https://gist.github.com/TimNN/039cefc6cfe601faa1d129398583fd39 |
|
If you want to take OpenSSL out of the picture, I believe the same issue can be triggered with openssl disabled: cargo test --no-default-features --features=ring |
|
Incident 1 mentionned in this bug description is in tantivy. I tried to trim down the unit test to something simpler, smaller that stills fails without success. Introducing a single print statement is enough for the bug to disappear. The code does contain a lot of unsafe calls so this is probably a bad example to investigate. In case this is still useful, |
|
One debugging step that might help is to switch over to the system allocator, which has better runtime corruption checks, and then maybe run in valgrind. |
|
Following @sfackler advise, I used the system allocator. The failure happens when deallocating a |
|
petgraph has been failing recently, not sure if its the same regression |
|
I switched Looking at the logs, I believe the bad codegen happens in this code: let mut nsec_info: Option<(&Name, Vec<RecordType>)> = None;
for key in self.records.keys() {
match nsec_info {
None => nsec_info = Some((&key.name, vec![key.record_type])),
Some((name, ref mut vec)) if name == &key.name => vec.push(key.record_type),
Some((name, vec)) => {
// names aren't equal, create the NSEC record
let mut record = Record::with(name.clone(), RecordType::NSEC, ttl);
let rdata = NSEC::new(key.name.clone(), vec);
record.set_rdata(RData::NSEC(rdata));
records.push(record);
// new record...
nsec_info = Some((&key.name, vec![key.record_type]))
}
}
}As far as I can tell, the problem is that |
|
Checking the mir, it indeed seems to be the case that the |
|
Here is the shortest piece of code I could write that reproduces. It is independant of tantivy but still calls the https://gist.github.com/fulmicoton/facc8d8218e9e78d05195fa8718e1c76 |
|
@fulmicoton: Thanks for that small sample! It also generates a broken mir with an erroneous drop for Note how |
|
The cc @nikomatsakis, @arielb1 since you reviewed that PR. |
TimNN
added
the
I-unsound
|
Here is an even smaller sample code that faults. No dependency whatsoever, no unsafe code. |
|
Further minified: pub struct DropHasLifetime<'a>(&'a ());
impl<'a> ::std::ops::Drop for DropHasLifetime<'a> {
fn drop(&mut self) {}
}
fn move_and_return<T>(val: T) -> T {
val
}
struct Wrapper<'a>(DropHasLifetime<'a>);
fn main() {
static STATIC: () = ();
let mut wrapper = Wrapper(DropHasLifetime(&STATIC));
wrapper.0 = move_and_return(wrapper.0);
}This no longer crashes but you'll see the erroneous drop if you look at the mir (slightly simplified, full version): Note how |
TimNN
changed the title
TimNN
changed the title
|
Fwiw, I've minified @fulmicoton 's example a bit, still crashes: https://is.gd/918r5N |
|
Further minified crashing example, but only in debug mode: https://is.gd/f3xUjK |
|
It appears as if Before Edit: The gist now includes the diff for |
|
I tried to make some sense of the debug logs, here is an excerpt: Judging by the doc comment on |
|
I'll take this. |
|
@arielb1 this seems pretty important to track down; if you don't have time, do raise the bat signal. |
The move gathering code is sensitive to type-equality - that is rather un-robust and I plan to fix it eventually, but that's a more invasive change. And we want to fix the visitor anyway. Fixes rust-lang#42903.
Leaving types unerased would lead to 2 types with a different "name" getting different move-paths, which would cause major brokenness (see e.g. rust-lang#42903). This does not fix any *known* issue, but is required if we want to use abs_domain with non-erased regions (because the same can easily have different names). cc @RalfJung.
erase types in the move-path abstract domain Leaving types unerased would lead to 2 types with a different "name" getting different move-paths, which would cause major brokenness (see e.g. rust-lang#42903). This does not fix any *known* issue, but is required if we want to use abs_domain with non-erased regions (because the same can easily have different names). cc @RalfJung. r? @eddyb
erase types in the move-path abstract domain Leaving types unerased would lead to 2 types with a different "name" getting different move-paths, which would cause major brokenness (see e.g. rust-lang#42903). This does not fix any *known* issue, but is required if we want to use abs_domain with non-erased regions (because the same can easily have different names). cc @RalfJung. r? @eddyb
Updated summary:
Under some conditions, drops are generated if a previously moved value is reassigned. This was bisected to #39409.
In the following minimised code for example:
wrapper.0is dropped twice, see #42903 (comment) for complete mir.-- Edited by @TimNN
Original Description:
There are now 3 instances of strange crashes/segfaults/hangs/error messages that happen using nightly 2017-06-20 (or it might be 2017-06-19) that don't happen with rustc 1.19.0-beta.2 (a175ee5 2017-06-15) reported on this reddit thread.
I wanted to start an issue for discussing and investigating this because I think it's clear there's something wrong, but I apologize that we haven't yet gotten an isolated reproduction. I also don't know if these cases are the same bug or different bugs yet, please let me know if I should split this out into more issues.
Or perhaps these already have issues! I didn't see any reported issues that looked similar enough to these and in the right time frame :-/
Incident 1 - unit tests crashing with "error: An unknown error occurred"
Via reddit user fulmicoton:
Will ask if they can share more details.
Incident 2 - crates.io's tests (at least one in particular)
Described in pull request rust-lang/crates.io#795, but I'm seeing weirdness on master too. All tests are passing using stable Rust on crates.io master.
On Wednesday, @jtgeibel was seeing one of cargo's tests (
krate::index_queries) intermittently segfaulting and crashing with(signal: 4, SIGILL: illegal instruction)On master, I'm seeing the tests fail in a really really bizarre way: it looks like serialized JSON is being corrupted???? The test is making a request to the crates.io server, which returns JSON, and then the test attempts to deserialize the JSON and make assertions on it. The JSON returned by the server is valid JSON, but one of the keys looks like it's set to the value of the beginning of the JSON???? Here's the output that won't deserialize, pretty-printed and a bunch of stuff that looks fine removed to make the problem more obvious, also with comments that I've added here even though JSON doesn't have comments:
To reproduce:
cargo test krate::index_queries. You've reproduced the problem if you see:cargo test krate::index_queries --verbose, I alternately get a failure that looks like:and a crash that looks like:
Incident 3 - trust-dns
Via @bluejekyll, see their description on this issue: bluejekyll/trust-dns#152
To reproduce:
cargo test test_nsec_query_example_nonet/cc @Mark-Simulacrum @jtgeibel @bluejekyll
The text was updated successfully, but these errors were encountered: