GNU bug report logs - #7320
'group' command gives wrong/extra group

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 7320 in the body.
You can then email your comments to 7320 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: owen <at> illinois.edu
To: bug-coreutils <at> gnu.org
Subject: 'group' command gives wrong/extra group
Date: Tue, 2 Nov 2010 16:40:21 -0500

Hi,

  It appears that the 'groups' command doesn't get the list of groups
for a currently-running process quite right.  The conditions I tested
were where the UID and EUID of the process were different, and the GID
and EGID were both set to the primary group of the EUID.  The groups
command looked up the real user and added the primary group from
/etc/passwd to the list of groups the user was in, even though the
process had no permissions for this extra group.

So, for example say /etc/passwd has at least these 2 entries:

root:x:0:0:root:/root:/bin/sh
user:x:1000:1000:user:/home/user:/bin/sh

and /etc/group has at least these 3 entries:
root:x:0:
other:x:500:
user:x:1000:

If a process as UID=0, EUID=1000, GID=1000, and EGID=1000 and no
supplemental groups, the correct list of groups should be

user

but the groups command shows

user root


If the passwd file is changed like so:

root:x:0:500:root:/root:/bin/sh

then the output of groups is

user other

---
Tested on Debian Slink

groups (GNU coreutils) 8.5
Copyright (C) 2010 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later
<http://gnu.org/licenses/gpl.html>.
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

Written by David MacKenzie and James Youngman.


-- 
<><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><>
<>  Brynnen Owen            (     this space for rent                      )<>
<>  owen <at> illinois.edu       (                                              )<>
<><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><><>

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

From: James Youngman <jay <at> gnu.org>
To: owen <at> illinois.edu
Cc: 7320 <at> debbugs.gnu.org
Subject: Re: bug#7320: 'group' command gives wrong/extra group
Date: Mon, 8 Nov 2010 10:35:00 +0000

Does the same thing happen with "id -G"?

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

From: "Marc W. Mengel" <mengel <at> fnal.gov>
To: <7320 <at> debbugs.gnu.org>
Subject: Confirmed on coreutils-8.4-13
Date: Thu, 26 Apr 2012 13:16:53 -0500

This is still broken in RedHat in coreutils-8.4-13

All of  "groups" and "id" and "id -G" report groups that you don't have
if you list a new/different primary group in /etc/passwd.

This is just plain wrong.  "id" and "groups" should list the groups you
actually have, not what you would possibly have if you logged out and 
back in again.

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

From: Jim Meyering <jim <at> meyering.net>
To: "Marc W. Mengel" <mengel <at> fnal.gov>
Cc: 7320 <at> debbugs.gnu.org
Subject: Re: bug#7320: Confirmed on coreutils-8.4-13
Date: Fri, 27 Apr 2012 13:08:08 +0200

Marc W. Mengel wrote:
> This is still broken in RedHat in coreutils-8.4-13
>
> All of  "groups" and "id" and "id -G" report groups that you don't have
> if you list a new/different primary group in /etc/passwd.
>
> This is just plain wrong.  "id" and "groups" should list the groups you
> actually have, not what you would possibly have if you logged out and
> back in again.

Thank you for the report.
It looks like there is indeed a bug.

I demonstrated it with this:

    echo 'for i in 1 2; do id -G; sleep 1.5; done; sleep 4' \
      |su -s /bin/sh ftp - &
    sleep 1; perl -pi -e 's/^(ftp:x:\d+):(\d+)/$1:9876/' /etc/passwd

Those id -G commands printed the following:

    50
    50 9876

With the patch below, they print this:

    50
    50

The problem is that group-list.c(print_group_list), called by id.c
did not invoke xgetgroups properly.  xgetgroups is just a wrapper
around mgetgroups, which has this comment:

     If USERNAME is
     NULL, store the supplementary groups of the current process, and GID
     should be -1 or the effective group ID (getegid).

In the case you've noted, USERNAME is indeed NULL,
so print_group_list should pass the effective group ID,
and not getpwuid(ruid)->pw_gid.

This also shows that when username is NULL, print_group_list's
call to getpwuid is now useless and wasteful.

With all that, here's the patch I expect to commit:

