On 10/16/2014 12:14 AM, Pádraig Brady wrote: > On 10/15/2014 10:55 PM, Bernhard Voelker wrote: >> On 10/15/2014 07:17 PM, Pádraig Brady wrote: >>> I agree with your analysis and that we should revert >>> to the previous behavior here, which is done in >>> the attached patch. >> >> Hi Padraig, >> >> I also agree that chroot(1) should chroot(2) in such a case, but wouldn't >> be the obvious fix to STREQ() the canonicalized DIR against "/" rather >> than reverting the whole change - something like the following? >> >> Have a nice day, >> Berny >> >> diff --git a/src/chroot.c b/src/chroot.c >> index 171ced9..7f60106 100644 >> --- a/src/chroot.c >> +++ b/src/chroot.c >> @@ -175,7 +175,13 @@ is_root (const char* dir) >> error (EXIT_CANCELED, errno, _("failed to get attributes of %s"), >> quote (dir)); >> >> - return SAME_INODE (root_ino, arg_st); >> + if (! SAME_INODE (root_ino, arg_st)) >> + return false; >> + >> + char *resolved = canonicalize_file_name (dir); >> + bool is_res_root = resolved && STREQ ("/", resolved); >> + free (resolved); >> + return is_res_root; >> } > > Yes I considered that and it should work for this case. > BTW the inode check would then be moot right? > > Doing that would give better performance for --userspec=... --skip-chdir > and provide consistency for non root `chroot /` across platforms. > I'm worried though there are other edge cases we've not considered, > and the benefits above are not worth the risk? I suppose a compromise is to keep most of the better performance for: chroot --userspec=... --skip-chdir / but do the chroot(2) unconditionally. Updated patch is attached. thanks, Pádraig.