rustup install fails when rustup-init is owned by another user

[nlfiedler]

Not sure if this is a rustup or Homebrew rustup-init issue, but I found that installing rustup-init using Homebrew, and then running rustup-init as a different user resulted in a permissions error (shown below). The result was that rustup would install toolchains, but never set up the binaries in ~/.cargo/bin.

Current installation options:

   default host triple: x86_64-apple-darwin
     default toolchain: stable
  modify PATH variable: yes

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

error: failed to set permissions for '/Users/p4nathan/.cargo/bin/rustup'
info: caused by: Operation not permitted (os error 1)

Changing the ownership of /usr/local/bin/rustup-init to the user running rustup averted the issue and everything installed as expected. Maybe I'm weird in that I want one installation of rustup-init but have multiple accounts on the same system using rustup. At least for now I can work around this.

[kinnison]

Is, by any chance, /usr/local/bin/rustup-init a symlink? Or perhaps however your user is finding rustup-init on your path a symlink? if so, we don't copy the file we link to it, which would make the chmod fail for sure.

Given that, perhaps the trick will be to make utils::make_executable() notice if the target is already executable and simply not attempt the chmod().

If it's not a symlink then I'll need to think again.

[nlfiedler]

Yes, with Homebrew everything under /usr/local/bin is a link.

$ ls -l /usr/local/bin/rustup-init
lrwxr-xr-x 1 nfiedler admin 44 Dec 11 08:16 /usr/local/bin/rustup-init -> ../Cellar/rustup-init/1.16.0/bin/rustup-init

$ ls -l /usr/local/Cellar/rustup-init/1.16.0/bin/
total 6500
-rwxr-xr-x 10 nfiedler staff 6655064 Dec  6 12:01 rustup-init

Thanks

[kinnison]

Okay, so I believe the right approach will be to check the permissions and not try and alter them if they're okay already. I'll look at what that will entail, and update the issue. I think this might end up a nice new-contributor issue if you're okay waiting for a bit for a fix?

[nlfiedler]

No problem, it's easy to work around.

[kinnison]

For someone who wanted to ensure that this kind of thing was fixed for the future, then it'll probably be a combination of altering fn make_executable(path: &Path) -> Result<()> which lives in src/rustup-utils/src/utils.rs

I'd expect that something where we can only call set_permissions if the mode changes in the non-Windows inner() would be the right approach.

A test which then validates this could be done by means of creating a read-only directory containing rustup-init already executable; make a symlink to that in another directory, and then run it and see if an install can be done.

My expectation is that making the test will be the hardest part of this change. I am happy to help someone through this, ping me if you want to try and write a PR to solve it.

[lallotta]

Hi @kinnison
I'd like to help here if still needed. I've replicated the issue and am prepared to test. Any further tips? Thanks!

[kinnison]

Hi @lallotta and thank you for wanting to help.

I would suggest that since you've managed to replicate the issue you're in a good position to try and narrow down a bare-minimum test case to be added to the test suite which demonstrates the problem.

My un-researched thoughts are above, but in summary, I think that this is a case that set_permissions likely fails because it's unable to alter the source binary, despite it already being executable. To that end, I think it's worth first trying and seeing if you get a similar error if the binary is owned by the same user, but is in a directory which is read-only rather than read-write. E.g.

mkdir test-dir
cp target/debug/rustup-init test-dir/
chmod 555 test-dir
test-dir/rustup-init ...

If that also replicates the issue then we're a step closer to building a plausible test to run in CI.

Once we have a test in CI, probably as part of tests/cli-self-upd.rs, then we can proceed to the fix.

Good luck, and please don't hesitate to ask if you want further guidance. I'm also usually on the #wg-rustup channel in Discord

[lallotta]

Thank you for your guidance @kinnison

I did some experimenting with your notes, and I found that running the binary as its owner from a read-only directory still succeeds:

$ ls -ld test-dir/ test-dir/rustup-init
dr-xr-xr-x  3 lorenzo  staff        96 Apr 10 18:36 test-dir/
-rwxr-xr-x  1 lorenzo  staff  36419552 Apr 10 18:36 test-dir/rustup-init

$ RUSTUP_HOME=rustup-home CARGO_HOME=rustup-home test-dir/rustup-init --no-modify-path -y
info: syncing channel updates for 'stable-x86_64-apple-darwin'
info: latest update on 2019-04-11, rust version 1.34.0 (91856ed52 2019-04-10)
info: downloading component 'rustc'

Running a symlink succeeds as well:

$ ls -l rustup-test/rustup-init
lrwxr-xr-x  1 lorenzo  staff  35 Apr 10 18:39 rustup-test/rustup-init -> /Users/lorenzo/test-dir/rustup-init

$ RUSTUP_HOME=rustup-home CARGO_HOME=rustup-home rustup-test/rustup-init --no-modify-path -y

I only seem to produce this error under the original conditions:

$ ls -l /usr/local/bin/rustup-init
lrwxr-xr-x  1 lorenzo  admin  44 Apr 10 17:53 /usr/local/bin/rustup-init -> ../Cellar/rustup-init/1.17.0/bin/rustup-init

$ ls -l /usr/local/Cellar/rustup-init/1.17.0/bin/
total 12440
-rwxr-xr-x  12 lorenzo  staff  6365960 Mar  5 18:50 rustup-init

$ RUSTUP_HOME=home CARGO_HOME=home rustup-init --no-modify-path -y
error: failed to set permissions for '/Users/m/home/bin/rustup'
info: caused by: Operation not permitted (os error 1)

[kinnison]

Hmm, looks like I was mistaken, the read-only directory only prevents directory changes, not changes to the inode data of entries inside it. Urgh, this makes life harder for trying to get this to a state we can CI it since I don't think our CI systems have multiple users we can set up for testing. Certainly it'd be unreasonable to expect that of random people running cargo test too ;(

It's possible we'll have to take a fix for the implementation without an easily automatable test for verifying we don't regress again.

For now, I suggest focussing on a fix, and once you have the fix we can see again if we can work out how to build a test for this. Sorry to have made a suggestion which turned out to not be helpful.

[lallotta]

No apologies necessary. I've learned something new.

My changes below appear to fix the issue. I aimed to check if the mode is ok already, and avoid calling set_permissions if so. I appreciate your advice if this can be improved. Thank you again.

https://github.com/rust-lang/rustup.rs/compare/master...lallotta:fix-1638?expand=1

[kinnison]

That patch LGTM as the bugfix itself. I'm happy to merge that if we can't come up with a good way to CI test it.