Skip to content

rewrite rustup to protect against truncation errors - #19170

Merged
bors merged 11 commits into
rust-lang:masterfrom
erickt:rustup
Dec 4, 2014
Merged

rewrite rustup to protect against truncation errors#19170
bors merged 11 commits into
rust-lang:masterfrom
erickt:rustup

Conversation

@erickt

Copy link
Copy Markdown
Contributor

This closes#19168.

Please be careful reviewing this since this gets used all over the place. I've tested all the options and everything appears to be working though.

@aturon

Copy link
Copy Markdown
Contributor

cc @brson@alexcrichton

Comment threadsrc/etc/rustup.sh Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should still default to downloading from rust-nightly-... because we're not always guaranteed to have nightlies for each date (they could fail to sync).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or... I may not know how to read bash. So let me confirm instead! This doesn't download from a specific date with no flags specified, right?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, when no flag is specified it downloads the current nightly, which does not include the date in the url.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this sets the cache dir based on the local system's date? If so, won't it fail to update if a new nightly is available on the server, but the date hasn't yet changed on my computer (e.g. if I'm in a different timezone)?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with this comment. It is very likely the dates will get out of sync. I'd suggest that if a specific date isn't provided that either a) --save has no effect, b) it is stored directly in the .rustup folder and the .sha256 file (which exists for all tarballs) is d/l'd and used to determine if it is fresh before using.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternately, it could be stored in the dated folder, but all cached installs could be checksummed from the network before installing. Might be a slightly simpler arrangement.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, hm. It looks like because of https://github.com/erickt/rust/blob/9afe5de38d279a0cc127b038c072e849adaffe18/src/etc/rustup.sh#L495, if --save is selected then the script tries to download from the archives using today's date. That solves the problem of the dates being out of sync, but adds the problem that today may not have a nightly yet. I at least like this better since --save is not the default, but it's a bit wonky.

