bug#7320: [PATCH] 'id' prints incorrectly groups for the session
On 06/26/2014 05:42 PM, Jim Meyering wrote: > On Thu, Jun 26, 2014 at 3:23 AM, Pádraig Brady wrote: >> -g=$(id -u $NON_ROOT_USERNAME) || framework_failure_ >> +u=$(id -u $NON_ROOT_USERNAME) || framework_failure_ >> +g=u > > This will work better :-) >g=$u It's already pushed with that amendment as it luckily failed with the above :) cheers, Pádraig.
bug#7320: [PATCH] 'id' prints incorrectly groups for the session
On Thu, Jun 26, 2014 at 3:23 AM, Pádraig Brady wrote: > -g=$(id -u $NON_ROOT_USERNAME) || framework_failure_ > +u=$(id -u $NON_ROOT_USERNAME) || framework_failure_ > +g=u This will work better :-) g=$u
bug#7320: [PATCH] 'id' prints incorrectly groups for the session
On 06/26/2014 06:44 AM, Bernhard Voelker wrote:
> On 06/26/2014 03:53 AM, Pádraig Brady wrote:
>> diff --git a/tests/id/setgid.sh b/tests/id/setgid.sh
>> index aa43ea3..a81b42c 100755
>> --- a/tests/id/setgid.sh
>> +++ b/tests/id/setgid.sh
>> @@ -1,5 +1,5 @@
>> #!/bin/sh
>> -# Verify that id -G prints the right group when run set-GID.
>> +# Verify that id [-G] prints the right group when run set-GID.
>>
>> # Copyright (C) 2012-2014 Free Software Foundation, Inc.
>>
>> @@ -27,9 +27,14 @@ gp1=$(expr $g + 1)
>>
>> echo $gp1 > exp || framework_failure_
>>
>> +# With coreutils-8.16 and earlier, id -G would print both: $gp1 $g
>> chroot --user=$NON_ROOT_USERNAME:$gp1 --groups='' / env PATH="$PATH" \
>
> shouldn't we better avoid group name resolution here?
>
> - chroot --user=$NON_ROOT_USERNAME:$gp1 --groups='' / env PATH="$PATH" \
> + chroot --user=$NON_ROOT_USERNAME:+$gp1 --groups='' / env PATH="$PATH" \
>
>>id -G > out || fail=1
>> compare exp out || fail=1
>> -# With coreutils-8.16 and earlier, id -G would print both: $gp1 $g
>> +
>> +# With coreutils-8.22 and earlier, id would erroneously print groups=$g
>> +chroot --user=$NON_ROOT_USERNAME:$gp1 --groups='' / env PATH="$PATH" \
>
> Likewise.
>
>> + id > out || fail=1
>> +grep -F "groups=$gp1" out || fail=1
>>
>> Exit $fail
>
> Another minor nit:
> for a better diagnostic, it'd be better to use the construct
> we already use in many places:
>
> ... || { cat out; fail=1; }
Cool, that saves 2 syscalls per id invocation.
We can save another 2 by also using + for the user.
Pushed with these changes.
thanks!
Pádraig.
diff --git a/tests/id/setgid.sh b/tests/id/setgid.sh
index a81b42c..e0543f4 100755
--- a/tests/id/setgid.sh
+++ b/tests/id/setgid.sh
@@ -20,7 +20,8 @@
print_ver_ id
require_root_
-g=$(id -u $NON_ROOT_USERNAME) || framework_failure_
+u=$(id -u $NON_ROOT_USERNAME) || framework_failure_
+g=u
# Construct a different group number.
gp1=$(expr $g + 1)
@@ -28,13 +29,13 @@ gp1=$(expr $g + 1)
echo $gp1 > exp || framework_failure_
# With coreutils-8.16 and earlier, id -G would print both: $gp1 $g
-chroot --user=$NON_ROOT_USERNAME:$gp1 --groups='' / env PATH="$PATH" \
+chroot --user=+$u:+$gp1 --groups='' / env PATH="$PATH" \
id -G > out || fail=1
-compare exp out || fail=1
+compare exp out || { cat out; fail=1; }
# With coreutils-8.22 and earlier, id would erroneously print groups=$g
-chroot --user=$NON_ROOT_USERNAME:$gp1 --groups='' / env PATH="$PATH" \
+chroot --user=+$u:+$gp1 --groups='' / env PATH="$PATH" \
id > out || fail=1
-grep -F "groups=$gp1" out || fail=1
+grep -F "groups=$gp1" out || { cat out; fail=1; }
Exit $fail
bug#7320: [PATCH] 'id' prints incorrectly groups for the session
On 06/26/2014 03:53 AM, Pádraig Brady wrote:
> diff --git a/tests/id/setgid.sh b/tests/id/setgid.sh
> index aa43ea3..a81b42c 100755
> --- a/tests/id/setgid.sh
> +++ b/tests/id/setgid.sh
> @@ -1,5 +1,5 @@
> #!/bin/sh
> -# Verify that id -G prints the right group when run set-GID.
> +# Verify that id [-G] prints the right group when run set-GID.
>
> # Copyright (C) 2012-2014 Free Software Foundation, Inc.
>
> @@ -27,9 +27,14 @@ gp1=$(expr $g + 1)
>
> echo $gp1 > exp || framework_failure_
>
> +# With coreutils-8.16 and earlier, id -G would print both: $gp1 $g
> chroot --user=$NON_ROOT_USERNAME:$gp1 --groups='' / env PATH="$PATH" \
shouldn't we better avoid group name resolution here?
- chroot --user=$NON_ROOT_USERNAME:$gp1 --groups='' / env PATH="$PATH" \
+ chroot --user=$NON_ROOT_USERNAME:+$gp1 --groups='' / env PATH="$PATH" \
>id -G > out || fail=1
> compare exp out || fail=1
> -# With coreutils-8.16 and earlier, id -G would print both: $gp1 $g
> +
> +# With coreutils-8.22 and earlier, id would erroneously print groups=$g
> +chroot --user=$NON_ROOT_USERNAME:$gp1 --groups='' / env PATH="$PATH" \
Likewise.
> + id > out || fail=1
> +grep -F "groups=$gp1" out || fail=1
>
> Exit $fail
Another minor nit:
for a better diagnostic, it'd be better to use the construct
we already use in many places:
... || { cat out; fail=1; }
Otherwise +1.
Thanks & have a nice day,
Berny
bug#7320: [PATCH] 'id' prints incorrectly groups for the session
On 06/25/2014 01:17 PM, Petr Stodůlka wrote:
> Hi,
>
> command 'id' prints wrong groups for the session. This is similar to reported
> bug #7320 [0],
> which was patched earlier for 'groups' and 'id -G', however just 'id' still
> prints wrong groups.
> I propose this patch based on previous solution:
> --
> diff --git a/src/id.c b/src/id.c
> index 3348f80..6cfe884 100644
> --- a/src/id.c
> +++ b/src/id.c
> @@ -399,8 +399,12 @@ print_full_info (const char *username)
> gid_t *groups;
> int i;
>
> -int n_groups = xgetgroups (username, (pwd ? pwd->pw_gid : -1),
> - &groups);
> +int n_groups;
> +if(username)
> + n_groups = xgetgroups (username, (pwd ? pwd->pw_gid : -1),
> + &groups);
> +else
> + n_groups = xgetgroups (username, egid, &groups);
> if (n_groups < 0)
>{
> if (username)
>
>
> [0] http://debbugs.gnu.org/cgi/bugreport.cgi?bug=7320
Logic looks correct.
The attached refactors slightly, and adds a test and NEWS.
I'll apply upon your ack.
thanks!
Pádraig.
>From 2fd678d1201dbb1c877aba139190b0ac1025964f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Petr=20Stod=C5=AFlka?=
Date: Wed, 25 Jun 2014 18:26:23 +0100
Subject: [PATCH] id: output the effective group for the process
* src/id.c (print_full_info): When no user is specified,
output the effective group for the _process_, rather than
the default group from the system database, which may be different.
* tests/id/setgid.sh: Add a case for `id` as well as `id -G`.
* NEWS: Mention the bug fix.
Fixes http://bugs.gnu.org/7320
Reported at http://bugzilla.redhat.com/1016163
---
NEWS |6 ++
src/id.c | 19 ++-
tests/id/setgid.sh |9 +++--
3 files changed, 23 insertions(+), 11 deletions(-)
diff --git a/NEWS b/NEWS
index 6532785..e5ea77c 100644
--- a/NEWS
+++ b/NEWS
@@ -67,6 +67,12 @@ GNU coreutils NEWS-*- outline -*-
now copies all input to stdout. Previously nothing was output in this case.
[bug introduced with the --lines=-N feature in coreutils-5.0.1]
+ id, when invoked with no user name argument, now prints the correct group ID.
+ Previously, in the default output format, it would print the default group ID
+ in the password database, which may be neither real nor effective. For e.g.,
+ when run set-GID, or when the database changes outside the current session.
+ [bug introduced in coreutils-8.1]
+
ln -sf now replaces symbolic links whose targets can't exist. Previously
it would display an error, requiring --no-dereference to avoid the issue.
[bug introduced in coreutils-5.3.0]
diff --git a/src/id.c b/src/id.c
index 3348f80..f46bb41 100644
--- a/src/id.c
+++ b/src/id.c
@@ -399,19 +399,20 @@ print_full_info (const char *username)
gid_t *groups;
int i;
-int n_groups = xgetgroups (username, (pwd ? pwd->pw_gid : -1),
- &groups);
+gid_t primary_group;
+if (username)
+ primary_group = pwd ? pwd->pw_gid : -1;
+else
+ primary_group = egid;
+
+int n_groups = xgetgroups (username, primary_group, &groups);
if (n_groups < 0)
{
if (username)
- {
-error (0, errno, _("failed to get groups for user %s"),
- quote (username));
- }
+ error (0, errno, _("failed to get groups for user %s"),
+ quote (username));
else
- {
-error (0, errno, _("failed to get groups for the current process"));
- }
+ error (0, errno, _("failed to get groups for the current process"));
ok = false;
return;
}
diff --git a/tests/id/setgid.sh b/tests/id/setgid.sh
index aa43ea3..a81b42c 100755
--- a/tests/id/setgid.sh
+++ b/tests/id/setgid.sh
@@ -1,5 +1,5 @@
#!/bin/sh
-# Verify that id -G prints the right group when run set-GID.
+# Verify that id [-G] prints the right group when run set-GID.
# Copyright (C) 2012-2014 Free Software Foundation, Inc.
@@ -27,9 +27,14 @@ gp1=$(expr $g + 1)
echo $gp1 > exp || framework_failure_
+# With coreutils-8.16 and earlier, id -G would print both: $gp1 $g
chroot --user=$NON_ROOT_USERNAME:$gp1 --groups='' / env PATH="$PATH" \
id -G > out || fail=1
compare exp out || fail=1
-# With coreutils-8.16 and earlier, id -G would print both: $gp1 $g
+
+# With coreutils-8.22 and earlier, id would erroneously print groups=$g
+chroot --user=$NON_ROOT_USERNAME:$gp1 --groups='' / env PATH="$PATH" \
+ id > out || fail=1
+grep -F "groups=$gp1" out || fail=1
Exit $fail
--
1.7.7.6
