rust-lang / rustup

The Rust toolchain installer
https://rust-lang.github.io/rustup/
Apache License 2.0
6.18k stars 891 forks source link

Rustup doesn't check the platform OS version correctly. #2606

Closed arafangion closed 3 years ago

arafangion commented 3 years ago

I previously mis-reported this at https://github.com/rust-lang/rust/issues/79924 Problem

Steps

  1. Try to install rust using Rustup on an Apple Silicon laptop.

% curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh

Expected this to install rust, however there are two problems, lets focus on the first one in this ticket: (The other issue was I was expecting it to install the tier-2 platform, but it seems I need the nightly for that, which will be fine).

Warning: Not enforcing strong cipher suites for TLS, this is potentially less secure
Warning: Detected OS X platform older than 10.13
Warning: Not enforcing TLS v1.2, this is potentially less secure

Big Sur is definitely not older than 10.13!

Possible Solution(s)

Correctly parse the output for the OS version, which is as follows:

% sw_vers -productVersion 11.0.1

Notes

Output of rustup --version: Output of rustup show:

kinnison commented 3 years ago

Yep, currently we only check the minor of the version and so we get this wrong. It should be a pretty easy fix in rustup-init.sh's check_help_for code.

shepmaster commented 3 years ago

I don't see this behavior:

% sw_vers -productVersion
11.0.1

% curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh
info: downloading installer

Welcome to Rust!

This will download and install the official compiler for the Rust
programming language, and its package manager, Cargo.

Rustup metadata and toolchains will be installed into the Rustup
home directory, located at:

  /Users/shepmaster/.rustup

This can be modified with the RUSTUP_HOME environment variable.

The Cargo home directory located at:

  /Users/shepmaster/.cargo

This can be modified with the CARGO_HOME environment variable.

The cargo, rustc, rustup and other commands will be added to
Cargo's bin directory, located at:

  /Users/shepmaster/.cargo/bin

This path will then be added to your PATH environment variable by
modifying the profile files located at:

  /Users/shepmaster/.profile
  /Users/shepmaster/.zshenv

You can uninstall at any time with rustup self uninstall and
these changes will be reverted.

Current installation options:

   default host triple: aarch64-apple-darwin
     default toolchain: stable (default)
               profile: default
  modify PATH variable: yes

1) Proceed with installation (default)
2) Customize installation
3) Cancel installation
>2

I'm going to ask you the value of each of these installation options.
You may simply press the Enter key to leave unchanged.

Default host triple?

Default toolchain? (stable/beta/nightly/none)
beta

Profile (which tools and data to install)? (minimal/default/complete)
minimal

Modify PATH variable? (y/n)

Current installation options:

   default host triple: aarch64-apple-darwin
     default toolchain: beta
               profile: minimal
  modify PATH variable: yes

1) Proceed with installation (default)
2) Customize installation
3) Cancel installation
>1

info: profile set to 'minimal'
info: setting default host triple to aarch64-apple-darwin
warning: Updating existing toolchain, profile choice will be ignored
info: syncing channel updates for 'beta-aarch64-apple-darwin'
info: default toolchain set to 'beta-aarch64-apple-darwin'

  beta-aarch64-apple-darwin unchanged - rustc 1.49.0-beta.4 (877c7cbe1 2020-12-10)

Rust is installed now. Great!

To get started you need Cargo's bin directory ($HOME/.cargo/bin) in your PATH
environment variable. Next time you log in this will be done
automatically.

To configure your current shell, run:
source $HOME/.cargo/env
arafangion commented 3 years ago

I took a look at this because I was asked to test the changes, but then I noticed I didn't see this behaviour in the git version of the code and still see it in the production version.

It appears that the code changes in 01fd3a19774e97839f6b3c52075a491ea9479abd somehow suppresses this code, so to test this specific change now in the official master I applied a revert of that commit locally.

However, I don't see that in kinnvison's version, I am not sure of the implications here, however, kinnvison's change does fix the symptom I have.

(Incidentally, because it's relevant to that commit 01fd3a... I've applied my curl version below).

% curl --version
curl 7.73.0 (arm-apple-darwin20.1.0) libcurl/7.73.0 OpenSSL/1.1.1h zlib/1.2.11 zstd/1.4.5 libidn2/2.3.0 libpsl/0.21.1 (+libidn2/2.3.0)
Release-Date: 2020-10-14
Protocols: dict file ftp ftps gopher http https imap imaps mqtt pop3 pop3s rtsp smb smbs smtp smtps telnet tftp
Features: AsynchDNS HTTPS-proxy IDN IPv6 Largefile libz NTLM NTLM_WB PSL SSL TLS-SRP UnixSockets zstd
kinnison commented 3 years ago

It's unclear how the code change you reference could have affected the result of calling sw_vers - very odd.

arafangion commented 3 years ago

@kinnison It took a bit of digging, but it appears that check_help_for is first called with $_arch being set to 'notspecified', which then influences whether curl is supported and with which cipher suites. With the code change I referenced, this now returns '1'.

If it can't find anything, then it provides those warnings by calling check_help_for, and this time specifies the arch to 'aarch64-apple-darwin', which then executes the code containing your fix, and that's why the code change I referenced affects the result because it changes the code path that is ultimately being used.

kinnison commented 3 years ago

And this is why reading only an isolated function is a bad plan when reviewing shell code :D It sounds like in theory we don't need the PR I wrote for this issue; but I'd rather belt-and-braces this just in case someone has an old curl on their path.

arafangion commented 3 years ago

I agree! And if you don't fix it and the code is used again in the future, it'll cause a bit of a surprise, so I think this is a good change to make.

mohd-akram commented 3 years ago

The version on rustup.rs is still broken btw.

kinnison commented 3 years ago

@mohd-akram Yes, we've not made a release yet. It's pretty close but not done yet.