(My original comment may have been confusing since I didn't notice this).

@alexcrichton

Copy link
Copy Markdown
Member

Just confirming, but you've run this multiple times in a few flavorful configurations, to make sure it works, right?

@alexcrichton

Copy link
Copy Markdown
Member

also otherwise looks good to me, r? @brson (good to have two sets of eyes on shell scripts)

@alexcrichton

Copy link
Copy Markdown
Member

Hm I checked this out locally and got this error:

$ curl -O https://raw.githubusercontent.com/erickt/rust/rustup/src/etc/rustup.sh
$ chmod +x rustup.sh
$ ./rustup.sh
rustup: CFG_CURL := /usr/bin/curl (7.35.0)
rustup: CFG_TAR := /bin/tar (1.27.1)
rustup: rustup: processing ./rustup.sh args
rustup: rustup: CFG_PREFIX := rustup: CFG_NIGHTLY := rustup: rustup: validating ./rustup.sh args
rustup: rustup: host triple: x86_64-unknown-linux-gnu
rustup: downloading https://static.rust-lang.org/dist/rust-nightly-x86_64-unknown-linux-gnu.tar.gz to ./rustup-tmp-install/rust-nightly-x86_64-unknown-linux-gnu.tar.gz
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 105M 100 105M 0 0 43.9M 0 0:00:02 0:00:02 --:--:-- 43.9M
rustup: Extracting ./rustup-tmp-install/rust-nightly-x86_64-unknown-linux-gnu.tar.gz
/bin/tar: ./rustup-tmp-install/rust-nightly-x86_64-unknown-linux-gnu.tar.gz: Cannot open: No such file or directory
/bin/tar: Error is not recoverable: exiting now
rustup: error: failed to unpack installer

@brson

Copy link
Copy Markdown
Contributor

This change appears to ignore the --disable-cargo flag. This flag may not be worth supporting since in the future we're going to be installing both from the same tarball, but it should probably not just be ignored.

The old script did all downloading before all installing, which seems appropriate to me: get the fallable network business done first. I think I'd prefer to preserve that behavior.

What does it mean to 'protect against truncation'? Is it that everything is wrapped in a function call, so the entire function has to be downloaded before anything in the script can be run? If so please spell that out in comments so I don't forget.

I presume the use-case for --save is projects that are pinning to a specific nightly and expect to be switching between branches frequently.

In anticipation of release channels we might want to futureproof the name of the --nightly flag. There will probably end up being a --channel=nightly|beta|stable flag, and it will be possible to pull the archives from any of those channels. I might suggest --date as the name of this flag instead of --nightly.

@erickt

Copy link
Copy Markdown
ContributorAuthor

shell can be confusing to read, so I refactored the code into a bunch of small, incremental changes. I've also pulled out the caching behavior for a future PR so that we can land a safer rustup.sh sooner rather than later. In addition, I fixed the problem @alexcrichton ran into, and added better documentation as requested by @brson.

@erickterickt changed the title rewrite rustup to support caching nightlies, protects against truncation errorsrewrite rustup to protect against truncation errorsDec 1, 2014
@erickt
ericktforce-pushed the rustup branch 3 times, most recently from 7b02098 to 76fbf57CompareDecember 1, 2014 07:49
@alexcrichton

Copy link
Copy Markdown
Member

@erickt hm I still get an odd error locally, maybe a different one?

[~] $ curl https://raw.githubusercontent.com/erickt/rust/rustup/src/etc/rustup.sh | sh % Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 10860 100 10860 0 0 205k 0 --:--:-- --:--:-- --:--:-- 207k
rustup: CFG_CURL := /usr/bin/curl (7.35.0)
rustup: CFG_TAR := /bin/tar (1.27.1)
rustup: rustup: processing sh args
rustup: rustup: CFG_PREFIX := rustup: rustup: validating sh args
rustup: rustup: host triple: x86_64-unknown-linux-gnu
rustup: downloading https://static.rust-lang.org/dist/rust-nightly-x86_64-unknown-linux-gnu.tar.gz to ./rustup-tmp-install/rust-nightly-x86_64-unknown-linux-gnu.tar.gz
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 104M 100 104M 0 0 61.7M 0 0:00:01 0:00:01 --:--:-- 61.7M
rustup: downloading https://static.rust-lang.org/cargo-dist/cargo-nightly-x86_64-unknown-linux-gnu.tar.gz to ./rustup-tmp-install/cargo-nightly-x86_64-unknown-linux-gnu.tar.gz
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 4642k 100 4642k 0 0 30.4M 0 --:--:-- --:--:-- --:--:-- 30.6M
rustup: Extracting rust-nightly-x86_64-unknown-linux-gnu.tar.gz
/bin/tar: ./rustup-tmp-install/rust-nightly-x86_64-unknown-linux-gnu.tar.gz: Cannot open: No such file or directory
/bin/tar: Error is not recoverable: exiting now
rustup: error: failed to unpack installer

@nodakai

Copy link
Copy Markdown
Contributor

static.rust-lang.org should publicize MD5 or SHA* checksums and that should be the final solution.

@thestinger

Copy link
Copy Markdown
Contributor

It should have GPG signatures for nightlies just as it does for the tagged releases.

@thestinger

Copy link
Copy Markdown
Contributor

There can be a separate key for whatever automated builder is responsible for making them.

@erickt

Copy link
Copy Markdown
ContributorAuthor

Nightlies do have a .sha256 hash file along side of the tar balls. I've got a patch queued up that checks the hash, I just had enough trouble with this PR I wanted land this first before I do the next round of patches.

bors added a commit that referenced this pull request Dec 4, 2014
This closes#19168.
Please be careful reviewing this since this gets used all over the place. I've tested all the options and everything appears to be working though.
@borsbors closed this Dec 4, 2014
@bors
bors merged commit f867379 into rust-lang:masterDec 4, 2014
lnicola added a commit to lnicola/rust that referenced this pull request Feb 17, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make sure rustup.sh is safe from truncation errors

8 participants

@erickt@aturon@alexcrichton@brson@nodakai@thestinger@zofrex@bors