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

starting from nightly-2020-08-18 rustls can't connect to some websites #76803

Closed
paolobarbolini opened this issue Sep 16, 2020 · 19 comments · Fixed by #76844
Closed

starting from nightly-2020-08-18 rustls can't connect to some websites #76803

paolobarbolini opened this issue Sep 16, 2020 · 19 comments · Fixed by #76844
Assignees
Labels
A-mir-opt Area: MIR optimizations 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-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@paolobarbolini
Copy link
Contributor

I tried this code:

use std::io::{self, stdout, Write};
use std::net::TcpStream;
use std::sync::Arc;

use rustls::{ciphersuite, ClientConfig, ClientSession, Session, Stream};
use webpki::DNSNameRef;
use webpki_roots::TLS_SERVER_ROOTS;

fn main() {
    env_logger::init();

    let mut config = ClientConfig::with_ciphersuites(&[&ciphersuite::TLS13_AES_256_GCM_SHA384]);
    config
        .root_store
        .add_server_trust_anchors(&TLS_SERVER_ROOTS);
    // works with TLSv1_2 or with different ciphersuites
    config.versions = vec![rustls::ProtocolVersion::TLSv1_3];
    // this is what gets picked by default when using TLS 1.3
    config.ciphersuites = vec![&rustls::ciphersuite::TLS13_AES_256_GCM_SHA384];

    const DOMAIN: &str = "example.com";

    let dns_name = DNSNameRef::try_from_ascii_str(DOMAIN).unwrap();
    let mut sess = ClientSession::new(&Arc::new(config), dns_name);
    let mut sock = TcpStream::connect((DOMAIN, 443)).unwrap();
    let mut tls = Stream::new(&mut sess, &mut sock);
    tls.write_all(
        [
            "GET / HTTP/1.1\r\n",
            "Host: ",
            DOMAIN,
            "\r\n",
            "Connection: close\r\n",
            "\r\n",
        ]
        .join("")
        .as_bytes(),
    )
    .unwrap(); // fails here
    let ciphersuite = tls.sess.get_negotiated_ciphersuite().unwrap();
    println!("Current ciphersuite: {:?}", ciphersuite.suite);

    let mut out = stdout();
    if let Err(err) = io::copy(&mut tls, &mut out) {
        eprintln!("err: {}", err)
    }
}

With dependencies:

rustls = { version = "0.18.1", features = ["logging"] }
webpki = "0.21.3"
webpki-roots = "0.20.0"
env_logger = "0.7.1"

I expected to see this happen: it establishes a TLS connection, sends the HTTP/1.1 request and prints the entire response

Instead, this happened: fails with Custom { kind: InvalidData, error: DecryptError }

I bisected this to nightly-2020-08-18. I couldn't reproduce this issue with other websites.

Meta

rustc --version --verbose:

rustc 1.48.0-nightly (9b4154193 2020-09-14)
binary: rustc
commit-hash: 9b4154193e8471f36b1a9e781f1ef7d492fc6a6c
commit-date: 2020-09-14
host: x86_64-unknown-linux-gnu
release: 1.48.0-nightly
LLVM version: 11.0
Backtrace when running with cargo run

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Custom { kind: InvalidData, error: DecryptError }', src/main.rs:39:6
stack backtrace:
   0: rust_begin_unwind
             at /rustc/9b4154193e8471f36b1a9e781f1ef7d492fc6a6c/library/std/src/panicking.rs:483
   1: core::panicking::panic_fmt
             at /rustc/9b4154193e8471f36b1a9e781f1ef7d492fc6a6c/library/core/src/panicking.rs:85
   2: core::option::expect_none_failed
             at /rustc/9b4154193e8471f36b1a9e781f1ef7d492fc6a6c/library/core/src/option.rs:1221
   3: core::result::Result<T,E>::unwrap
             at /home/paolo/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/result.rs:973
   4: proxy_bug::main
             at ./src/main.rs:27
   5: core::ops::function::FnOnce::call_once
             at /home/paolo/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:227
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

@paolobarbolini paolobarbolini added the C-bug Category: This is a bug. label Sep 16, 2020
@jonas-schievink
Copy link
Contributor

I bisected this to nightly-2020-08-18.

Which commit is that? (and nightly-2020-08-17)

@paolobarbolini
Copy link
Contributor Author

Ok I got cargo-bisect-rustc to work, here's the output:


searched nightlies: from nightly-2020-08-15 to nightly-2020-08-20
regressed nightly: nightly-2020-08-18
searched commits: from 7e6d6e5 to 792c645
regressed commit: 33c96b4

bisected with cargo-bisect-rustc v0.5.2

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

cargo bisect-rustc --preserve --start=2020-08-15 --end=2020-08-20 -- test 

@jonas-schievink jonas-schievink added A-mir-opt Area: MIR optimizations E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 16, 2020
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Sep 16, 2020
@jonas-schievink
Copy link
Contributor

Great, thanks for the report!

@tmiasko
Copy link
Contributor

tmiasko commented Sep 16, 2020

#[derive(Copy, Clone, Debug, PartialEq, Eq)]
#[repr(u8)]
pub enum Type {
    A = 1,
    B = 7,
}

impl Type {
    fn encode(&self) -> Type {
        match self {
            Type::A => Type::B,
            _ => *self,
        }
    }
}

fn main() {
    assert_eq!(Type::A.encode(), Type::B);
}
$ rustc -Zmir-opt-level=0 b.rs && ./b
$ rustc -Zmir-opt-level=1 b.rs && ./b
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `A`,
 right: `B`', b.rs:18:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@Stupremee
Copy link
Member

Stupremee commented Sep 16, 2020

