On 05/14/2014 01:02 AM, Pádraig Brady wrote:
> On 05/13/2014 11:45 PM, Bernhard Voelker wrote:
>> On 05/13/2014 05:14 PM, Pádraig Brady wrote:
>>> From 9c1bad82852cec8403ead49f12f53280c468a2cf Mon Sep 17 00:00:00 2001
>>> From: =?UTF-8?q?P=C3=A1draig=20Brady?= <p...@draigbrady.com>
>>> Date: Tue, 13 May 2014 15:56:34 +0100
>>> Subject: [PATCH] chroot: don't chdir() if not changing root
>>>
>>> This allows chroot to use used as a light weight tool
>>> to change user identification for a command,
>>> while not changing the current working directory.
>>>
>>> * src/chroot.c (main): If the same root is specified. i.e. '/'
>>> then don't change the current working directory, and avoid the
>>> overhead of the other redundant calls.
>>
>> Looks good, but deserves a NEWS entry, doesn't it?
>
> Yep as a "Change in behavior".
> Also needs tests.
>
> Just popping it out as an RFC at this stage.
>
> Thanks for the opinion.
>
> cheers,
> Pádraig.
>
>
Full patch attached.
thanks,
Pádraig
>From 17a185c8e02a56fa09f91ce97151d4e071ecf236 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <p...@draigbrady.com>
Date: Tue, 13 May 2014 15:56:34 +0100
Subject: [PATCH] chroot: don't chdir() if not changing root
This allows chroot to use used as a light weight tool
to change user identification for a command,
while not changing the current working directory.
It also makes `chroot / true` consistently succeed on
all platforms for non root users.
* src/chroot.c (main): If the same root is specified. i.e. '/'
then don't change the current working directory, and avoid the
overhead of the other redundant calls.
* tests/misc/chroot-fail.sh: Remove failure guard previously
needed on some systems. Also add an explicit case to ensure
we don't change directory.
* NEWS: Mention the change in behavior.
---
NEWS | 3 +++
src/chroot.c | 36 +++++++++++++++++++++---------------
tests/misc/chroot-fail.sh | 24 +++++++++++++-----------
3 files changed, 37 insertions(+), 26 deletions(-)
diff --git a/NEWS b/NEWS
index 4efd60d..35d0bda 100644
--- a/NEWS
+++ b/NEWS
@@ -80,6 +80,9 @@ GNU coreutils NEWS -*- outline -*-
** Changes in behavior
+ chroot with an argument of "/" no longer implicitly changes the current
+ directory to "/", allowing changing only user credentials for a command.
+
ls with none of LS_COLORS or COLORTERM environment variables set,
will now honor an empty or unknown TERM environment variable,
and not output colors even with --colors=always.
diff --git a/src/chroot.c b/src/chroot.c
index 8b08b84..a2debac 100644
--- a/src/chroot.c
+++ b/src/chroot.c
@@ -225,21 +225,27 @@ main (int argc, char **argv)
usage (EXIT_CANCELED);
}
- /* We have to look up users and groups twice.
- - First, outside the chroot to load potentially necessary passwd/group
- parsing plugins (e.g. NSS);
- - Second, inside chroot to redo the parsing in case IDs are different. */
- if (userspec)
- ignore_value (parse_user_spec (userspec, &uid, &gid, NULL, NULL));
- if (groups && *groups)
- ignore_value (parse_additional_groups (groups, &out_gids, &n_gids, false));
-
- if (chroot (argv[optind]) != 0)
- error (EXIT_CANCELED, errno, _("cannot change root directory to %s"),
- argv[optind]);
-
- if (chdir ("/"))
- error (EXIT_CANCELED, errno, _("cannot chdir to root directory"));
+ /* Only do chroot specific actions if actually changing root.
+ The main difference here is that we don't change working dir. */
+ if (! STREQ (argv[optind], "/"))
+ {
+ /* We have to look up users and groups twice.
+ - First, outside the chroot to load potentially necessary passwd/group
+ parsing plugins (e.g. NSS);
+ - Second, inside chroot to redo parsing in case IDs are different. */
+ if (userspec)
+ ignore_value (parse_user_spec (userspec, &uid, &gid, NULL, NULL));
+ if (groups && *groups)
+ ignore_value (parse_additional_groups (groups, &out_gids, &n_gids,
+ false));
+
+ if (chroot (argv[optind]) != 0)
+ error (EXIT_CANCELED, errno, _("cannot change root directory to %s"),
+ argv[optind]);
+
+ if (chdir ("/"))
+ error (EXIT_CANCELED, errno, _("cannot chdir to root directory"));
+ }
if (argc == optind + 1)
{
diff --git a/tests/misc/chroot-fail.sh b/tests/misc/chroot-fail.sh
index 11f0345..56be8e2 100755
--- a/tests/misc/chroot-fail.sh
+++ b/tests/misc/chroot-fail.sh
@@ -28,16 +28,18 @@ test $? = 125 || fail=1
chroot --- / true # unknown option
test $? = 125 || fail=1
-# chroot("/") succeeds for non-root users on some systems, but not all.
-if chroot / true ; then
- 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
+# Note chroot("/") succeeds for non-root users on some systems, but not all,
+# however we avoid the chroot() with "/" to have common behvavior.
+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
+
+# Ensure we don't chdir("/") when not changing root
+# to allow only changing user ids for a command.
+curdir=$(chroot / env pwd) || fail=1
+test "$curdir" = '/' && fail=1
Exit $fail
--
1.7.7.6
