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

[Pádraig Brady]

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?=<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).

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.