@tmiasko: Is there a relation to the original example code or to the issue in general?

@jyn514
Copy link
Member

jyn514 commented Sep 16, 2020

@Stupremee the code example has different behavior with and without optimizations. Since there's no unsafe in the code, that means the compiler is unsound.

@Stupremee
Copy link
Member

Yea I know, but what does it have to do with the issue / code from paolobarbolini?

@tmiasko
Copy link
Contributor

tmiasko commented Sep 16, 2020

It's minimized code from rustls <HandshakeMessagePayload as Codec>::encode.

@jyn514 jyn514 removed the E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example label Sep 16, 2020
@jyn514
Copy link
Member

jyn514 commented Sep 16, 2020

Marking as P-critical as determined by WG-prioritization.

@jyn514 jyn514 added P-critical Critical priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Sep 16, 2020
@Stupremee
Copy link
Member

Ahh okay. Thanks

@lcnr
Copy link
Contributor

lcnr commented Sep 17, 2020

This looks like a mir-opt bug to me: https://godbolt.org/z/n86G5M

cc @rust-lang/wg-mir-opt

#[derive(Copy, Clone)]
#[repr(u8)]
pub enum Type {
    A = 1,
    B = 7,
}


pub fn encode(v: &Type) -> Type {
    match v {
        Type::A => Type::B,
        _ => *v,
    }
}

with --emit=mir

fn encode(_1: &Type) -> Type {
    debug v => _1;                       // in scope 0 at ./example.rs:9:15: 9:16
    let mut _0: Type;                    // return place in scope 0 at ./example.rs:9:28: 9:32

    bb0: {
        _0 = (*_1);                      // scope 0 at ./example.rs:12:14: 12:16
        return;                          // scope 0 at ./example.rs:14:2: 14:2
    }
}

edit: fn encode(v: Type) has the same bug, so it is not necessarily related to references

@lcnr
Copy link
Contributor

lcnr commented Sep 17, 2020

simpler repro: https://godbolt.org/z/3cdfa8

pub enum Type {
    A,
    B,
}


pub fn encode(v: Type) -> Type {
    match v {
        Type::A => Type::B,
        _ => v,
    }
}

@oli-obk
Copy link
Contributor

oli-obk commented Sep 17, 2020

Wild guess: #74748

@jonas-schievink
Copy link
Contributor

It was already bisected to that PR in #76803 (comment)

@SNCPlay42
Copy link
Contributor

This repros on beta, so this is a stable-to-beta regression.

@spastorino spastorino added regression-from-stable-to-beta Performance or correctness regression from stable to beta. and removed regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Sep 17, 2020
@SNCPlay42
Copy link
Contributor

SNCPlay42 commented Sep 17, 2020

/// Tests if two statements can be considered equal
///
/// Statements can be trivially equal if the kinds match.
/// But they can also be considered equal in the following case A:
/// ```
/// discriminant(_0) = 0; // bb1
/// _0 = move _1; // bb2
/// ```
/// In this case the two statements are equal iff
/// 1: _0 is an enum where the variant index 0 is fieldless, and
/// 2: bb1 was targeted by a switch where the discriminant of _1 was switched on

This fails to consider that the discriminant value assigned in bb1 might not be the discriminant of _1, as happens here.

    bb0: {
        _2 = discriminant(_1);           // scope 0 at src/test/mir-opt/issue-76803.rs:12:9: 12:16
        switchInt(move _2) -> [0_isize: bb2, otherwise: bb1]; // scope 0 at src/test/mir-opt/issue-76803.rs:12:9: 12:16
        //                     ^^^^^^^ not the same value!
    }

    bb1: {
        _0 = move _1;                    // scope 0 at src/test/mir-opt/issue-76803.rs:13:14: 13:15
        goto -> bb3;                     // scope 0 at src/test/mir-opt/issue-76803.rs:11:5: 14:6
    }

    bb2: {
        discriminant(_0) = 1;            // scope 0 at src/test/mir-opt/issue-76803.rs:12:20: 12:27
        //                 ^ not the same value!
        goto -> bb3;                     // scope 0 at src/test/mir-opt/issue-76803.rs:11:5: 14:6
    }

@wesleywiser
Copy link
Member

I opened a PR to disable the logic and nominated it for beta backport.

wesleywiser added a commit to wesleywiser/rust that referenced this issue Sep 17, 2020
The logic is currently broken and we need to disable it to fix a beta
regression (see rust-lang#76803)
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 18, 2020
…r=oli-obk

[mir-opt] Disable the `ConsideredEqual` logic in SimplifyBranchSame opt

The logic is currently broken and we need to disable it to fix a beta
regression (see rust-lang#76803)

r? `@oli-obk`
@wesleywiser wesleywiser self-assigned this Sep 24, 2020
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 25, 2020
Fix rust-lang#76803 miscompilation

Fixes rust-lang#76803
Seems like it was an oversight that the discriminant value being set was not compared to the target value from the SwitchInt, as a comment says this is a requirement for the optimization to be sound.

r? `@wesleywiser` since you are probably familiar with the optimization and made rust-lang#76837 to workaround the bug
@bors bors closed this as completed in 738ed9b Sep 25, 2020
@wesleywiser
Copy link
Member

I'm re-opening to track the beta-backport. Once that is completed, we can close this.

@wesleywiser wesleywiser reopened this Sep 25, 2020
Mark-Simulacrum pushed a commit to Mark-Simulacrum/rust that referenced this issue Sep 28, 2020
The logic is currently broken and we need to disable it to fix a beta
regression (see rust-lang#76803)
@wesleywiser
Copy link
Member

The beta backport has landed (#77308). Closing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-mir-opt Area: MIR optimizations 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-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.