chrootenv: rewrite on top of GLib #33139

lukateras · 2017-12-28T15:23:12Z

Changes:

doesn't handle root user separately
doesn't chdir("/") which makes using it seamless
only bind mounts, doesn't symlink

Incidentally, it fixes #33106.

It's about two times shorter than the previous version (it's even shorter than the Ruby version), and much, much easier to read/follow through. It uses GLib quite heavily, along with RAII (available in GCC/Clang).

Symlinking already happens in init script, so it's redundant to have it in this code. It has only been in chrootenv for a week, and I might reintroduce it back later.

About closure size: Steam itself uses GLib, so it won't result in a larger closure for most chrootenv users. It's also likely to be required by other programs. I've tested Steam, and of course, it still works.

/cc @Mic92 @dezgeg @srhb @pbogdan

lukateras · 2017-12-28T22:52:09Z

I feel confident about new code and will merge on 31st if no one has any issues with it.

I should note that none of the changes to chrootenv are breaking buildFHSUserEnv behavior because root-specific logic was an optimization, and scripts that wrap chrootenv do chdir and symlink anyway.

orivej · 2017-12-29T05:16:39Z

pkgs/build-support/build-fhs-userenv/chrootenv/chrootenv.c

+
+    else if (WIFSIGNALED(status))
+      kill(getpid(), WTERMSIG(status));
+  }


It is not obvious that the equivalent of return EX_OSERR; of the old version is unneeded in the new version: the signal that terminated the child process may be ignored by default and not terminate chrootenv.

If signal is ignored, chrootenv will return 0. This seems to be logical behavior.

If the child overrode the default of ignoring a signal and died on receiving it, it did not exit cleanly and chrootenv should not return 0.

You are right, it's better to return 1 in this case. I've added it, see ac40a57.

FRidh · 2017-12-29T13:31:40Z

What do you think about documenting what this code is for and how it (roughly) works. I think each piece of code should explain why it is needed. So, what problem does this code solve?

Looking at the older version, there were some comments that helps the reader understand what is happening.

lukateras · 2017-12-29T16:49:40Z

There are only two comments that still apply, specifically "Create new mount and user namespaces" and "Map users and groups to the parent namespace". I don't think these are particularly useful to keep around, because people who know about mount and user namespaces probably know about unshare anyway. If not, then man 2 unshare provides better documentation than these comments.

So, what problem does this code solve?

Setting up a chroot to ultimately run programs that assume a FHS-compliant environment.

FRidh · 2017-12-29T16:58:45Z

Setting up a chroot to ultimately run programs that assume a FHS-compliant environment.

That at the very least should be included in the code header.

lukateras · 2017-12-29T17:00:41Z

I'll add something like it to meta.description, thanks.

dtzWill · 2017-12-29T18:14:22Z

According to docs it seems g_error calls abort instead of exit which results in a coredump and is not intended for user-facing errors.

This seems inappropriate for at least the error path that prints messages to the user about running sysctl.

Maybe make a pass with this in mind, or instead adjust the g_perr macro to just log and exit instead?

lukateras · 2017-12-29T18:18:03Z

abort is correct. Also, failed unshare might be a user configuration problem, or might be not, and coredump might be useful.

dtzWill · 2017-12-29T18:20:34Z

Not to bikeshed but I can't help but wonder how much of benefit glib brings here compared to C++ or something (g_autofree patterns, etc.)?

Regardless I'd appreciate, if for my curiosity if nothing else, a mention of the role glib plays in obtaining the improvements in this PR or to what extent they're orthogonal?

lukateras · 2017-12-29T18:31:05Z

I think C++ is a bad language because it's very complex.