diff --git a/src/group-list.c b/src/group-list.c
index cf49911..edbb342 100644
--- a/src/group-list.c
+++ b/src/group-list.c
@@ -38,11 +38,14 @@ print_group_list (const char *username,
                   bool use_names)
 {
   bool ok = true;
-  struct passwd *pwd;
+  struct passwd *pwd = NULL;

-  pwd = getpwuid (ruid);
-  if (pwd == NULL)
-    ok = false;
+  if (username)
+    {
+      pwd = getpwuid (ruid);
+      if (pwd == NULL)
+        ok = false;
+    }

   if (!print_group (rgid, use_names))
     ok = false;
@@ -58,8 +61,7 @@ print_group_list (const char *username,
     gid_t *groups;
     int i;

-    int n_groups = xgetgroups (username, (pwd ? pwd->pw_gid : (gid_t) -1),
-                               &groups);
+    int n_groups = xgetgroups (username, (pwd ? pwd->pw_gid : egid), &groups);
     if (n_groups < 0)
       {
         if (username)

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

From: Jim Meyering <jim <at> meyering.net>
To: "Marc W. Mengel" <mengel <at> fnal.gov>, owen <at> illinois.edu
Cc: 7320 <at> debbugs.gnu.org
Subject: Re: bug#7320: id and groups may lie
Date: Fri, 27 Apr 2012 15:43:37 +0200

Jim Meyering wrote:
> Marc W. Mengel wrote:
>> This is still broken in RedHat in coreutils-8.4-13
>>
>> All of  "groups" and "id" and "id -G" report groups that you don't have
>> if you list a new/different primary group in /etc/passwd.
>>
>> This is just plain wrong.  "id" and "groups" should list the groups you
>> actually have, not what you would possibly have if you logged out and
>> back in again.
>
> Thank you for the report.
> It looks like there is indeed a bug.
>
> I demonstrated it with this:
...
> With all that, here's the patch I expect to commit:
>
> diff --git a/src/group-list.c b/src/group-list.c

Here's a complete patch.
Note the lack of a test case.
Even in a root-only test, and briefly, I don't want to change
the password database.

From 3bcb3ea46d685f499c7a02efb1cbbbf15f858325 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering <at> redhat.com>
Date: Fri, 27 Apr 2012 13:28:32 +0200
Subject: [PATCH] id,groups: with no user name, print only real and/or
 effective IDs,

... i.e., don't use the getpw* functions.
Before this change, running
groups or id with no user name argument would include a group
name or ID from /etc/passwd.  Thus, under unusual circumstances
(default group is changed, but has not taken effect for a given
session), those programs could print a name or ID that is neither
real nor effective.

To demonstrate, run this:

    echo 'for i in 1 2; do id -G; sleep 1.5; done' \
      |su -s /bin/sh ftp - &
    sleep 1; perl -pi -e 's/^(ftp:x:\d+):(\d+)/$1:9876/' /etc/passwd

Those id -G commands printed the following:

    50
    50 9876

With this change, they print this:

    50
    50

* src/group-list.c (print_group_list): When username is NULL, pass
egid, not getpwuid(ruid)->pw_gid), to xgetgroups, per the API
requirements of xgetgroups callee, mgetgroups.
When not using the password database, don't call getpwuid.
* NEWS (Bug fixes): Mention it.
Originally reported by Brynnen Owen as http://bugs.gnu.org/7320.
Raised again by Marc Mengel in http://bugzilla.redhat.com/816708.
---
 NEWS             |    8 ++++++++
 THANKS.in        |    2 ++
 src/group-list.c |   14 ++++++++------
 3 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/NEWS b/NEWS
index ef4e508..c50336b 100644
--- a/NEWS
+++ b/NEWS
@@ -2,6 +2,14 @@ GNU coreutils NEWS                                    -*- outline -*-

 * Noteworthy changes in release ?.? (????-??-??) [?]

+** Bug fixes
+
+  id and groups, when invoked with no user name argument, would print
+  the default group ID listed in the password database, and sometimes
+  that ID would be neither real nor effective.  For example, in a session
+  for which the default group has just been changed, the new group ID
+  would be listed, even though it is not yet effective.
+
 ** New features

   fmt now accepts the --goal=WIDTH (-g) option.
diff --git a/THANKS.in b/THANKS.in
index d23f7b3..a7403fd 100644
--- a/THANKS.in
+++ b/THANKS.in
@@ -98,6 +98,7 @@ Brian Silverman                     bsilverman <at> conceptxdesign.com
 Brian Youmans                       3diff <at> gnu.org
 Britton Leo Kerin                   fsblk <at> aurora.uaf.edu
 Bruce Robertson                     brucer <at> theodolite.dyndns.org
+Brynnen Owen                        owen <at> illinois.edu
 Carl Johnson                        carlj <at> cjlinux.home.org
 Carl Lowenstein                     cdl <at> mpl.UCSD.EDU
 Carl Roth                           roth <at> urs.us
@@ -355,6 +356,7 @@ Manfred Hollstein                   manfred <at> s-direktnet.de
 Марк Коренберг                      socketpair <at> gmail.com
 Marc Boucher                        marc <at> mbsi.ca
 Marc Haber                          mh+debian-bugs <at> zugschlus.de
+Marc Mengel                         mengel <at> fnal.gov
 Marc Lehman                         schmorp <at> schmorp.de
 Marc Olzheim                        marcolz <at> stack.nl
 Marco Franzen                       Marco.Franzen <at> Thyron.com
diff --git a/src/group-list.c b/src/group-list.c
index cf49911..edbb342 100644
--- a/src/group-list.c
+++ b/src/group-list.c
@@ -38,11 +38,14 @@ print_group_list (const char *username,
                   bool use_names)
 {
   bool ok = true;
-  struct passwd *pwd;
+  struct passwd *pwd = NULL;

-  pwd = getpwuid (ruid);
-  if (pwd == NULL)
-    ok = false;
+  if (username)
+    {
+      pwd = getpwuid (ruid);
+      if (pwd == NULL)
+        ok = false;
+    }

   if (!print_group (rgid, use_names))
     ok = false;
@@ -58,8 +61,7 @@ print_group_list (const char *username,
     gid_t *groups;
     int i;

-    int n_groups = xgetgroups (username, (pwd ? pwd->pw_gid : (gid_t) -1),
-                               &groups);
+    int n_groups = xgetgroups (username, (pwd ? pwd->pw_gid : egid), &groups);
     if (n_groups < 0)
       {
         if (username)
--
1.7.10.336.gc5e31

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

From: Pádraig Brady <P <at> draigBrady.com>
To: Jim Meyering <jim <at> meyering.net>
Cc: owen <at> illinois.edu, 7320 <at> debbugs.gnu.org, "Marc W. Mengel" <mengel <at> fnal.gov>
Subject: Re: bug#7320: id and groups may lie
Date: Fri, 27 Apr 2012 15:07:16 +0100

looks good

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

From: "Marc W. Mengel" <mengel <at> fnal.gov>
To: Jim Meyering <jim <at> meyering.net>
Cc: owen <at> illinois.edu, 7320 <at> debbugs.gnu.org
Subject: Re: bug#7320: id and groups may lie
Date: Fri, 27 Apr 2012 09:55:36 -0500

The other test case is to make a copy of "id" and make it
setuid to some user (i.e. mysql) and run it; it will show
itself as having mysql's primary group, even though it doesn't.

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

From: Jim Meyering <jim <at> meyering.net>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: owen <at> illinois.edu, 7320 <at> debbugs.gnu.org, "Marc W. Mengel" <mengel <at> fnal.gov>
Subject: Re: bug#7320: id and groups may lie
Date: Fri, 27 Apr 2012 16:57:16 +0200

Pádraig Brady wrote:
> looks good

Thanks for the quick review.

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

From: Jim Meyering <jim <at> meyering.net>
To: "Marc W. Mengel" <mengel <at> fnal.gov>
Cc: owen <at> illinois.edu, 7320 <at> debbugs.gnu.org
Subject: Re: bug#7320: id and groups may lie
Date: Fri, 27 Apr 2012 18:30:17 +0200

Marc W. Mengel wrote:
> The other test case is to make a copy of "id" and make it
> setuid to some user (i.e. mysql) and run it; it will show
> itself as having mysql's primary group, even though it doesn't.

Oh!  Yes, that will work.  Thanks.
With that, I'll add a test like this:

New:
    $ sudo src/setuidgid -g 3 root src/id -G
    3

Old:
    $ sudo src/setuidgid -g 3 root id -G
    3 0

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

From: Jim Meyering <jim <at> meyering.net>
To: "Marc W. Mengel" <mengel <at> fnal.gov>
Cc: owen <at> illinois.edu, 7320 <at> debbugs.gnu.org
Subject: Re: bug#7320: id and groups may lie
Date: Fri, 27 Apr 2012 18:57:11 +0200

Jim Meyering wrote:

> Marc W. Mengel wrote:
>> The other test case is to make a copy of "id" and make it
>> setuid to some user (i.e. mysql) and run it; it will show
>> itself as having mysql's primary group, even though it doesn't.
>
> Oh!  Yes, that will work.  Thanks.
> With that, I'll add a test like this:
>
> New:
>     $ sudo src/setuidgid -g 3 root src/id -G
>     3
>
> Old:
>     $ sudo src/setuidgid -g 3 root id -G
>     3 0

Here's the new test that I'll merge into the actual fix:

From d11303c1e643d31aea70e15f79ecf8b55038446a Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering <at> redhat.com>
Date: Fri, 27 Apr 2012 18:44:08 +0200
Subject: [PATCH] tests: add a test for the just-fixed id/groups bug

* tests/misc/id-setgid: New file.
* tests/Makefile.am (TESTS): Add it.
---
 tests/Makefile.am    |    1 +
 tests/misc/id-setgid |   34 ++++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)
 create mode 100755 tests/misc/id-setgid

diff --git a/tests/Makefile.am b/tests/Makefile.am
index ce2366b..cd1fc5c 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -198,6 +198,7 @@ TESTS =						\
   misc/head-pos					\
   misc/id-context				\
   misc/id-groups				\
+  misc/id-setgid				\
   misc/md5sum					\
   misc/md5sum-bsd				\
   misc/md5sum-newline				\
diff --git a/tests/misc/id-setgid b/tests/misc/id-setgid
new file mode 100755
index 0000000..12fab38
--- /dev/null
+++ b/tests/misc/id-setgid
@@ -0,0 +1,34 @@
+#!/bin/sh
+# Verify that id -G prints the right group when run set-GID.
+
+# Copyright (C) 2012 Free Software Foundation, Inc.
+
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+. "${srcdir=.}/init.sh"; path_prepend_ ../src
+print_ver_ id
+require_root_
+
+g=$(id -u $NON_ROOT_USERNAME) || framework_failure_
+
+# Construct a different group number.
+gp1=$(expr $g + 1)
+
+echo $gp1 > exp || framework_failure_
+
+setuidgid -g $gp1 $NON_ROOT_USERNAME 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
+
+Exit $fail
--
1.7.10.365.g7cacb

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

From: Jim Meyering <jim <at> meyering.net>
To: "Marc W. Mengel" <mengel <at> fnal.gov>
Cc: owen <at> illinois.edu, 7320 <at> debbugs.gnu.org
Subject: Re: bug#7320: id and groups may lie
Date: Fri, 27 Apr 2012 19:53:53 +0200

Jim Meyering wrote:

> Jim Meyering wrote:
>
>> Marc W. Mengel wrote:
>>> The other test case is to make a copy of "id" and make it
>>> setuid to some user (i.e. mysql) and run it; it will show
>>> itself as having mysql's primary group, even though it doesn't.
>>
>> Oh!  Yes, that will work.  Thanks.
>> With that, I'll add a test like this:
>>
>> New:
>>     $ sudo src/setuidgid -g 3 root src/id -G
>>     3
>>
>> Old:
>>     $ sudo src/setuidgid -g 3 root id -G
>>     3 0
>
> Here's the new test that I'll merge into the actual fix:
>
>>From d11303c1e643d31aea70e15f79ecf8b55038446a Mon Sep 17 00:00:00 2001
> From: Jim Meyering <meyering <at> redhat.com>
> Date: Fri, 27 Apr 2012 18:44:08 +0200
> Subject: [PATCH] tests: add a test for the just-fixed id/groups bug
>
> * tests/misc/id-setgid: New file.
> * tests/Makefile.am (TESTS): Add it.
> ---
>  tests/Makefile.am    |    1 +
>  tests/misc/id-setgid |   34 ++++++++++++++++++++++++++++++++++

Nearly missed it.  I'd added a new root-only test
without listing it in the root_tests list.
Once we depend on GNU make, we should be able to automate this.

Folding this in:

From 7418ab4c137d5fb608ea9cd974aabf5f6a76445e Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering <at> redhat.com>
Date: Fri, 27 Apr 2012 19:51:54 +0200
Subject: [PATCH] tests: fix syntax-check failure

* tests/Makefile.am (root_tests): It's a root-only test,
so add it here, too.
---
 tests/Makefile.am |    1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/Makefile.am b/tests/Makefile.am
index cd1fc5c..72717e3 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -36,6 +36,7 @@ root_tests =					\
   ls/nameless-uid				\
   misc/chcon					\
   misc/chroot-credentials			\
+  misc/id-setgid				\
   misc/selinux					\
   misc/truncate-owned-by-other			\
   mkdir/writable-under-readonly			\
--
1.7.10.365.g7cacb

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

From: Pádraig Brady <P <at> draigBrady.com>
To: Petr Stodůlka <pstodulk <at> redhat.com>
Cc: coreutils <at> gnu.org, 7320 <at> debbugs.gnu.org
Subject: Re: [PATCH] 'id' prints incorrectly groups for the session
Date: Thu, 26 Jun 2014 02:53:08 +0100

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

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.

[id-effective-group.patch (text/x-patch, attachment)]

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

From: Jim Meyering <jim <at> meyering.net>
To: Pádraig Brady <P <at> draigbrady.com>
Cc: Petr Stodůlka <pstodulk <at> redhat.com>,
 Coreutils <coreutils <at> gnu.org>, 7320 <at> debbugs.gnu.org
Subject: Re: [PATCH] 'id' prints incorrectly groups for the session
Date: Wed, 25 Jun 2014 19:45:22 -0700

On Wed, Jun 25, 2014 at 6:53 PM, Pádraig Brady <P <at> draigbrady.com> wrote:
> 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.

Nice patch. Looks perfect.  Thanks to both of you.

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

From: Bernhard Voelker <mail <at> bernhard-voelker.de>
To: Pádraig Brady <P <at> draigBrady.com>, 
 Petr Stodůlka <pstodulk <at> redhat.com>
Cc: coreutils <at> gnu.org, 7320 <at> debbugs.gnu.org
Subject: Re: [PATCH] 'id' prints incorrectly groups for the session
Date: Thu, 26 Jun 2014 07:44:03 +0200

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

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

From: Pádraig Brady <P <at> draigBrady.com>
To: Bernhard Voelker <mail <at> bernhard-voelker.de>
Cc: Petr Stodůlka <pstodulk <at> redhat.com>, coreutils <at> gnu.org,
 7320 <at> debbugs.gnu.org
Subject: Re: [PATCH] 'id' prints incorrectly groups for the session
Date: Thu, 26 Jun 2014 11:23:09 +0100

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

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

From: Jim Meyering <jim <at> meyering.net>
To: Pádraig Brady <P <at> draigbrady.com>
Cc: Petr Stodůlka <pstodulk <at> redhat.com>,
 Bernhard Voelker <mail <at> bernhard-voelker.de>, Coreutils <coreutils <at> gnu.org>,
 7320 <7320 <at> debbugs.gnu.org>
Subject: Re: [PATCH] 'id' prints incorrectly groups for the session
Date: Thu, 26 Jun 2014 09:42:21 -0700

On Thu, Jun 26, 2014 at 3:23 AM, Pádraig Brady <P <at> draigbrady.com> 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

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

From: Pádraig Brady <P <at> draigBrady.com>
To: Jim Meyering <jim <at> meyering.net>
Cc: Petr Stodůlka <pstodulk <at> redhat.com>,
 Bernhard Voelker <mail <at> bernhard-voelker.de>, Coreutils <coreutils <at> gnu.org>,
 7320 <7320 <at> debbugs.gnu.org>
Subject: Re: bug#7320: [PATCH] 'id' prints incorrectly groups for the session
Date: Thu, 26 Jun 2014 18:25:45 +0100

On 06/26/2014 05:42 PM, Jim Meyering wrote:
> On Thu, Jun 26, 2014 at 3:23 AM, Pádraig Brady <P <at> draigbrady.com> 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.

Previous Next

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