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

Guide.md: Change download instructions and remove bad info #15204

Closed
wants to merge 1 commit into from

Conversation

mcpherrinm
Copy link
Contributor

Piping curl directly to | sudo sh has very weird failure modes: If the connection is interrupted, you can end up executing part of a script.

The small rant about "Trusting Rust" to not hack your computer is completely misguided as well. You're going to run binaries one way or another, so you're still trusting Rust developers.

The fact that we're delivering unsigned binaries over http is still cringeworthy though.

Piping `curl` directly to `| sudo sh` has very weird failure modes:  If the connection is interrupted, you can end up executing part of a script.

The small rant about "Trusting Rust" to not hack your computer is completely misguided as well.  You're going to run binaries one way or another, so you're still trusting Rust developers.

The fact that we're delivering unsigned binaries over http is still cringeworthy though.
@mcpherrinm mcpherrinm changed the title Change download instructions and remove bad info Guide.md: Change download instructions and remove bad info Jun 26, 2014
@lilyball
Copy link
Contributor

I agree that the rant about "Trusting Rust" is inappropriate. I also agree that delivering unsigned binaries over http is unfortunate.

However, I think the problem of executing a partially-downloaded script can be solved without changing the ergonomics of running the script (i.e. by still allowing for piping directly to sudo sh). That can be done by taking the body of rustup.sh and wrapping it in a main() {} function, and then invoking that function at the bottom of the file. This way if the connection is interrupted before it finishes download the file, it won't actually execute any commands.

Technically speaking, there are various points in the file where a termination would in fact attempt to invoke a command, e.g. in any of the function definitions, terminating before the () would cause the function name to be invoked as a command, but it's unlikely that that would do anything.

Another suggestion might be to get rid of the sudo sh and have the script itself invoke sudo. That way a partial download won't even run anything with root permissions.

@lilyball
Copy link
Contributor

Perhaps piping is actually the wrong approach. It could instead say

script=$(curl -s http://www.rust-lang.org/rustup.sh) && sudo sh -c "$script"

This will avoid executing the script if curl returns with a non-zero exit code.

@steveklabnik
Copy link
Member

I would strongly prefer fixing Rustup rather than changing the instructions.

@mcpherrinm
Copy link
Contributor Author

How would you propose changing rustup? If it defaulted to installing per-user, I would still request this same change be made to the instructions, just without the sudo calls

@steveklabnik
Copy link
Member

There's two big concerns with Rustup, right now:

  1. sudo
  2. partial script execution

The first is fixible by modifying Rustup. The second... I can't even believe that that happens, though of course, lol computers. Do you have any more information on this actually happening?

Secondarily, it seems that a fix for that problem was presented by @kballard : " taking the body of rustup.sh and wrapping it in a main() {} function, and then invoking that function at the bottom of the file". Would that address your concern?

Other projects just pipe to sh. How do they solve this issue? Do they just ignore it?

I'm a bit loathe to change the instructions, because a single step to install is so much nicer than 'two steps and then delete this file.' Especially if we can get away with just one.

Finally, while you're right that you're installing a compiler, and so you have to trust us anyway, this argument always comes up on every HN/reddit thread where someone suggests curl | sh. It's an attempt to acknowledge and head that off. It should stay.

@mcpherrinm
Copy link
Contributor Author

Do you have any more information on this actually happening?

I've had it happen, and can somewhat reliably make it happen using a slow network connection that goes down for a few seconds in the middle of the script downloading. (In this case, I deliberately throttled it, but no worse than what I get on my cell phone on caltrain...). I tried to make an automated test case so you can try at home but it was a pain compared to just doing it manually.

@kyrias
Copy link
Contributor

kyrias commented Jun 30, 2014

I'm a bit loathe to change the instructions, because a single step to install is so much nicer than 'two steps and then delete this file.' Especially if we can get away with just one.

That sort of the point of @kballard's suggestion, it saves the script to a variable instead of a file, giving you the same benefit as a file but without having to care about deleting it.

@alexcrichton
Copy link
Member

Closing due to inactivity

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 17, 2023
internal: Add analysis-stats flag to trigger some IDE features

Closes rust-lang/rust-analyzer#15131

Running this on r-a showed an 86mb memory increase, but that was without running it on the deps, will try that later when I don't need to use my pc.
Sign up for free to 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.

5 participants