bug#18736: chroot regression - chroot avoids the chroot() call too eagerly.

[Bernhard Voelker]

Hi Padraig,

On 10/16/2014 02:05 AM, Pádraig Brady wrote:
>  From d520929586ee2063d48359aaaef8f28807604cae Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?P=C3=A1draig=20Brady?=<address@hidden>
> 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.

I'm not sure that introducing the inconsistency again is the right
way to go, although I don't have a strong preference (40:60):
I'd probably go with the smaller change in is_root() and wait for
other edge cases to come (which I think is improbable).

> 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

Minor nit: it's better to initialize 'can_chroot_root' here.

Thanks & have a nice day,
Berny