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.
>From d520929586ee2063d48359aaaef8f28807604cae Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email protected]>
Date: Wed, 15 Oct 2014 18:08:42 +0100
Subject: [PATCH] chroot: call chroot() unconditionally to handle bind mounted
"/"
* src/chroot.c (is_root): Adjust to compare canonicalized paths
rather than inodes, to handle (return false in) the case where
we have a tree that is constructed by first bind mounting "/"
(thus having the same inode).
(main): Unconditionally call chroot() because it's safer
and of minimal performance benefit to avoid in this case.
This will cause inconsistency with some platforms
not allowing `chroot / true` for non root users.
* tests/misc/chroot-fail.sh: Adjust appropriately.
* NEWS: Mention the bug fixes.
Fixes http://bugs.gnu.org/18736
---
NEWS | 11 ++++-----
src/chroot.c | 29 +++++++++++----------------
tests/misc/chroot-fail.sh | 46 +++++++++++++++++++++++++-------------------
3 files changed, 43 insertions(+), 43 deletions(-)
diff --git a/NEWS b/NEWS
index 52332bd..5fbdc6a 100644
--- a/NEWS
+++ b/NEWS
@@ -7,6 +7,11 @@ GNU coreutils NEWS -*- outline -*-
dd supports more robust SIGINFO/SIGUSR1 handling for outputting statistics.
Previously those signals may have inadvertently terminated the process.
+ chroot again calls chroot(DIR) and chdir("/"), even if DIR is "/".
+ This handles separate bind mounted "/" trees, and environments
+ depending on the implicit chdir("/").
+ [bugs introduced in coreutils-8.23]
+
cp no longer issues an incorrect warning about directory hardlinks when a
source directory is specified multiple times. Now, consistent with other
file types, a warning is issued for source directories with duplicate names,
@@ -25,12 +30,6 @@ GNU coreutils NEWS -*- outline -*-
dd accepts a new status=progress level to print data transfer statistics
on stderr approximately every second.
-** Changes in behavior
-
- chroot changes the current directory to "/" in again - unless the above new
- --skip-chdir option is specified.
- [bug introduced in coreutils-8.23]
-
** Improvements
cp,install,mv will convert smaller runs of NULs in the input to holes,
diff --git a/src/chroot.c b/src/chroot.c
index 171ced9..3aacafa 100644
--- a/src/chroot.c
+++ b/src/chroot.c
@@ -162,20 +162,17 @@ parse_additional_groups (char const *groups, GETGROUPS_T **pgids,
return ret;
}
+/* Return whether the passed path is equivalent to "/".
+ Note we don't compare against get_root_dev_ino() as "/"
+ could be bind mounted to a separate location. */
+
static bool
is_root (const char* dir)
{
- struct dev_ino root_ino;
- if (! get_root_dev_ino (&root_ino))
- error (EXIT_CANCELED, errno, _("failed to get attributes of %s"),
- quote ("/"));
-
- struct stat arg_st;
- if (stat (dir, &arg_st) == -1)
- error (EXIT_CANCELED, errno, _("failed to get attributes of %s"),
- quote (dir));
-
- return SAME_INODE (root_ino, arg_st);
+ char *resolved = canonicalize_file_name (dir);
+ bool is_res_root = resolved && STREQ ("/", resolved);
+ free (resolved);
+ return is_res_root;
}
void
@@ -291,8 +288,6 @@ main (int argc, char **argv)
usage (EXIT_CANCELED);
}
- /* Only do chroot specific actions if actually changing root.
- The main difference here is that we don't change working dir. */
if (! is_oldroot)
{
/* We have to look up users and groups twice.
@@ -328,12 +323,12 @@ main (int argc, char **argv)
n_gids = ngroups;
}
#endif
-
- if (chroot (newroot) != 0)
- error (EXIT_CANCELED, errno, _("cannot change root directory to %s"),
- newroot);
}
+ if (chroot (newroot) != 0)
+ error (EXIT_CANCELED, errno, _("cannot change root directory to %s"),
+ newroot);
+
if (! skip_chdir && chdir ("/"))
error (EXIT_CANCELED, errno, _("cannot chdir to root directory"));
diff --git a/tests/misc/chroot-fail.sh b/tests/misc/chroot-fail.sh
index 82ae23c..75f724a 100755
--- a/tests/misc/chroot-fail.sh
+++ b/tests/misc/chroot-fail.sh
@@ -29,14 +29,18 @@ test $? = 125 || fail=1
chroot --- / true # unknown option
test $? = 125 || fail=1
-# Note chroot("/") succeeds for non-root users on some systems, but not all,
-# however we avoid the chroot() with "/" to have common behavior.
-chroot / sh -c 'exit 2' # exit status propagation
-test $? = 2 || fail=1
-chroot / . # invalid command
-test $? = 126 || fail=1
-chroot / no_such # no such command
-test $? = 127 || fail=1
+# chroot("/") succeeds for non-root users on some systems, but not all.
+if chroot / true ; then
+ can_chroot_root=1
+ chroot / sh -c 'exit 2' # exit status propagation
+ test $? = 2 || fail=1
+ chroot / . # invalid command
+ test $? = 126 || fail=1
+ chroot / no_such # no such command
+ test $? = 127 || fail=1
+else
+ test $? = 125 || fail=1
+fi
# Ensure that --skip-chdir fails with a non-"/" argument.
cat <<\EOF > exp || framework_failure_
@@ -47,17 +51,19 @@ chroot --skip-chdir . env pwd >out 2>err && fail=1
compare /dev/null out || fail=1
compare exp err || fail=1
-# Ensure we don't chroot("/") when NEWROOT is old "/".
-ln -s / isroot || framework_failure_
-for dir in '/' '/.' '/../' isroot; do
- # Verify that chroot(1) succeeds and performs chdir("/")
- # (chroot(1) of coreutils-8.23 failed to run the latter).
- curdir=$(chroot "$dir" env pwd) || fail=1
- test "$curdir" = '/' || fail=1
-
- # Test the "--skip-chdir" option.
- curdir=$(chroot --skip-chdir "$dir" env pwd) || fail=1
- test "$curdir" = '/' && fail=1
-done
+# Ensure we chdir("/") appropriately when NEWROOT is old "/".
+if test "$can_chroot_root"; then
+ ln -s / isroot || framework_failure_
+ for dir in '/' '/.' '/../' isroot; do
+ # Verify that chroot(1) succeeds and performs chdir("/")
+ # (chroot(1) of coreutils-8.23 failed to run the latter).
+ curdir=$(chroot "$dir" env pwd) || fail=1
+ test "$curdir" = '/' || fail=1
+
+ # Test the "--skip-chdir" option.
+ curdir=$(chroot --skip-chdir "$dir" env pwd) || fail=1
+ test "$curdir" = '/' && fail=1
+ done
+fi
Exit $fail
--
1.7.7.6
