GNU bug report logs - #18736
chroot regression - chroot avoids the chroot() call too eagerly.

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 18736 in the body.
You can then email your comments to 18736 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Rogier <rogier777 <at> gmail.com>
To: bug-coreutils <at> gnu.org
Subject: chroot regression - chroot avoids the chroot() call too eagerly.
Date: Wed, 15 Oct 2014 11:40:16 +0200

Hi,

Since a few months, it seems that chroot has started avoiding the 
chroot call if it can be determined to be idempotent. 
It looks like the new check is based on inode comparison - if the 
inode is the same, the chroot() call is considered idempotent, and not 
performed.

However, while two directories being in the same file system and having 
the same inode number implies that they are the same directory, it 
does not necessarily imply that they have same file system path (i.e. 
use the same mountpoint), so there is no guarantee that the entire 
directory trees rooted at the two directories are also identical even 
though the directories are.This means that chroot will fail to 
chroot() in cases when the call would in fact not be idempotent.

On my system (debian testing), I have / bind-mounted elsewhere, with a 
slightly different directory tree mounted beneath it, as is mounted 
beneath /. In my case, I need this so that I can make partial backups 
of the system - including some file systems but leaving out others. 
Undoubtly, there are other use-cases as well - I wouldn't be surprised 
if users of libpam-chroot can be affected by this, depending on exactly 
how they configure their chroot environments.

The new chroot behavior no longer allows chrooting into such alternate 
trees. In my case, that means that my backup fails.

Kind regards,

Rogier.

Message #8 received at 18736 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Rogier <rogier777 <at> gmail.com>
Cc: 18736 <at> debbugs.gnu.org
Subject: Re: bug#18736: chroot regression - chroot avoids the chroot() call
 too eagerly.
Date: Wed, 15 Oct 2014 18:17:12 +0100

[Message part 1 (text/plain, inline)]

On 10/15/2014 10:40 AM, Rogier wrote:
> Hi,
> 
> Since a few months, it seems that chroot has started avoiding the 
> chroot call if it can be determined to be idempotent. 
> It looks like the new check is based on inode comparison - if the 
> inode is the same, the chroot() call is considered idempotent, and not 
> performed.
> 
> However, while two directories being in the same file system and having 
> the same inode number implies that they are the same directory, it 
> does not necessarily imply that they have same file system path (i.e. 
> use the same mountpoint), so there is no guarantee that the entire 
> directory trees rooted at the two directories are also identical even 
> though the directories are.This means that chroot will fail to 
> chroot() in cases when the call would in fact not be idempotent.
> 
> On my system (debian testing), I have / bind-mounted elsewhere, with a 
> slightly different directory tree mounted beneath it, as is mounted 
> beneath /. In my case, I need this so that I can make partial backups 
> of the system - including some file systems but leaving out others. 
> Undoubtly, there are other use-cases as well - I wouldn't be surprised 
> if users of libpam-chroot can be affected by this, depending on exactly 
> how they configure their chroot environments.
> 
> The new chroot behavior no longer allows chrooting into such alternate 
> trees. In my case, that means that my backup fails.

I didn't consider this unusual case when avoiding the chroot() call,
for efficiency (especially in the --userspec case), and to add
consistency between platforms in the handling of `chroot / true`
for non root users.

I agree with your analysis and that we should revert
to the previous behavior here, which is done in
the attached patch.

thanks!
Pádraig.

[chroot-always.patch (text/x-patch, attachment)]

Message #11 received at 18736 <at> debbugs.gnu.org (full text, mbox):

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Pádraig Brady <P <at> draigBrady.com>, 
 Rogier <rogier777 <at> gmail.com>
Cc: 18736 <at> debbugs.gnu.org
Subject: Re: bug#18736: chroot regression - chroot avoids the chroot() call
 too eagerly.
Date: Wed, 15 Oct 2014 23:55:11 +0200

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;
 }

 void
-- 
1.8.4.5

Message #14 received at 18736 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Bernhard Voelker <mail <at> bernhard-voelker.de>
Cc: 18736 <at> debbugs.gnu.org, Rogier <rogier777 <at> gmail.com>
Subject: Re: bug#18736: chroot regression - chroot avoids the chroot() call
 too eagerly.
Date: Thu, 16 Oct 2014 00:14:35 +0100

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?

Pádraig.

Message #17 received at 18736 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Bernhard Voelker <mail <at> bernhard-voelker.de>
Cc: 18736 <at> debbugs.gnu.org, Rogier <rogier777 <at> gmail.com>
Subject: Re: bug#18736: chroot regression - chroot avoids the chroot() call
 too eagerly.
Date: Thu, 16 Oct 2014 01:05:21 +0100

[Message part 1 (text/plain, inline)]

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.

[chroot-always.patch (text/x-patch, attachment)]

Message #20 received at 18736 <at> debbugs.gnu.org (full text, mbox):

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 18736 <at> debbugs.gnu.org, Rogier <rogier777 <at> gmail.com>
Subject: Re: bug#18736: chroot regression - chroot avoids the chroot() call
 too eagerly.
Date: Thu, 16 Oct 2014 09:38:07 +0200

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?=<P <at> draigBrady.com>
> 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

Message #23 received at 18736 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Bernhard Voelker <mail <at> bernhard-voelker.de>
Cc: 18736 <at> debbugs.gnu.org, Rogier <rogier777 <at> gmail.com>
Subject: Re: bug#18736: chroot regression - chroot avoids the chroot() call
 too eagerly.
Date: Thu, 16 Oct 2014 11:02:07 +0100

On 10/16/2014 08:38 AM, Bernhard Voelker wrote:
> 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?=<P <at> draigBrady.com>
>> 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).

I don't have a strong preference either but would be (60:40) the other way.
In my mind they comes under the "it's better to ask forgiveness than permission" idea.
I.E. it's better to invoke lower layer logic where possible, rather than
adding higher level logic. Given it's more risky to avoid the chroot()
I'd be inclined to wait until there were complaints about the inconsistent
behavior rather than the other way around.


>> diff --git a/tests/misc/chroot-fail.sh b/tests/misc/chroot-fail.sh

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

Good point. Done.

thanks,
Pádraig.

Message #26 received at 18736 <at> debbugs.gnu.org (full text, mbox):

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 18736 <at> debbugs.gnu.org, Rogier <rogier777 <at> gmail.com>
Subject: Re: bug#18736: chroot regression - chroot avoids the chroot() call
 too eagerly.
Date: Thu, 16 Oct 2014 12:30:51 +0200

On 10/16/2014 12:02 PM, Pádraig Brady wrote:
> I don't have a strong preference either but would be (60:40) the other way.
> In my mind they comes under the "it's better to ask forgiveness than permission" idea.
> I.E. it's better to invoke lower layer logic where possible, rather than
> adding higher level logic. Given it's more risky to avoid the chroot()
> I'd be inclined to wait until there were complaints about the inconsistent
> behavior rather than the other way around.

Another possibility would be to suppress issuing an error diagnostic
for a chroot() failure when DIR is "/" ...

Have a nice day,
Berny

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.