g_autofree is negligible convenience, and improvement mostly comes from utility functions: g_build_filename, g_mkdtemp_full (handles chmod), g_file_test, g_dir_open (doesn't return . and .. unlike opendir), g_strv_contains.

dtzWill · 2017-12-29T18:35:54Z

Okay, thanks for the comments and consideration of abort. LGTM review-wise.

lukateras · 2017-12-29T18:36:55Z

Thanks for reviewing it!

orivej · 2017-12-30T03:24:32Z

pkgs/build-support/build-fhs-userenv/chrootenv/chrootenv.c

+#include <unistd.h>
+
+#define g_perr(s, err) g_error("%s#L%d: %s: %s", __func__, __LINE__, s, g_strerror(err))
+#define g_perrno(s) g_perr(s, errno)


g_ is a glib namespace, you should not define names starting with g_.

Sure, and names themselves are misleading (perror doesn't abort). I've pushed a new version with fail and fail_if macros: I think that separates concerns much better than previously used error macros, and names are much more straightforward. fail_if also cuts some boilerplate.

Changes: * doesn't handle root user separately * doesn't chdir("/") which makes using it seamless * only bind mounts, doesn't symlink (i.e. files) Incidentally, fixes NixOS#33106. It's about two times shorter than the previous version, and much easier to read/follow through. It uses GLib quite heavily, along with RAII (available in GCC/Clang).

Mic92

I have not tested the code itself, but the rest looks good to me.

lukateras · 2017-12-31T00:18:56Z

@Mic92: Thanks for mentioning __cleanup__ GCC/Clang attribute on IRC!

lukateras · 2017-12-31T03:57:53Z

@orivej: Thanks for the review!

I still want to implement a different order of messages in case of failed unshare, and the only clean way that I could come up with is via a SIGABRT or SIGTRAP handler. The problem is that if I use GLib logging from the handler, call to any logging function results in recursion warning and a subsequent abort (which is technically what I want to do anyway, but it doesn't let me print two messages).

GrahamcOfBorg added 10.rebuild-darwin: 0 10.rebuild-linux: 11-100 labels Dec 28, 2017

lukateras force-pushed the 20171228.053707/chrootenv branch from 9307ec2 to be611fd Compare December 28, 2017 17:03

orivej reviewed Dec 29, 2017

View reviewed changes

lukateras force-pushed the 20171228.053707/chrootenv branch 3 times, most recently from a0d1430 to 430476a Compare December 29, 2017 08:23

orivej reviewed Dec 30, 2017

View reviewed changes

lukateras force-pushed the 20171228.053707/chrootenv branch 2 times, most recently from a1182bd to 37da1a8 Compare December 30, 2017 18:26

lukateras force-pushed the 20171228.053707/chrootenv branch from 37da1a8 to 4b1cf5a Compare December 30, 2017 22:28

Mic92 approved these changes Dec 30, 2017

View reviewed changes

lukateras merged commit 60a133f into NixOS:master Dec 31, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chrootenv: rewrite on top of GLib #33139

chrootenv: rewrite on top of GLib #33139

lukateras commented Dec 28, 2017 •

edited

lukateras commented Dec 28, 2017 •

edited

orivej Dec 29, 2017

lukateras Dec 29, 2017 •

edited

orivej Dec 29, 2017

lukateras Dec 29, 2017

FRidh commented Dec 29, 2017

lukateras commented Dec 29, 2017 •

edited

FRidh commented Dec 29, 2017

lukateras commented Dec 29, 2017 •

edited

dtzWill commented Dec 29, 2017

lukateras commented Dec 29, 2017 •

edited

dtzWill commented Dec 29, 2017

lukateras commented Dec 29, 2017 •

edited

dtzWill commented Dec 29, 2017

lukateras commented Dec 29, 2017

orivej Dec 30, 2017

lukateras Dec 30, 2017 •

edited

Mic92 left a comment

lukateras commented Dec 31, 2017 •

edited

lukateras commented Dec 31, 2017 •

edited

chrootenv: rewrite on top of GLib #33139

chrootenv: rewrite on top of GLib #33139

Conversation

lukateras commented Dec 28, 2017 • edited

lukateras commented Dec 28, 2017 • edited

orivej Dec 29, 2017

Choose a reason for hiding this comment

lukateras Dec 29, 2017 • edited

Choose a reason for hiding this comment

orivej Dec 29, 2017

Choose a reason for hiding this comment

lukateras Dec 29, 2017

Choose a reason for hiding this comment

FRidh commented Dec 29, 2017

lukateras commented Dec 29, 2017 • edited

FRidh commented Dec 29, 2017

lukateras commented Dec 29, 2017 • edited

dtzWill commented Dec 29, 2017

lukateras commented Dec 29, 2017 • edited

dtzWill commented Dec 29, 2017

lukateras commented Dec 29, 2017 • edited

dtzWill commented Dec 29, 2017

lukateras commented Dec 29, 2017

orivej Dec 30, 2017

Choose a reason for hiding this comment

lukateras Dec 30, 2017 • edited

Choose a reason for hiding this comment

Mic92 left a comment

Choose a reason for hiding this comment

lukateras commented Dec 31, 2017 • edited

lukateras commented Dec 31, 2017 • edited

lukateras commented Dec 28, 2017 •

edited

lukateras commented Dec 28, 2017 •

edited

lukateras Dec 29, 2017 •

edited

lukateras commented Dec 29, 2017 •

edited

lukateras commented Dec 29, 2017 •

edited

lukateras commented Dec 29, 2017 •

edited

lukateras commented Dec 29, 2017 •

edited

lukateras Dec 30, 2017 •

edited

lukateras commented Dec 31, 2017 •

edited

lukateras commented Dec 31, 2017 •

edited