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

Regression from 1.44 to 1.45 #74739

Closed
mcarton opened this issue Jul 25, 2020 · 19 comments · Fixed by #75449
Closed

Regression from 1.44 to 1.45 #74739

mcarton opened this issue Jul 25, 2020 · 19 comments · Fixed by #75449
Labels
C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-critical Critical priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-release Relevant to the release subteam, which will review and decide on the PR/issue.

Comments

@mcarton
Copy link
Member

mcarton commented Jul 25, 2020

The following MVE prints 42 with rustc 1.45.0 but 13 with rustc 1.44.0

struct Foo {
    x: i32,
}

fn main() {
    let mut foo = Foo { x: 42 };
    let x = &mut foo.x;
    *x = 13;
    let y = foo;
    println!("{}", y.x); // -> 42; expected result: 13
}

Initially reported on Stack Overflow.

@mcarton mcarton added the C-bug Category: This is a bug. label Jul 25, 2020
@mcarton
Copy link
Member Author

mcarton commented Jul 25, 2020

With beta or nightly this again prints 13 as expected.

@jonas-schievink jonas-schievink added I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness regression-from-stable-to-stable Performance or correctness regression from one stable version to another. labels Jul 25, 2020
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Jul 25, 2020
@ghost
Copy link

ghost commented Jul 25, 2020

struct Foo {
    x: i32,
}

fn main() {
    let mut foo = Foo { x: 42 };

    let x = &mut foo.x;
    *x = 13;
    let y = foo;
    println!("{:?}", (&y).x);  //only added this line
    println!("{:?}", y.x); //13
}

If there is any benefit, it works as expected with this code.

@lcnr
Copy link
Contributor

lcnr commented Jul 25, 2020

This is a duplicate of the already fixed #73609

It seems like this bug somehow slipped into beta :/

@jonas-schievink jonas-schievink added the T-release Relevant to the release subteam, which will review and decide on the PR/issue. label Jul 25, 2020
@SimonSapin
Copy link
Contributor

@rust-lang/release Should there be a 1.45.1 with #73609?

@peterjoel

This comment has been minimized.

@jonas-schievink

This comment has been minimized.

@peterjoel

This comment has been minimized.

@benschulz
Copy link
Contributor

I'm confused. Based on the examples this bug appears to be very impactful, i.e. it should break a lot of code. Yet it made it to stable which, I assume, involves a crater run and independent testing on beta. Is there a subtle condition to this bug that isn't met in most productive code?

@oli-obk
Copy link
Contributor

oli-obk commented Jul 25, 2020

The bug is almost impossible to trigger on real world code. You need all values that are going into the bug to be constant values and there can't be any control flow or function calls in between.

@wesleywiser
Copy link
Member

As far as I know, rustup has no support for yanking releases. Doing so would probably have to be done manually by the infra team and it could cause more harm than good. For example, are current or old versions of rustup going to gracefully handle the case where a user has an installed release that doesn't exist anymore? We've had other point releases for major issues in the past and users are generally quick to upgrade.

There is already a 1.45.1 release scheduled for later this week with other backports and this fix will almost certainly be included.

@jieyouxu

This comment has been minimized.

@LeSeulArtichaut LeSeulArtichaut added P-critical Critical priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Jul 25, 2020
@LeSeulArtichaut
Copy link
Contributor

Assigning P-critical as discussed as part of the Prioritization WG Procedure.

@Mark-Simulacrum
Copy link
Member

I am happy to include 5fa8b08 in 1.45.1. @oli-obk -- there's some merge conflicts when cherry-picking onto stable, could you provide a commit/PR against stable with the merge conflicts fixed?

FWIW, I find the notion of putting warnings or saying "don't use this release" to be something we probably don't want to do right now. We'd basically have to do so for all non-latest Rust versions; they're all "unsupported" and may contain this (or other) problems. We ourselves do not patch more than the latest stable release if CVEs are detected, generally speaking. I would also ask that further discussion about yanking releases and such be directed to an internals thread and not this issue to avoid creating noise while we resolve this bug.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 25, 2020

on it Nevermind, @wesleywiser is already on it ❤️

@zhuxiujia

This comment has been minimized.

@llacroix

This comment has been minimized.

@oli-obk

This comment has been minimized.

@jonas-schievink
Copy link
Contributor

This thread has served its purpose, so locking it.

@rust-lang rust-lang locked and limited conversation to collaborators Jul 25, 2020
@pnkfelix
Copy link
Member

pnkfelix commented Jul 30, 2020

@lcnr commented:

This is a duplicate of the already fixed #73609

by the way, this issue was not, technically, a duplicate of #73609.

Or at least, the issue described in #73609 only surfaces under conditions witnessed after commit 38bd83d (PR #71911, merged on June 21st)

bisected with cargo-bisect-rustc v0.5.2

Host triple: x86_64-unknown-linux-gnu
Reproduce with:

cargo bisect-rustc 2020-04-01 --end 2020-06-22 -- run

while this issue surfaces under conditions witnessed after commit 3fe4dd2 (PR #71953, merged on May 11th)

bisected with cargo-bisect-rustc v0.5.2

Host triple: x86_64-unknown-linux-gnu
Reproduce with:

cargo bisect-rustc 2020-05-01 --end 2020-06-01 -- run

It does seem like the same patch (PR #73613) fixes both issues; i.e., it seems likely that there is indeed one bug underlying both distinct issues.

But the fact that they are distinct issues helps explain why we did not backport the fix to beta way back when.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Aug 13, 2020
@bors bors closed this as completed in 845fb94 Aug 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-critical Critical priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-release Relevant to the release subteam, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.