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

/bin/sh -> ${stdenv.shell} #21905

Closed
wants to merge 1 commit into from
Closed

/bin/sh -> ${stdenv.shell} #21905

wants to merge 1 commit into from

Conversation

rht
Copy link
Member

@rht rht commented Jan 15, 2017

Motivation for this change

Use ${stdenv.shell} in pkgs/build-support and pkgs/servers.

Address a subset of #183, #1424.

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@mention-bot
Copy link

@rht, thanks for your PR! By analyzing the history of the files in this pull request, we identified @lethalman, @viric and @rbvermaa to be potential reviewers.

@@ -82,7 +82,7 @@ rec {
export PATH=${shadow}/bin:$PATH
mkdir -p /etc/pam.d
if [[ ! -f /etc/passwd ]]; then
echo "root:x:0:0::/root:/bin/sh" > /etc/passwd
echo "root:x:0:0::/root:${stdenv.shell}" > /etc/passwd
Copy link
Member

@Mic92 Mic92 Jan 15, 2017

Choose a reason for hiding this comment

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

is this safe to use? I think there is a reason why on nixos the root login shell points to /run/current-system/sw/bin/bash. I could be that docker is special because the shell in nixos-based images are never updated.

Copy link
Member Author

Choose a reason for hiding this comment

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

(I have an insufficient information to figure out why, for docker's case)

Copy link
Member

Choose a reason for hiding this comment

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

IIRC you can only use paths from /etc/shells as a login shell. Universal paths like /run/current-system/... seem less likely to break that.

Copy link
Member

Choose a reason for hiding this comment

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

Though in a docker container the risk might be very low.

Copy link
Member

@7c6f434c 7c6f434c Jan 16, 2017

Choose a reason for hiding this comment

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

/etc/shells is only relevant for a non-root user doing chsh on herself; nologin is usually not in /etc/shells but works as a login-no-intended shell just fine.

Copy link
Member

Choose a reason for hiding this comment

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

Re: root shell: /bin/sh is non-interactive, too.

@@ -22,7 +22,7 @@ in {
runInWindowsVM = drv: let
newDrv = drv.override {
stdenv = drv.stdenv.override {
shell = "/bin/sh";
shell = "${stdenv.shell}";
Copy link
Contributor

@joachifm joachifm Jan 15, 2017

Choose a reason for hiding this comment

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

This one looks a bit odd to me, are you sure it is not deliberately using /bin/sh here?

Copy link
Member Author

Choose a reason for hiding this comment

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

It definitely looks deliberate -- there is a further check attrs.builder == attrs.stdenv.shell when defining origBuilder later on. The file was originally written by @aszlig, if @aszlig could quickly shed some light here.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is deliberate, because this is within a VM and within a cygwin shell and there is no access to the store.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see.

@7c6f434c
Copy link
Member

Manually merged to master.

@7c6f434c 7c6f434c closed this Apr 30, 2017
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.

None yet

7 participants