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

Use mktemp for temporary download directory #19227

Merged
merged 5 commits into from
Dec 29, 2014
Merged

Conversation

johshoff
Copy link
Contributor

Using the current directory may not always be appropriate, for example in
the case where it will unnecessarily trigger a backup to be made.

The only risk with this change is that systems might not have a mktemp.
I am not aware of such a system, but have not tested on Windows. It is
working on a basic Ubuntu and OS X installation.

Using the current directory may not always be appropriate, for example in
the case where it will unnecessarily trigger a backup to be made.

The only risk with this change is that systems might not have a mktemp.
I am not aware of such a system, but have not tested on Windows. It is
working on a basic Ubuntu and OS X installation.
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @huonw (or someone else) soon.

@nodakai
Copy link
Contributor

nodakai commented Nov 23, 2014

Does it not suffice to simply cd to the $TMPDIR environment variable (or alike) when creating the working directory? Security experts would say mktemp(1) is essential for avoiding the symlink attack, but... should all of us be that cautious? Using different working directories every time isn't desirable for, eg. supporting HTTP GET resume.

Apart from that, you really need to check if TMP_DIR was successfully set or not. One of the idioms for check is:

TMP_DIR=`mktemp -d` || exit 1

There are several invocation of rm -Rf "${TMP_DIR}" later in the script. They scare me a lot...

If mktemp fails, fall back to a hard coded directory, per @nodakai's feedback.
@johshoff
Copy link
Contributor Author

@nodakai Thanks for taking a look! Good point about exiting if it fails.

I made a fallback to the current bahavior if mktemp fails. That should take care of those problems.

I didn't think about the HTTP resume scenario, but it doesn't seem like rustup is trying to do that—it deletes the folder up front.

Does it not suffice to simply cd to the $TMPDIR environment variable (or alike) when creating the working directory?

Yes, I could definitely just do that. It just didn't seem right to me to pollute the current directory (whatever that might be) with a rustup-tmp-install directory. Especially when if the user, like me, triggers automatic backup routines in said folder.

To be honest, this isn't a big deal to me; I can always work around it. I just assume that someone else will have simlar issues.

@steveklabnik
Copy link
Member

/cc @brson

@brson
Copy link
Contributor

brson commented Dec 3, 2014

Will probably need a rebase after #19170 lands.

@frewsxcv
Copy link
Member

#19170 has landed (19 days ago) and this just needs a rebase at this point

@johshoff
Copy link
Contributor Author

@frewsxcv Thanks for the heads up. Merged master into the fork.

@@ -401,7 +413,7 @@ then
CFG_INSTALL_FLAGS="${CFG_INSTALL_FLAGS} --prefix=${CFG_PREFIX}"
fi

CFG_TMP_DIR="./rustup-tmp-install"
CFG_TMP_DIR=`mktemp -d 2>/dev/null || mktemp -d -t 'rustup-tmp-install' 2>/dev/null` || CFG_TMP_DIR=$(create_tmp_dir)
Copy link
Member

Choose a reason for hiding this comment

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

This line is over 100 characters long, which fails the style checker

Copy link
Contributor

Choose a reason for hiding this comment

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

This probably use $() instead of backticks, but it must certainly be quoted.

@johshoff
Copy link
Contributor Author

@frewsxcv Thanks again for the feedback. I split the line into three lines. Not sure if it matches the style guide (if you even have one for bash).

@tbu- I changed it to use $() instead. Good suggestion. Not sure what you mean about the quotation—the current way should work fine even if there are e.g. spaces in the returned values.

@tbu-
Copy link
Contributor

tbu- commented Dec 28, 2014

@johshoff Yea, I was wrong about the needed quotation. Thanks for the change to $().

bors added a commit that referenced this pull request Dec 29, 2014
Using the current directory may not always be appropriate, for example in
the case where it will unnecessarily trigger a backup to be made.

The only risk with this change is that systems might not have a mktemp.
I am not aware of such a system, but have not tested on Windows. It is
working on a basic Ubuntu and OS X installation.
@bors bors merged commit 0e2b5d9 into rust-lang:master Dec 29, 2014
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.