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

Improvements for FHS user chrootenv #16030

Merged
merged 5 commits into from Jun 11, 2016
Merged

Conversation

abbradar
Copy link
Member

@abbradar abbradar commented Jun 7, 2016

Motivation for this change

This takes another approach at binding FHS directory structure. We
now bind-mount all the root filesystem to directory "/host" in the target tree.
From that we symlink all the directories into the tree if they do not already
exist in FHS structure.

This probably makes CHROOTENV_EXTRA_BINDS unnecessary -- its main usecase was
to add bound directories from the host to the sandbox, and we now just symlink
all of them. I plan to get some feedback on its usage and maybe deprecate it.

This also drops old buildFHSChrootEnv infrastructure. The main problem with it
is it's very difficult to unmount a recursive-bound directory when mount is not
sandboxed. This problem is a bug even without these changes -- if
you have for example /home/alice mounted to somewhere, you wouldn't see
it in buildFHSChrootEnv now. With the new directory structure, it's
impossible to use regular bind at all. After some tackling with this I realized
that the fix would be brittle and dangerous (if you don't unmount everything
cleanly and proceed to removing the temporary directory, bye-bye fs!). It also
probably doesn't worth it because I haven't heard that someone actually uses it
for a long time, and buildFHSUserEnv should cover most cases while being much
more maintainable and safe for the end-user.

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
    • OS X
    • 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.

cc @bjornfor (concerning CHROOTENV_EXTRA_BINDS)

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @svanderburg, @hrdinka and @bjornfor to be potential reviewers

This takes another approach at binding FHS directory structure. We
now bind-mount all the root filesystem to directory "/host" in the target tree.
From that we symlink all the directories into the tree if they do not already
exist in FHS structure.

This probably makes `CHROOTENV_EXTRA_BINDS` unnecessary -- its main usecase was
to add bound directories from the host to the sandbox, and we not just symlink
all of them. I plan to get some feedback on its usage and maybe deprecate it.

This also drops old `buildFHSChrootEnv` infrastructure. The main problem with it
is it's very difficult to unmount a recursive-bound directory when mount is not
sandboxed. This problem is a bug even without these changes -- if
you have for example `/home/alice` mounted to somewhere, you wouldn't see
it in `buildFHSChrootEnv` now. With the new directory structure, it's
impossible to use regular bind at all. After some tackling with this I realized
that the fix would be brittle and dangerous (if you don't unmount everything
clearly and proceed to removing the temporary directory, bye-bye fs!). It also
probably doesn't worth it because I haven't heard that someone actually uses it
for a long time, and `buildFHSUserEnv` should cover most cases while being much
more maintainable and safe for the end-user.
@abbradar
Copy link
Member Author

abbradar commented Jun 7, 2016

BTW I have an experimental branch which uses /etc/ld-nix.so.cache instead of LD_LIBRARY_PATH to find libraries. It is more close to what happens on actual FHS systems and survives environment cleanups, but I haven't yet found a use case when it's needed (I thought I did but it turned out the problem was somewhere else) so I left this as is for now. If someone wants it, ping me!

@abbradar
Copy link
Member Author

abbradar commented Jun 7, 2016

Oh, and this also takes another non-trivial chunk off chroot-user.rb, bringing it closer to a point when I overcome my laziness and rewrite it to something more lightweight (i.e. C).

@bjornfor
Copy link
Contributor

bjornfor commented Jun 7, 2016

Hi @abbradar.

Although I don't understand all the code, it looks like a nice cleanup (lots of removed code)! I tested it on NixOS and Ubuntu and it works fine.

I don't mind if you drop CHROOTENV_EXTRA_BINDS, because you've (bind)mounted everything already. At first I worried if more things would leak from host to env (like /usr/ or other places where binaries and libraries might be located), but it seems safe. The scary paths are handled by the env, the rest that is mapped from the host is just "convenient" (like I used CHROOTENV_EXTRA_BINDS to access extra disk before).

Observations:

  • I had to add "gcc" in my expression (I build software in my FHS env). A comment in the commit message, why you remove gcc, would be appreciated. Is it just that it's not intended for building? I can add "gcc" to my expression now already (being explicit doesn't hurt), so I don't mind the change. Just curious.
  • Due to multiple outputs change, I have to add "dev" to extraOutputsToInstall. I see you have a commit that adds "bin" to the default extraOutputsToInstall, but I guess "dev" is something users (me) should add themself, instead of having it on by default.

@abbradar
Copy link
Member Author

abbradar commented Jun 7, 2016

@bjornfor Yes, GCC-the-compiler is removed to save disk space, likewise absence of dev outputs. I expect that there is non-negligible amount of those who use buildFHSUserEnv just to run software, and while you can add gcc or dev outputs trivially in your expression, it's impossible to not install them if they are listed by default.

Concerning CHROOTENV_EXTRA_BINDS -- I'll make it so some message like "discussed for deprecation; if you use it, drop a note in this issue" is printed for now.

@bjornfor
Copy link
Contributor

bjornfor commented Jun 7, 2016

@abbradar: Thanks for info. +1 for merge.

@abbradar
Copy link
Member Author

abbradar commented Jun 7, 2016

I'll merge it in several days to leave more room for discussion. Thanks for review!

@domenkozar
Copy link
Member

Do we need to update documentation? http://nixos.org/nixpkgs/manual/#sec-fhs-environments

@abbradar
Copy link
Member Author

abbradar commented Jun 7, 2016

Yes! Thanks for the reminder.

@hrdinka
Copy link
Contributor

hrdinka commented Jun 7, 2016

👍 for the changes and it doesn't break anything for me.

@abbradar
Copy link
Member Author

abbradar commented Jun 9, 2016

I've updated the documentation. (Imaginary) timer till merge re-started ^_^

@abbradar abbradar merged commit b341de8 into NixOS:master Jun 11, 2016
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/hardcoded-opt-paths-in-binary-files/14661/4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants