GNU bug report logs - #10021
[PATCH id] Add error-checking on GNU

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 10021 in the body.
You can then email your comments to 10021 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: ludo <at> gnu.org (Ludovic Courtès)
To: bug-coreutils <at> gnu.org
Cc: bug-hurd <at> gnu.org
Subject: [PATCH id] Add error-checking on GNU
Date: Fri, 11 Nov 2011 20:31:57 +0100

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

Hello,

On GNU, processes can have zero or more UIDs/GIDs.  In the case of a
process with zero UIDs, for instance, ‘getuid’ returns -1 and sets
ERRNO [0] (as an extension to POSIX [1].)

Currently ‘id’ would print (unsigned int) -1 as the UID in that case,
whereas it should rather print an error.  The attached patch does that.

(Note that the Hurd comes with another utility, called ‘ids’, which
prints all the (E)[UG]IDs of the process and gracefully handles the
zero-UID/GID case [2].)

Thanks,
Ludo’.

[0] http://git.savannah.gnu.org/cgit/hurd/glibc.git/tree/sysdeps/mach/hurd/getuid.c
[1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/getuid.html
[2] http://git.savannah.gnu.org/cgit/hurd/hurd.git/tree/utils/ids.c

[Message part 2 (text/x-patch, inline)]

diff --git a/src/id.c b/src/id.c
index f80fcd1..824a471 100644
--- a/src/id.c
+++ b/src/id.c
@@ -202,9 +202,28 @@ main (int argc, char **argv)
   else
     {
       euid = geteuid ();
+#ifdef __GNU__
+      if (euid == -1)
+	error (EXIT_FAILURE, errno, _("Cannot get effective UID"));
+#endif
+
       ruid = getuid ();
+#ifdef __GNU__
+      if (ruid == -1)
+	error (EXIT_FAILURE, errno, _("Cannot get real UID"));
+#endif
+
       egid = getegid ();
+#ifdef __GNU__
+      if (egid == -1)
+	error (EXIT_FAILURE, errno, _("Cannot get effective GID"));
+#endif
+
       rgid = getgid ();
+#ifdef __GNU__
+      if (rgid == -1)
+	error (EXIT_FAILURE, errno, _("Cannot get real GID"));
+#endif
     }
 
   if (just_user)

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

From: Jim Meyering <jim <at> meyering.net>
To: ludo <at> gnu.org (Ludovic Courtès)
Cc: bug-hurd <at> gnu.org, 10021 <at> debbugs.gnu.org
Subject: Re: bug#10021: [PATCH id] Add error-checking on GNU
Date: Fri, 11 Nov 2011 21:03:43 +0100

Ludovic Courtès wrote:
> On GNU, processes can have zero or more UIDs/GIDs.  In the case of a
> process with zero UIDs, for instance, ‘getuid’ returns -1 and sets
> ERRNO [0] (as an extension to POSIX [1].)
>
> Currently ‘id’ would print (unsigned int) -1 as the UID in that case,
> whereas it should rather print an error.  The attached patch does that.
>
> (Note that the Hurd comes with another utility, called ‘ids’, which
> prints all the (E)[UG]IDs of the process and gracefully handles the
> zero-UID/GID case [2].)
>
> Thanks,
> Ludo’.
>
> [0]
> http://git.savannah.gnu.org/cgit/hurd/glibc.git/tree/sysdeps/mach/hurd/getuid.c
> [1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/getuid.html
> [2] http://git.savannah.gnu.org/cgit/hurd/hurd.git/tree/utils/ids.c

Hi Ludo,

Thanks for the patch.
However, wouldn't that fail unnecessarily for a process without
one ID even though id is being asked to print some other(s)?
E.g., it'd fail for a process with no EUID even when id
is being asked to print only the real UID or GIDs.

Also, if you send another version, please indent only with spaces
and change each diagnostic to start with lower case letter.
You may want to run "make syntax-check" to check for things like that.

> diff --git a/src/id.c b/src/id.c
> index f80fcd1..824a471 100644
> --- a/src/id.c
> +++ b/src/id.c
> @@ -202,9 +202,28 @@ main (int argc, char **argv)
>    else
>      {
>        euid = geteuid ();
> +#ifdef __GNU__
> +      if (euid == -1)
> +	error (EXIT_FAILURE, errno, _("Cannot get effective UID"));
> +#endif
> +
>        ruid = getuid ();
> +#ifdef __GNU__
> +      if (ruid == -1)
> +	error (EXIT_FAILURE, errno, _("Cannot get real UID"));
> +#endif
> +
>        egid = getegid ();
> +#ifdef __GNU__
> +      if (egid == -1)
> +	error (EXIT_FAILURE, errno, _("Cannot get effective GID"));
> +#endif
> +
>        rgid = getgid ();
> +#ifdef __GNU__
> +      if (rgid == -1)
> +	error (EXIT_FAILURE, errno, _("Cannot get real GID"));
> +#endif
>      }
>
>    if (just_user)

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

From: ludo <at> gnu.org (Ludovic Courtès)
To: Jim Meyering <jim <at> meyering.net>
Cc: bug-hurd <at> gnu.org, 10021 <at> debbugs.gnu.org
Subject: Re: bug#10021: [PATCH id] Add error-checking on GNU
Date: Sat, 12 Nov 2011 22:48:58 +0100

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

Hi Jim,

Thanks for the quick review!

Jim Meyering <jim <at> meyering.net> skribis:

> However, wouldn't that fail unnecessarily for a process without
> one ID even though id is being asked to print some other(s)?
> E.g., it'd fail for a process with no EUID even when id
> is being asked to print only the real UID or GIDs.

Indeed.

> Also, if you send another version, please indent only with spaces
> and change each diagnostic to start with lower case letter.

Sure.  (There’s one just above that doesn’t follow the rule.)

> You may want to run "make syntax-check" to check for things like that.

OK.

Here’s an updated patch with a test case.

I don’t have a copyright assignment on file for Coreutils but I guess
it’s OK for this change?

Thanks,
Ludo’.

[0001-id-error-out-for-zero-UIDs-GIDs-on-GNU-Hurd.patch (text/x-patch, inline)]

From b1e9324ea83a2df604256d7d41d25440e21dd2d8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ludovic=20Court=C3=A8s?= <ludo <at> gnu.org>
Date: Sat, 12 Nov 2011 01:25:45 +0100
Subject: [PATCH] id: error out for zero UIDs/GIDs on GNU/Hurd

* src/id.c (main)[__GNU__]: Check for EUID, RUID, EGID, or RGID = -1;
  raise an error when appropriate.

* tests/Makefile.am (TESTS): Add `id/gnu-zero-uids'.
* tests/id/gnu-zero-uids: New file.
* tests/init.cfg (require_gnu_): New function.
---
 src/id.c               |   21 +++++++++++++++++++++
 tests/Makefile.am      |    1 +
 tests/id/gnu-zero-uids |   31 +++++++++++++++++++++++++++++++
 tests/init.cfg         |    8 ++++++++
 4 files changed, 61 insertions(+), 0 deletions(-)
 create mode 100644 tests/id/gnu-zero-uids

diff --git a/src/id.c b/src/id.c
index f80fcd1..9325282 100644
--- a/src/id.c
+++ b/src/id.c
@@ -202,9 +202,30 @@ main (int argc, char **argv)
   else
     {
       euid = geteuid ();
+#ifdef __GNU__
+      if (euid == -1 && !use_real
+          && !just_group && !just_group_list && !just_context)
+        error (EXIT_FAILURE, errno, _("cannot get effective UID"));
+#endif
+
       ruid = getuid ();
+#ifdef __GNU__
+      if (ruid == -1 && use_real
+          && !just_group && !just_group_list && !just_context)
+        error (EXIT_FAILURE, errno, _("cannot get real UID"));
+#endif
+
       egid = getegid ();
+#ifdef __GNU__
+      if (egid == -1 && !use_real && !just_user)
+        error (EXIT_FAILURE, errno, _("cannot get effective GID"));
+#endif
+
       rgid = getgid ();
+#ifdef __GNU__
+      if (rgid == -1 && use_real && !just_user)
+        error (EXIT_FAILURE, errno, _("cannot get real GID"));
+#endif
     }
 
   if (just_user)
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 5021c18..80f95b1 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -399,6 +399,7 @@ TESTS =						\
   du/slink					\
   du/trailing-slash				\
   du/two-args					\
+  id/gnu-zero-uids				\
   id/no-context					\
   install/basic-1				\
   install/create-leading			\
diff --git a/tests/id/gnu-zero-uids b/tests/id/gnu-zero-uids
new file mode 100644
index 0000000..b4a7d5a
--- /dev/null
+++ b/tests/id/gnu-zero-uids
@@ -0,0 +1,31 @@
+#!/bin/sh
+# On GNU, `id' must fail for processes with zero UIDs.
+
+# Copyright (C) 2011 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_gnu_
+
+if sush - true; then
+    # Run `id' with zero UIDs.  It should exit with a non-zero status.
+    sush - id > out && fail=1
+else
+    skip_ "the \`sush' command does not work"
+fi
+
+Exit $fail
diff --git a/tests/init.cfg b/tests/init.cfg
index 915f38a..f5f27dd 100644
--- a/tests/init.cfg
+++ b/tests/init.cfg
@@ -499,4 +499,12 @@ print_ver_()
   fi
 }
 
+# Are we running on GNU/Hurd?
+require_gnu_()
+{
+  if test "`uname`" != GNU; then
+     skip_ 'not running on GNU/Hurd'
+  fi
+}
+
 sanitize_path_
-- 
1.7.6

[Message part 3 (application/pgp-signature, inline)]

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

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: bug-hurd <at> gnu.org, Jim Meyering <jim <at> meyering.net>, 10021 <at> debbugs.gnu.org
Subject: Re: bug#10021: [PATCH id] Add error-checking on GNU
Date: Sat, 12 Nov 2011 15:15:34 -0800

On 11/12/11 13:48, Ludovic Courtès wrote:
> +#ifdef __GNU__
> +      if (euid == -1 && !use_real
> +          && !just_group && !just_group_list && !just_context)
> +        error (EXIT_FAILURE, errno, _("cannot get effective UID"));
> +#endif

I suggest removing the "#ifdef __GNU__" here and in its other
three uses in the patch, as functions like as geteuid() can fail on
a few non-GNU systems too.  See:

http://publib.boulder.ibm.com/infocenter/iseries/v5r3/index.jsp?topic=%2Fapis%2Fgeteuid.htm

For this particular application (the 'id' program) I doubt whether
it's worth our time to configure this stuff at compile-time,
and that it's fine to do a run-time check on all platforms.

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

From: Jim Meyering <jim <at> meyering.net>
To: ludo <at> gnu.org (Ludovic Courtès)
Cc: bug-hurd <at> gnu.org, 10021 <at> debbugs.gnu.org
Subject: Re: bug#10021: [PATCH id] Add error-checking on GNU
Date: Sun, 13 Nov 2011 10:40:59 +0100

Ludovic Courtès wrote:
> Thanks for the quick review!
>
> Jim Meyering <jim <at> meyering.net> skribis:
>
>> However, wouldn't that fail unnecessarily for a process without
>> one ID even though id is being asked to print some other(s)?
>> E.g., it'd fail for a process with no EUID even when id
>> is being asked to print only the real UID or GIDs.
>
> Indeed.
>
>> Also, if you send another version, please indent only with spaces
>> and change each diagnostic to start with lower case letter.
>
> Sure.  (There’s one just above that doesn’t follow the rule.)

Thanks.  There are two like that.  I'll adjust them separately.

>> You may want to run "make syntax-check" to check for things like that.
>
> OK.
>
> Here’s an updated patch with a test case.
>
> I don’t have a copyright assignment on file for Coreutils but I guess
> it’s OK for this change?

It's borderline, but ok once we take Paul's advice and remove the #ifdefs.
Thanks for the test suite addition.

In addition to the #ifdef-removing changes, I've also made these:

diff --git a/tests/id/gnu-zero-uids b/tests/id/gnu-zero-uids
index b4a7d5a..bc9043c 100644
--- a/tests/id/gnu-zero-uids
+++ b/tests/id/gnu-zero-uids
@@ -21,11 +21,9 @@ print_ver_ id

 require_gnu_

-if sush - true; then
-    # Run `id' with zero UIDs.  It should exit with a non-zero status.
-    sush - id > out && fail=1
-else
-    skip_ "the \`sush' command does not work"
-fi
+sush - true || skip_ "the \`sush' command does not work"
+
+# Run `id' with zero UIDs.  It should exit with a non-zero status.
+sush - id > out && fail=1

 Exit $fail
diff --git a/tests/init.cfg b/tests/init.cfg
index f5f27dd..9b05b34 100644
--- a/tests/init.cfg
+++ b/tests/init.cfg
@@ -502,9 +502,8 @@ print_ver_()
 # Are we running on GNU/Hurd?
 require_gnu_()
 {
-  if test "`uname`" != GNU; then
-     skip_ 'not running on GNU/Hurd'
-  fi
+  test "$(uname)" = GNU \
+    || skip_ 'not running on GNU/Hurd'
 }

 sanitize_path_

Here's the complete patch:
Oh, I also had to make the new test script executable,
or else "make check" would fail.

Since I've made so many changes, I'll wait to hear back
from you before pushing it.

From 70903f8e4ebabd08e2e01403d9eea1e45fe42bb8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ludovic=20Court=C3=A8s?= <ludo <at> gnu.org>
Date: Sat, 12 Nov 2011 01:25:45 +0100
Subject: [PATCH] id: fail when getuid, getgid, etc. fail, e.g., on GNU/Hurd

POSIX-conforming getuid, geteuid, etc. functions cannot fail,
but on GNU/Hurd systems and some others, they may.
* src/id.c (main): Detect and diagnose any such failure.
* tests/id/gnu-zero-uids: New file.
* tests/Makefile.am (TESTS): Add it to the list.
* tests/init.cfg (require_gnu_): New function.
---
 src/id.c               |   13 +++++++++++++
 tests/Makefile.am      |    1 +
 tests/id/gnu-zero-uids |   29 +++++++++++++++++++++++++++++
 tests/init.cfg         |    7 +++++++
 4 files changed, 50 insertions(+), 0 deletions(-)
 create mode 100644 tests/id/gnu-zero-uids

diff --git a/src/id.c b/src/id.c
index f80fcd1..0966d15 100644
--- a/src/id.c
+++ b/src/id.c
@@ -202,9 +202,22 @@ main (int argc, char **argv)
   else
     {
       euid = geteuid ();
+      if (euid == -1 && !use_real
+          && !just_group && !just_group_list && !just_context)
+        error (EXIT_FAILURE, errno, _("cannot get effective UID"));
+
       ruid = getuid ();
+      if (ruid == -1 && use_real
+          && !just_group && !just_group_list && !just_context)
+        error (EXIT_FAILURE, errno, _("cannot get real UID"));
+
       egid = getegid ();
+      if (egid == -1 && !use_real && !just_user)
+        error (EXIT_FAILURE, errno, _("cannot get effective GID"));
+
       rgid = getgid ();
+      if (rgid == -1 && use_real && !just_user)
+        error (EXIT_FAILURE, errno, _("cannot get real GID"));
     }

   if (just_user)
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 64366a4..48c33cb 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -399,6 +399,7 @@ TESTS =						\
   du/slink					\
   du/trailing-slash				\
   du/two-args					\
+  id/gnu-zero-uids				\
   id/no-context					\
   install/basic-1				\
   install/create-leading			\
diff --git a/tests/id/gnu-zero-uids b/tests/id/gnu-zero-uids
new file mode 100644
index 0000000..bc9043c
--- /dev/null
+++ b/tests/id/gnu-zero-uids
@@ -0,0 +1,29 @@
+#!/bin/sh
+# On GNU, `id' must fail for processes with zero UIDs.
+
+# Copyright (C) 2011 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_gnu_
+
+sush - true || skip_ "the \`sush' command does not work"
+
+# Run `id' with zero UIDs.  It should exit with a non-zero status.
+sush - id > out && fail=1
+
+Exit $fail
diff --git a/tests/init.cfg b/tests/init.cfg
index 915f38a..9b05b34 100644
--- a/tests/init.cfg
+++ b/tests/init.cfg
@@ -499,4 +499,11 @@ print_ver_()
   fi
 }

+# Are we running on GNU/Hurd?
+require_gnu_()
+{
+  test "$(uname)" = GNU \
+    || skip_ 'not running on GNU/Hurd'
+}
+
 sanitize_path_
--
1.7.8.rc0.61.g8a042

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

From: ludo <at> gnu.org (Ludovic Courtès)
To: Jim Meyering <jim <at> meyering.net>
Cc: bug-hurd <at> gnu.org, 10021 <at> debbugs.gnu.org
Subject: Re: bug#10021: [PATCH id] Add error-checking on GNU
Date: Sun, 13 Nov 2011 22:58:03 +0100

Hi Jim,

Jim Meyering <jim <at> meyering.net> skribis:

> Oh, I also had to make the new test script executable,
> or else "make check" would fail.

It works for me even if it’s not executable when I run “make check
TESTS=id/gnu-zero-uids”, apparently because it’s run this way:

  execve("/bin/sh", ["/bin/sh", "./shell-or-perl", "--test-name", "id/gnu-zero-uids", "--srcdir", ".", "--shell", "/bin/sh", "--perl", "perl", "--", "./id/gnu-zero-uids"], [/* 96 vars */]) = 0

It might be different when running just “make check”.

(I tested it on GNU/Linux and GNU/Hurd.)

> Since I've made so many changes, I'll wait to hear back
> from you before pushing it.

Looks good to me, thank you!

> +  test "$(uname)" = GNU \

So you’re assuming Bash, right?

Thanks,
Ludo’.

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

From: ludo <at> gnu.org (Ludovic Courtès)
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: bug-hurd <at> gnu.org, Jim Meyering <jim <at> meyering.net>, 10021 <at> debbugs.gnu.org
Subject: Re: bug#10021: [PATCH id] Add error-checking on GNU
Date: Sun, 13 Nov 2011 23:02:19 +0100

Hi Paul,

Paul Eggert <eggert <at> cs.ucla.edu> skribis:

> On 11/12/11 13:48, Ludovic Courtès wrote:
>> +#ifdef __GNU__
>> +      if (euid == -1 && !use_real
>> +          && !just_group && !just_group_list && !just_context)
>> +        error (EXIT_FAILURE, errno, _("cannot get effective UID"));
>> +#endif
>
> I suggest removing the "#ifdef __GNU__" here and in its other
> three uses in the patch, as functions like as geteuid() can fail on
> a few non-GNU systems too.  See:
>
> http://publib.boulder.ibm.com/infocenter/iseries/v5r3/index.jsp?topic=%2Fapis%2Fgeteuid.htm
>
> For this particular application (the 'id' program) I doubt whether
> it's worth our time to configure this stuff at compile-time,
> and that it's fine to do a run-time check on all platforms.

OTOH, on POSIX-conforming systems (which includes GNU/Linux, so it may
be the majority of systems in use), -1 may well be a valid UID/GID.

That’s why I was conservative and decided to ifdef that.  This ifdef
also served as a documentation that this is a GNU extension to POSIX.

Perhaps it’d be safer to keep an ifdef and list all the OSes known to
have this behavior?

Thanks,
Ludo’.

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

From: "Alan Curry" <pacman-cu <at> kosh.dhis.org>
To: ludo <at> gnu.org (Ludovic Courtès)
Cc: Paul Eggert <eggert <at> cs.ucla.edu>, bug-hurd <at> gnu.org,
	Jim Meyering <jim <at> meyering.net>, 10021 <at> debbugs.gnu.org
Subject: Re: bug#10021: [PATCH id] Add error-checking on GNU
Date: Sun, 13 Nov 2011 17:32:22 -0500 (GMT+5)

Ludovic =?UTF-8?Q?Court=C3=A8s?= writes:
> 
> OTOH, on POSIX-conforming systems (which includes GNU/Linux, so it may
> be the majority of systems in use), -1 may well be a valid UID/GID.

That's a bizarre statement.

  3.428 User ID

  A non-negative integer that is used to identify a system user. When the
  identity of a user is associated with a process, a user ID value is
  referred to as a real user ID, an effective user ID, or a saved
  set-user-ID.

chown(2) uses (uid_t)-1 and (gid_t)-1 as the "don't change" special values.
So does setreuid(2)/setregid(2).

setuid(-1) isn't documented as special. Trying it out, it seems to be treated
as equivalent to setuid(1). Not what I expected, but it doesn't really
support your "-1 is a valid uid" theory.

-- 
Alan Curry

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

From: Jim Meyering <jim <at> meyering.net>
To: ludo <at> gnu.org (Ludovic Courtès)
Cc: bug-hurd <at> gnu.org, 10021 <at> debbugs.gnu.org
Subject: Re: bug#10021: [PATCH id] Add error-checking on GNU
Date: Mon, 14 Nov 2011 08:22:28 +0100

Ludovic Courtès wrote:
> Jim Meyering <jim <at> meyering.net> skribis:
>
>> Oh, I also had to make the new test script executable,
>> or else "make check" would fail.
>
> It works for me even if it’s not executable when I run “make check
> TESTS=id/gnu-zero-uids”, apparently because it’s run this way:
>
>   execve("/bin/sh", ["/bin/sh", "./shell-or-perl", "--test-name",
> "id/gnu-zero-uids", "--srcdir", ".", "--shell", "/bin/sh", "--perl",
> "perl", "--", "./id/gnu-zero-uids"], [/* 96 vars */]) = 0

The test itself runs either way.  However there's a make check
sub-test (vc_exe_in_TESTS) that ensures all such scripts are
consistently chmod a+x.  It was failing.

> It might be different when running just “make check”.
>
> (I tested it on GNU/Linux and GNU/Hurd.)
>
>> Since I've made so many changes, I'll wait to hear back
>> from you before pushing it.
>
> Looks good to me, thank you!
>
>> +  test "$(uname)" = GNU \
>
> So you’re assuming Bash, right?

No.  The test starts with the usual invocation of init.sh,
which ensures we're using a shell that handles $(...).

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

From: Jim Meyering <jim <at> meyering.net>
To: ludo <at> gnu.org (Ludovic Courtès)
Cc: Paul Eggert <eggert <at> cs.ucla.edu>, bug-hurd <at> gnu.org, 10021 <at> debbugs.gnu.org
Subject: Re: bug#10021: [PATCH id] Add error-checking on GNU
Date: Mon, 14 Nov 2011 09:52:22 +0100

Ludovic Courtès wrote:

> Hi Paul,
>
> Paul Eggert <eggert <at> cs.ucla.edu> skribis:
>
>> On 11/12/11 13:48, Ludovic Courtès wrote:
>>> +#ifdef __GNU__
>>> +      if (euid == -1 && !use_real
>>> +          && !just_group && !just_group_list && !just_context)
>>> +        error (EXIT_FAILURE, errno, _("cannot get effective UID"));
>>> +#endif
>>
>> I suggest removing the "#ifdef __GNU__" here and in its other
>> three uses in the patch, as functions like as geteuid() can fail on
>> a few non-GNU systems too.  See:
>>
>> http://publib.boulder.ibm.com/infocenter/iseries/v5r3/index.jsp?topic=%2Fapis%2Fgeteuid.htm
>>
>> For this particular application (the 'id' program) I doubt whether
>> it's worth our time to configure this stuff at compile-time,
>> and that it's fine to do a run-time check on all platforms.
>
> OTOH, on POSIX-conforming systems (which includes GNU/Linux, so it may
> be the majority of systems in use), -1 may well be a valid UID/GID.
>
> That’s why I was conservative and decided to ifdef that.  This ifdef
> also served as a documentation that this is a GNU extension to POSIX.
>
> Perhaps it’d be safer to keep an ifdef and list all the OSes known to
> have this behavior?

Thanks for persevering.
I tested this on a gnu/linux system and found that I could indeed create a
user with a UID of 2^32-1 (though not directly via adduser -- it imposes
a maximum of 2^32-2), and that without something like those #ifdefs,
GNU id would mistakenly fail for such a user:

    id: cannot get effective UID: No such file or directory

However, with the patch below, it does this:

    $ ./id -u
    4294967295

diff --git a/src/id.c b/src/id.c
index 19cf0cf..047e40b 100644
--- a/src/id.c
+++ b/src/id.c
@@ -38,6 +38,13 @@
   proper_name ("Arnold Robbins"), \
   proper_name ("David MacKenzie")

+/* Whether the functions getuid, geteuid, getgid and getegid may fail.  */
+#ifdef __GNU__
+# define GETID_MAY_FAIL 1
+#else
+# define GETID_MAY_FAIL 0
+#endif
+
 /* If nonzero, output only the SELinux context. -Z */
 static int just_context = 0;

@@ -202,21 +209,21 @@ main (int argc, char **argv)
   else
     {
       euid = geteuid ();
-      if (euid == -1 && !use_real
+      if (GETID_MAY_FAIL && euid == -1 && !use_real
           && !just_group && !just_group_list && !just_context)
         error (EXIT_FAILURE, errno, _("cannot get effective UID"));

       ruid = getuid ();
-      if (ruid == -1 && use_real
+      if (GETID_MAY_FAIL && ruid == -1 && use_real
           && !just_group && !just_group_list && !just_context)
         error (EXIT_FAILURE, errno, _("cannot get real UID"));

       egid = getegid ();
-      if (egid == -1 && !use_real && !just_user)
+      if (GETID_MAY_FAIL && egid == -1 && !use_real && !just_user)
         error (EXIT_FAILURE, errno, _("cannot get effective GID"));

       rgid = getgid ();
-      if (rgid == -1 && use_real && !just_user)
+      if (GETID_MAY_FAIL && rgid == -1 && use_real && !just_user)
         error (EXIT_FAILURE, errno, _("cannot get real GID"));
     }

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

From: ludo <at> gnu.org (Ludovic Courtès)
To: Jim Meyering <jim <at> meyering.net>
Cc: Paul Eggert <eggert <at> cs.ucla.edu>, bug-hurd <at> gnu.org, 10021 <at> debbugs.gnu.org
Subject: Re: bug#10021: [PATCH id] Add error-checking on GNU
Date: Mon, 14 Nov 2011 14:33:56 +0100

Hi Jim,

Jim Meyering <jim <at> meyering.net> skribis:

> Ludovic Courtès wrote:
>
>> Hi Paul,
>>
>> Paul Eggert <eggert <at> cs.ucla.edu> skribis:
>>
>>> On 11/12/11 13:48, Ludovic Courtès wrote:
>>>> +#ifdef __GNU__
>>>> +      if (euid == -1 && !use_real
>>>> +          && !just_group && !just_group_list && !just_context)
>>>> +        error (EXIT_FAILURE, errno, _("cannot get effective UID"));
>>>> +#endif
>>>
>>> I suggest removing the "#ifdef __GNU__" here and in its other
>>> three uses in the patch, as functions like as geteuid() can fail on
>>> a few non-GNU systems too.  See:
>>>
>>> http://publib.boulder.ibm.com/infocenter/iseries/v5r3/index.jsp?topic=%2Fapis%2Fgeteuid.htm
>>>
>>> For this particular application (the 'id' program) I doubt whether
>>> it's worth our time to configure this stuff at compile-time,
>>> and that it's fine to do a run-time check on all platforms.
>>
>> OTOH, on POSIX-conforming systems (which includes GNU/Linux, so it may
>> be the majority of systems in use), -1 may well be a valid UID/GID.
>>
>> That’s why I was conservative and decided to ifdef that.  This ifdef
>> also served as a documentation that this is a GNU extension to POSIX.
>>
>> Perhaps it’d be safer to keep an ifdef and list all the OSes known to
>> have this behavior?
>
> Thanks for persevering.
> I tested this on a gnu/linux system and found that I could indeed create a
> user with a UID of 2^32-1 (though not directly via adduser -- it imposes
> a maximum of 2^32-2), and that without something like those #ifdefs,
> GNU id would mistakenly fail for such a user:
>
>     id: cannot get effective UID: No such file or directory
>
> However, with the patch below, it does this:
>
>     $ ./id -u
>     4294967295

OK, good to know.  Thank *you* for persevering!

Ludo’.

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

From: ludo <at> gnu.org (Ludovic Courtès)
To: "Alan Curry" <pacman-cu <at> kosh.dhis.org>
Cc: Paul Eggert <eggert <at> cs.ucla.edu>, bug-hurd <at> gnu.org,
	Jim Meyering <jim <at> meyering.net>, 10021 <at> debbugs.gnu.org
Subject: Re: bug#10021: [PATCH id] Add error-checking on GNU
Date: Mon, 14 Nov 2011 14:35:42 +0100

Hi,

"Alan Curry" <pacman-cu <at> kosh.dhis.org> skribis:

> Ludovic =?UTF-8?Q?Court=C3=A8s?= writes:
>> 
>> OTOH, on POSIX-conforming systems (which includes GNU/Linux, so it may
>> be the majority of systems in use), -1 may well be a valid UID/GID.
>
> That's a bizarre statement.
>
>   3.428 User ID
>
>   A non-negative integer

It’s always non negative since glibc defines it as unsigned (see
__UID_T_TYPE in typesizes.h.)

I really meant 2^32 - 1.

Thanks,
Ludo’.

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

From: Jim Meyering <jim <at> meyering.net>
To: ludo <at> gnu.org (Ludovic Courtès)
Cc: Paul Eggert <eggert <at> cs.ucla.edu>, bug-hurd <at> gnu.org, 10021 <at> debbugs.gnu.org
Subject: Re: bug#10021: [PATCH id] Add error-checking on GNU
Date: Mon, 14 Nov 2011 14:42:32 +0100

Ludovic Courtès wrote:
...
>> However, with the patch below, it does this:
>>
>>     $ ./id -u
>>     4294967295
>
> OK, good to know.  Thank *you* for persevering!

I'll take that as an ACK ;-)
Pushed.

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

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Jim Meyering <jim <at> meyering.net>
Cc: Ludovic Courtès <ludo <at> gnu.org>, bug-hurd <at> gnu.org,
	10021 <at> debbugs.gnu.org
Subject: Re: bug#10021: [PATCH id] Add error-checking on GNU
Date: Mon, 14 Nov 2011 10:54:32 -0800

On 11/14/11 00:52, Jim Meyering wrote:
> I tested this on a gnu/linux system and found that I could indeed create a
> user with a UID of 2^32-1

If we really want to support that, there would be
a lot of other coreutils code that would need fixing,
no?  For example, the command

  chown 4294967295 /tmp/foo

always fails, but presumably on your host which has a
user (call it ubig) with UID 4294967295, the command

  chown ubig /tmp/foo

is a no-op?  Ouch.

I can see where this would have some serious security
implications.  Instead of trying to cater to systems
like that, it's probably better just to say "Don't Do That",
i.e., GNU/Linux systems that have a user with ID == (uid_t) -1
are improperly set up and coreutils does not especially
cater to such systems.

That being said, it's OK if 'id' does support such systems,
(if only to diagnose their brokenness :-).  But looking at
the current code, id is making the unportable assumption that
uid_t is at least as wide as int.  And the way that id works
around the GNU porting problem isn't quite right, as there
are systems other than GNU that have the problem.

How about this patch?  It's simpler and should fix the
issues mentioned in the previous paragraph.

id: handle (uid_t) -1 more portably
* src/id.c (GETID_MAY_FAIL): Remove.
(main): Check for negative return values, not for -1.
The old code was incorrect if uid_t was narrower than int,
regardless of whether we were on a GNU or a POSIX platform.
The new code is simpler and doesn't need GETID_MAY_FAIL.
(print_full_info): Remove unnecessary cast to -1.
diff --git a/src/id.c b/src/id.c
index 047e40b..7bd8ebd 100644
--- a/src/id.c
+++ b/src/id.c
@@ -38,13 +38,6 @@
   proper_name ("Arnold Robbins"), \
   proper_name ("David MacKenzie")

-/* Whether the functions getuid, geteuid, getgid and getegid may fail.  */
-#ifdef __GNU__
-# define GETID_MAY_FAIL 1
-#else
-# define GETID_MAY_FAIL 0
-#endif
-
 /* If nonzero, output only the SELinux context. -Z */
 static int just_context = 0;

@@ -208,22 +201,35 @@ main (int argc, char **argv)
     }
   else
     {
+      /* On GNU hosts, getuid etc. can fail and return -1.  On POSIX
+         hosts, such failures are not allowed and (uid_t) -1 may be a
+         valid UID if uid_t is unsigned.  To handle both cases
+         correctly, consider getuid etc. to fail if it returns a
+         negative value.
+
+         POSIX hosts should not give users a UID equal to (uid_t) -1
+         even if uid_t is unsigned, as system calls like chown would
+         behave unexpectedly with such a UID, and in general coreutils
+         therefore does not support such a UID.  However, 'id' makes a
+         special attempt to handle it, if only to help people diagnose
+         the problem.  */
+
       euid = geteuid ();
-      if (GETID_MAY_FAIL && euid == -1 && !use_real
+      if (euid < 0 && !use_real
           && !just_group && !just_group_list && !just_context)
         error (EXIT_FAILURE, errno, _("cannot get effective UID"));

       ruid = getuid ();
-      if (GETID_MAY_FAIL && ruid == -1 && use_real
+      if (ruid < 0 && use_real
           && !just_group && !just_group_list && !just_context)
         error (EXIT_FAILURE, errno, _("cannot get real UID"));

       egid = getegid ();
-      if (GETID_MAY_FAIL && egid == -1 && !use_real && !just_user)
+      if (egid < 0 && !use_real && !just_user)
         error (EXIT_FAILURE, errno, _("cannot get effective GID"));

       rgid = getgid ();
-      if (GETID_MAY_FAIL && rgid == -1 && use_real && !just_user)
+      if (rgid < 0 && use_real && !just_user)
         error (EXIT_FAILURE, errno, _("cannot get real GID"));
     }

@@ -316,7 +322,7 @@ print_full_info (const char *username)
     gid_t *groups;
     int i;

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

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

From: Roland McGrath <roland <at> hack.frob.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: Ludovic Courtès <ludo <at> gnu.org>, bug-hurd <at> gnu.org,
	Jim Meyering <jim <at> meyering.net>, 10021 <at> debbugs.gnu.org
Subject: Re: bug#10021: [PATCH id] Add error-checking on GNU
Date: Mon, 14 Nov 2011 11:02:08 -0800 (PST)

I think you can portably detect the failure case with the errno=0 method,
rather than relying on #ifdef.

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

From: Eric Blake <eblake <at> redhat.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: Ludovic Courtès <ludo <at> gnu.org>, bug-hurd <at> gnu.org,
	Jim Meyering <jim <at> meyering.net>, 10021 <at> debbugs.gnu.org
Subject: Re: bug#10021: [PATCH id] Add error-checking on GNU
Date: Mon, 14 Nov 2011 12:12:35 -0700

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

On 11/14/2011 11:54 AM, Paul Eggert wrote:
>    else
>      {
> +      /* On GNU hosts, getuid etc. can fail and return -1.  On POSIX
> +         hosts, such failures are not allowed and (uid_t) -1 may be a
> +         valid UID if uid_t is unsigned.

That doesn't read correctly.  You are correct that POSIX states that
getuid() cannot fail in a conforming environment (therefore, the only
way that GNU getuid fails is if you have done something outside the
realms of a conforming environment).  However, POSIX is quite clear that
chown() must honor (uid_t)(-1) as a means of not altering the uid of a
file, therefore, POSIX explicitly rejects (uid_t)(-1) as a valid UID.

POSIX leaves it up to the implementation whether the type uid_t is
signed or unsigned, but also specifies that valid uids are non-negative
integers (so if uid_t is signed, half the possible values will never be
encountered as valid uids).

>  To handle both cases
> +         correctly, consider getuid etc. to fail if it returns a
> +         negative value.

But if uid_t is an unsigned type (which POSIX permits), then it will
never be a negative value.

> +
> +         POSIX hosts should not give users a UID equal to (uid_t) -1
> +         even if uid_t is unsigned, as system calls like chown would
> +         behave unexpectedly with such a UID, and in general coreutils
> +         therefore does not support such a UID.  However, 'id' makes a
> +         special attempt to handle it, if only to help people diagnose
> +         the problem.  */
> +
>        euid = geteuid ();
> -      if (GETID_MAY_FAIL && euid == -1 && !use_real
> +      if (euid < 0 && !use_real

That is, how can this work?  On systems where uid_t is signed, it makes
sense, but on systems where uid_t is unsigned, this will always be false
(and probably trigger a gcc warning), and still fail to catch the
(uid_t)(-1) case that we are trying to diagnose.

-- 
Eric Blake   eblake <at> redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

[signature.asc (application/pgp-signature, attachment)]

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

From: ludo <at> gnu.org (Ludovic Courtès)
To: Eric Blake <eblake <at> redhat.com>
Cc: Paul Eggert <eggert <at> cs.ucla.edu>, bug-hurd <at> gnu.org,
	Jim Meyering <jim <at> meyering.net>, 10021 <at> debbugs.gnu.org
Subject: Re: bug#10021: [PATCH id] Add error-checking on GNU
Date: Mon, 14 Nov 2011 21:29:42 +0100

Hi,

Eric Blake <eblake <at> redhat.com> skribis:

> On 11/14/2011 11:54 AM, Paul Eggert wrote:

[...]

>>        euid = geteuid ();
>> -      if (GETID_MAY_FAIL && euid == -1 && !use_real
>> +      if (euid < 0 && !use_real
>
> That is, how can this work?  On systems where uid_t is signed, it makes
> sense, but on systems where uid_t is unsigned, this will always be false

And again, it *is* unsigned in glibc.

Thanks,
Ludo’.

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

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Eric Blake <eblake <at> redhat.com>
Cc: Ludovic Courtès <ludo <at> gnu.org>, bug-hurd <at> gnu.org,
	Jim Meyering <jim <at> meyering.net>, 10021 <at> debbugs.gnu.org
Subject: Re: bug#10021: [PATCH id] Add error-checking on GNU
Date: Mon, 14 Nov 2011 14:07:16 -0800

On 11/14/11 11:12, Eric Blake wrote:
> POSIX explicitly rejects (uid_t)(-1) as a valid UID.

That depends on what one means by "valid".  You're right about chown
of course, but it's not clear that POSIX absolutely prohibits getuid
from returning (uid_t) -1.  And even if POSIX did prohibit that,
GNU/Linux does not.

> if uid_t is an unsigned type (which POSIX permits), then it will
> never be a negative value.

Yes, that's correct, and the proposed patch relies on this.

I see that the proposed patch's comments weren't that clear about this,
and I enclose a revised proposal below, which is the same code but
(I hope) clearer comments.

>> -      if (GETID_MAY_FAIL && euid == -1 && !use_real
>> +      if (euid < 0 && !use_real
> 
> That is, how can this work?  On systems where uid_t is signed, it makes
> sense, but on systems where uid_t is unsigned, this will always be false
> (and probably trigger a gcc warning), and still fail to catch the
> (uid_t)(-1) case that we are trying to diagnose.

The idea is that it is not id's problem to diagnose the bug, because
no bug is triggered by id itself.  id should report what it found,
namely, a user with a funny userid (and presumably equally-funny name...).

That is, it's not id's job to anticipate bugs that may occur in other
applications.

We don't need to worry about the GCC warning, because there are
zillions of other places in coreutils that trigger the same warning.
Our approach is to ask builders to compile without that warning
enabled, or to ignore the warning, whichever they prefer.

Here's the revised proposal.

id: handle (uid_t) -1 more portably
* src/id.c (GETID_MAY_FAIL): Remove.
(main): Check for negative return values, not for -1.
The old code was incorrect if uid_t was narrower than int,
regardless of whether we were on a GNU or a POSIX platform.
The new code is simpler and doesn't need GETID_MAY_FAIL.
(print_full_info): Remove unnecessary cast to -1.
diff --git a/src/id.c b/src/id.c
index 047e40b..b3ee437 100644
--- a/src/id.c
+++ b/src/id.c
@@ -38,13 +38,6 @@
   proper_name ("Arnold Robbins"), \
   proper_name ("David MacKenzie")

-/* Whether the functions getuid, geteuid, getgid and getegid may fail.  */
-#ifdef __GNU__
-# define GETID_MAY_FAIL 1
-#else
-# define GETID_MAY_FAIL 0
-#endif
-
 /* If nonzero, output only the SELinux context. -Z */
 static int just_context = 0;

@@ -208,22 +201,36 @@ main (int argc, char **argv)
     }
   else
     {
+      /* On GNU/Hurd hosts, getuid etc. can fail and return -1.
+         However, on GNU/Linux hosts, uid_t is an unsigned value and
+         getuid etc. can return the positive value (uid_t) -1.  To
+         handle both cases correctly, consider getuid etc. to fail if
+         it returns a negative value (a value that is impossible on
+         GNU/Linux hosts).
+
+         GNU/Linux sysadmins should not give users the UID (uid_t) -1
+         even though uid_t is unsigned, as system calls like chown would
+         not have the desired behavior with such a UID, and other
+         coreutils applications therefore do not support such a UID.
+         However, 'id' makes a special attempt to handle this UID, to
+         help people diagnose the problem.  */
+
       euid = geteuid ();
-      if (GETID_MAY_FAIL && euid == -1 && !use_real
+      if (euid < 0 && !use_real
           && !just_group && !just_group_list && !just_context)
         error (EXIT_FAILURE, errno, _("cannot get effective UID"));

       ruid = getuid ();
-      if (GETID_MAY_FAIL && ruid == -1 && use_real
+      if (ruid < 0 && use_real
           && !just_group && !just_group_list && !just_context)
         error (EXIT_FAILURE, errno, _("cannot get real UID"));

       egid = getegid ();
-      if (GETID_MAY_FAIL && egid == -1 && !use_real && !just_user)
+      if (egid < 0 && !use_real && !just_user)
         error (EXIT_FAILURE, errno, _("cannot get effective GID"));

       rgid = getgid ();
-      if (GETID_MAY_FAIL && rgid == -1 && use_real && !just_user)
+      if (rgid < 0 && use_real && !just_user)
         error (EXIT_FAILURE, errno, _("cannot get real GID"));
     }

@@ -316,7 +323,7 @@ print_full_info (const char *username)
     gid_t *groups;
     int i;

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

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

From: ludo <at> gnu.org (Ludovic Courtès)
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: bug-hurd <at> gnu.org, Eric Blake <eblake <at> redhat.com>,
	Jim Meyering <jim <at> meyering.net>, 10021 <at> debbugs.gnu.org
Subject: Re: bug#10021: [PATCH id] Add error-checking on GNU
Date: Tue, 15 Nov 2011 14:07:13 +0100

Hi Paul,

Paul Eggert <eggert <at> cs.ucla.edu> skribis:

> -      if (GETID_MAY_FAIL && euid == -1 && !use_real
> +      if (euid < 0 && !use_real
>            && !just_group && !just_group_list && !just_context)
>          error (EXIT_FAILURE, errno, _("cannot get effective UID"));

On GNU/Hurd, no error would ever be raised (since uid_t is unsigned),
which defeats the purpose of the patch initially proposed.

Thanks,
Ludo’.

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

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: bug-hurd <at> gnu.org, Eric Blake <eblake <at> redhat.com>,
	Jim Meyering <jim <at> meyering.net>, 10021 <at> debbugs.gnu.org
Subject: Re: bug#10021: [PATCH id] Add error-checking on GNU
Date: Tue, 15 Nov 2011 08:59:51 -0800

On 11/15/11 05:07, Ludovic Courtès wrote:

> On GNU/Hurd, no error would ever be raised (since uid_t is unsigned),

Ouch.  Thanks, now I understand Roland's suggestion.
How about this patch instead?


id: handle (uid_t) -1 more portably
* src/id.c (GETID_MAY_FAIL): Remove.
(main): Check for nonzero errno, rather than having a compile-time
GETID_MAY_FAIL guess.  Suggested by Roland McGrath in
<http://debbugs.gnu.org/cgi/bugreport.cgi?bug=10021#47>.
Also, the old code was incorrect if uid_t was narrower than int.
(print_full_info): Remove unnecessary cast to -1.
diff --git a/src/id.c b/src/id.c
index 047e40b..9fa93f8 100644
--- a/src/id.c
+++ b/src/id.c
@@ -38,13 +38,6 @@
   proper_name ("Arnold Robbins"), \
   proper_name ("David MacKenzie")

-/* Whether the functions getuid, geteuid, getgid and getegid may fail.  */
-#ifdef __GNU__
-# define GETID_MAY_FAIL 1
-#else
-# define GETID_MAY_FAIL 0
-#endif
-
 /* If nonzero, output only the SELinux context. -Z */
 static int just_context = 0;

@@ -208,22 +201,32 @@ main (int argc, char **argv)
     }
   else
     {
+      /* POSIX says getuid etc. cannot fail, but they can fail under
+         GNU/Hurd and a few other systems.  Test for failure by
+         checking errno.  */
+      uid_t NO_UID = -1;
+      gid_t NO_GID = -1;
+
+      errno = 0;
       euid = geteuid ();
-      if (GETID_MAY_FAIL && euid == -1 && !use_real
+      if (euid == NO_UID && errno && !use_real
           && !just_group && !just_group_list && !just_context)
         error (EXIT_FAILURE, errno, _("cannot get effective UID"));

+      errno = 0;
       ruid = getuid ();
-      if (GETID_MAY_FAIL && ruid == -1 && use_real
+      if (ruid == NO_UID && errno && use_real
           && !just_group && !just_group_list && !just_context)
         error (EXIT_FAILURE, errno, _("cannot get real UID"));

+      errno = 0;
       egid = getegid ();
-      if (GETID_MAY_FAIL && egid == -1 && !use_real && !just_user)
+      if (egid == NO_GID && errno && !use_real && !just_user)
         error (EXIT_FAILURE, errno, _("cannot get effective GID"));

+      errno = 0;
       rgid = getgid ();
-      if (GETID_MAY_FAIL && rgid == -1 && use_real && !just_user)
+      if (rgid == NO_GID && errno && use_real && !just_user)
         error (EXIT_FAILURE, errno, _("cannot get real GID"));
     }

@@ -316,7 +319,7 @@ print_full_info (const char *username)
     gid_t *groups;
     int i;

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

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

From: Eric Blake <eblake <at> redhat.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: Ludovic Courtès <ludo <at> gnu.org>, bug-hurd <at> gnu.org,
	Jim Meyering <jim <at> meyering.net>, 10021 <at> debbugs.gnu.org
Subject: Re: bug#10021: [PATCH id] Add error-checking on GNU
Date: Tue, 15 Nov 2011 10:09:24 -0700

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

On 11/15/2011 09:59 AM, Paul Eggert wrote:
> On 11/15/11 05:07, Ludovic Courtès wrote:
> 
>> On GNU/Hurd, no error would ever be raised (since uid_t is unsigned),
> 
> Ouch.  Thanks, now I understand Roland's suggestion.
> How about this patch instead?

>    else
>      {
> +      /* POSIX says getuid etc. cannot fail, but they can fail under
> +         GNU/Hurd and a few other systems.  Test for failure by
> +         checking errno.  */
> +      uid_t NO_UID = -1;
> +      gid_t NO_GID = -1;
> +
> +      errno = 0;
>        euid = geteuid ();
> -      if (GETID_MAY_FAIL && euid == -1 && !use_real
> +      if (euid == NO_UID && errno && !use_real
>            && !just_group && !just_group_list && !just_context)
>          error (EXIT_FAILURE, errno, _("cannot get effective UID"));
> 

Still debatable.  POSIX explicitly states that the condition of errno
after a successful call to a standardized function is unspecified; that
is, a successful geteuid() may pollute errno, but it's okay, because the
user shouldn't be inspecting errno after geteuid().

It might be worth proposing a change to POSIX to require that geteuid()
and friends leave errno unchanged on success (in order to allow for the
GNU extension of setting errno on failure, even though POSIX did not
reserve a specific value for failure); I'll pursue that course.

But I like this version better than any previous one about trying to
reject result < 0.

-- 
Eric Blake   eblake <at> redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

[signature.asc (application/pgp-signature, attachment)]

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

From: Jim Meyering <jim <at> meyering.net>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: bug-hurd <at> gnu.org, Ludovic Courtès <ludo <at> gnu.org>,
	Eric Blake <eblake <at> redhat.com>, 10021 <at> debbugs.gnu.org
Subject: Re: bug#10021: [PATCH id] Add error-checking on GNU
Date: Tue, 15 Nov 2011 18:10:40 +0100

Paul Eggert wrote:
> On 11/15/11 05:07, Ludovic Courtès wrote:
>
>> On GNU/Hurd, no error would ever be raised (since uid_t is unsigned),
>
> Ouch.  Thanks, now I understand Roland's suggestion.
> How about this patch instead?
>
> id: handle (uid_t) -1 more portably
> * src/id.c (GETID_MAY_FAIL): Remove.
> (main): Check for nonzero errno, rather than having a compile-time
> GETID_MAY_FAIL guess.  Suggested by Roland McGrath in
> <http://debbugs.gnu.org/cgi/bugreport.cgi?bug=10021#47>.
> Also, the old code was incorrect if uid_t was narrower than int.
> (print_full_info): Remove unnecessary cast to -1.
> diff --git a/src/id.c b/src/id.c
> index 047e40b..9fa93f8 100644
> --- a/src/id.c
> +++ b/src/id.c
> @@ -38,13 +38,6 @@
>    proper_name ("Arnold Robbins"), \
>    proper_name ("David MacKenzie")
>
> -/* Whether the functions getuid, geteuid, getgid and getegid may fail.  */
> -#ifdef __GNU__
> -# define GETID_MAY_FAIL 1
> -#else
> -# define GETID_MAY_FAIL 0
> -#endif
> -
>  /* If nonzero, output only the SELinux context. -Z */
>  static int just_context = 0;
>
> @@ -208,22 +201,32 @@ main (int argc, char **argv)
>      }
>    else
>      {
> +      /* POSIX says getuid etc. cannot fail, but they can fail under
> +         GNU/Hurd and a few other systems.  Test for failure by
> +         checking errno.  */
> +      uid_t NO_UID = -1;
> +      gid_t NO_GID = -1;
> +
> +      errno = 0;
>        euid = geteuid ();
> -      if (GETID_MAY_FAIL && euid == -1 && !use_real
> +      if (euid == NO_UID && errno && !use_real

I like that.  Thanks!

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

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Jim Meyering <jim <at> meyering.net>
Cc: bug-hurd <at> gnu.org, Ludovic Courtès <ludo <at> gnu.org>,
	Eric Blake <eblake <at> redhat.com>, 10021 <at> debbugs.gnu.org
Subject: Re: bug#10021: [PATCH id] Add error-checking on GNU
Date: Tue, 15 Nov 2011 09:41:38 -0800

On 11/15/11 09:10, Jim Meyering wrote:
> I like that.  Thanks!

You're welcome; I pushed it.

Similar fixes are needed for groups.c,
install.c, su.c, test.c, and whoami.c,
due to the possibility of getuid etc.
failing on GNU/Hurd.

I'll add this to my list of things to do, and
post a patch here, unless someone else gets to it
first.

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

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: 10021 <at> debbugs.gnu.org
Subject: Re: bug#10021: [PATCH id] Add error-checking on GNU
Date: Tue, 15 Nov 2011 13:26:02 -0800

I found yet-another tricky bug in that id.c code,
and fixed it as follows:

From 5ca593e87eb1361b4696dd1e002a8ee310a5d1f6 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert <at> cs.ucla.edu>
Date: Tue, 15 Nov 2011 13:23:24 -0800
Subject: [PATCH] id: fix bug when euid != ruid

* src/id.c (main): Report an error if no args are given and getuid
fails, because print_full_info needs ruid.  Redo code so that
getuid and friends are invoked only when needed; this makes the
code easier to follow, and is how I found the above bug.
---
 src/id.c |   51 ++++++++++++++++++++++++++++++---------------------
 1 files changed, 30 insertions(+), 21 deletions(-)

diff --git a/src/id.c b/src/id.c
index 8f7ce9e..1047149 100644
--- a/src/id.c
+++ b/src/id.c
@@ -207,27 +207,36 @@ main (int argc, char **argv)
       uid_t NO_UID = -1;
       gid_t NO_GID = -1;
 
-      errno = 0;
-      euid = geteuid ();
-      if (euid == NO_UID && errno && !use_real
-          && !just_group && !just_group_list && !just_context)
-        error (EXIT_FAILURE, errno, _("cannot get effective UID"));
-
-      errno = 0;
-      ruid = getuid ();
-      if (ruid == NO_UID && errno && use_real
-          && !just_group && !just_group_list && !just_context)
-        error (EXIT_FAILURE, errno, _("cannot get real UID"));
-
-      errno = 0;
-      egid = getegid ();
-      if (egid == NO_GID && errno && !use_real && !just_user)
-        error (EXIT_FAILURE, errno, _("cannot get effective GID"));
-
-      errno = 0;
-      rgid = getgid ();
-      if (rgid == NO_GID && errno && use_real && !just_user)
-        error (EXIT_FAILURE, errno, _("cannot get real GID"));
+      if (just_user ? !use_real
+          : !just_group && !just_group_list && !just_context)
+        {
+          errno = 0;
+          euid = geteuid ();
+          if (euid == NO_UID && errno)
+            error (EXIT_FAILURE, errno, _("cannot get effective UID"));
+        }
+
+      if (just_user ? use_real
+          : !just_group && (just_group_list || !just_context))
+        {
+          errno = 0;
+          ruid = getuid ();
+          if (ruid == NO_UID && errno)
+            error (EXIT_FAILURE, errno, _("cannot get real UID"));
+        }
+
+      if (!just_user && (just_group || just_group_list || !just_context))
+        {
+          errno = 0;
+          egid = getegid ();
+          if (egid == NO_GID && errno)
+            error (EXIT_FAILURE, errno, _("cannot get effective GID"));
+
+          errno = 0;
+          rgid = getgid ();
+          if (rgid == NO_GID && errno)
+            error (EXIT_FAILURE, errno, _("cannot get real GID"));
+        }
     }
 
   if (just_user)
-- 
1.7.6.4

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

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Jim Meyering <jim <at> meyering.net>
Cc: bug-hurd <at> gnu.org, Ludovic Courtès <ludo <at> gnu.org>,
	Eric Blake <eblake <at> redhat.com>, 10021 <at> debbugs.gnu.org
Subject: Re: bug#10021: [PATCH id] Add error-checking on GNU
Date: Wed, 16 Nov 2011 00:44:16 -0800

Here are proposed patches for the other coreutils applications
that invoke getuid et al.

port to GNU hosts, where getuid and friends can fail
* src/groups.c (main):
* src/install.c (need_copy):
* src/su.c (log_su):
* src/test.c (unary_operator):
* src/whoami.c (main):
Don't assume that getuid and friends always succeed.
This fixes the same problem that we recently fixed with 'id'.
diff --git a/src/groups.c b/src/groups.c
index abb7bc7..279b831 100644
--- a/src/groups.c
+++ b/src/groups.c
@@ -97,9 +97,23 @@ main (int argc, char **argv)
   if (optind == argc)
     {
       /* No arguments.  Divulge the details of the current process. */
+      uid_t NO_UID = -1;
+      gid_t NO_GID = -1;
+
+      errno = 0;
       ruid = getuid ();
+      if (ruid == NO_UID && errno)
+        error (EXIT_FAILURE, errno, _("cannot get real UID"));
+
+      errno = 0;
       egid = getegid ();
+      if (egid == NO_GID && errno)
+        error (EXIT_FAILURE, errno, _("cannot get effective GID"));
+
+      errno = 0;
       rgid = getgid ();
+      if (rgid == NO_GID && errno)
+        error (EXIT_FAILURE, errno, _("cannot get real GID"));

       if (!print_group_list (NULL, ruid, rgid, egid, true))
         ok = false;
diff --git a/src/install.c b/src/install.c
index dbff9c9..94534f8 100644
--- a/src/install.c
+++ b/src/install.c
@@ -192,9 +192,27 @@ need_copy (const char *src_name, const char *dest_name,
     return true;

   if (src_sb.st_size != dest_sb.st_size
-      || (dest_sb.st_mode & CHMOD_MODE_BITS) != mode
-      || dest_sb.st_uid != (owner_id == (uid_t) -1 ? getuid () : owner_id)
-      || dest_sb.st_gid != (group_id == (gid_t) -1 ? getgid () : group_id))
+      || (dest_sb.st_mode & CHMOD_MODE_BITS) != mode)
+    return true;
+
+  if (owner_id == (uid_t) -1)
+    {
+      errno = 0;
+      uid_t ruid = getuid ();
+      if ((ruid == (uid_t) -1 && errno) || dest_sb.st_uid != ruid)
+        return true;
+    }
+  else if (dest_sb.st_uid != owner_id)
+    return true;
+
+  if (group_id == (uid_t) -1)
+    {
+      errno = 0;
+      gid_t rgid = getgid ();
+      if ((rgid == (uid_t) -1 && errno) || dest_sb.st_gid != rgid)
+        return true;
+    }
+  else if (dest_sb.st_gid != group_id)
     return true;

   /* compare SELinux context if preserving */
diff --git a/src/su.c b/src/su.c
index 081ecb2..b1ba2a7 100644
--- a/src/su.c
+++ b/src/su.c
@@ -173,7 +173,10 @@ log_su (struct passwd const *pw, bool successful)
     {
       /* getlogin can fail -- usually due to lack of utmp entry.
          Resort to getpwuid.  */
-      struct passwd *pwd = getpwuid (getuid ());
+      errno = 0;
+      uid_t ruid = getuid ();
+      uid_t NO_UID = -1;
+      struct passwd *pwd = (ruid == NO_UID && errno ? NULL : getpwuid (ruid));
       old_user = (pwd ? pwd->pw_name : "");
     }
   tty = ttyname (STDERR_FILENO);
diff --git a/src/test.c b/src/test.c
index 362df65..1b06ca8 100644
--- a/src/test.c
+++ b/src/test.c
@@ -414,13 +414,21 @@ unary_operator (void)

     case 'O':			/* File is owned by you? */
       unary_advance ();
-      return (stat (argv[pos - 1], &stat_buf) == 0
-              && (geteuid () == stat_buf.st_uid));
+      if (stat (argv[pos - 1], &stat_buf) != 0)
+        return false;
+      errno = 0;
+      uid_t euid = geteuid ();
+      uid_t NO_UID = -1;
+      return ! (euid == NO_UID && errno) && euid == stat_buf.st_uid;

     case 'G':			/* File is owned by your group? */
       unary_advance ();
-      return (stat (argv[pos - 1], &stat_buf) == 0
-              && (getegid () == stat_buf.st_gid));
+      if (stat (argv[pos - 1], &stat_buf) != 0)
+        return false;
+      errno = 0;
+      gid_t egid = getegid ();
+      gid_t NO_GID = -1;
+      return ! (egid == NO_GID && errno) && egid == stat_buf.st_gid;

     case 'f':			/* File is a file? */
       unary_advance ();
diff --git a/src/whoami.c b/src/whoami.c
index e563dcf..1b0c3cd 100644
--- a/src/whoami.c
+++ b/src/whoami.c
@@ -61,6 +61,7 @@ main (int argc, char **argv)
 {
   struct passwd *pw;
   uid_t uid;
+  uid_t NO_UID = -1;

   initialize_main (&argc, &argv);
   set_program_name (argv[0]);
@@ -81,8 +82,9 @@ main (int argc, char **argv)
       usage (EXIT_FAILURE);
     }

+  errno = 0;
   uid = geteuid ();
-  pw = getpwuid (uid);
+  pw = (uid == NO_UID && errno ? NULL : getpwuid (uid));
   if (pw)
     {
       puts (pw->pw_name);

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

From: Eric Blake <eblake <at> redhat.com>
Cc: Ludovic Courtès <ludo <at> gnu.org>,
	Paul Eggert <eggert <at> cs.ucla.edu>, bug-hurd <at> gnu.org,
	Jim Meyering <jim <at> meyering.net>, 10021 <at> debbugs.gnu.org
Subject: Re: bug#10021: [PATCH id] Add error-checking on GNU
Date: Wed, 16 Nov 2011 12:33:42 -0700

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

On 11/15/2011 10:09 AM, Eric Blake wrote:
> Still debatable.  POSIX explicitly states that the condition of errno
> after a successful call to a standardized function is unspecified; that
> is, a successful geteuid() may pollute errno, but it's okay, because the
> user shouldn't be inspecting errno after geteuid().
> 
> It might be worth proposing a change to POSIX to require that geteuid()
> and friends leave errno unchanged on success (in order to allow for the
> GNU extension of setting errno on failure, even though POSIX did not
> reserve a specific value for failure); I'll pursue that course.

Done: http://austingroupbugs.net/view.php?id=511

-- 
Eric Blake   eblake <at> redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

[signature.asc (application/pgp-signature, attachment)]

Message #85 received at 10021-done <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 10021-done <at> debbugs.gnu.org
Subject: Re: bug#10021: [PATCH id] Add error-checking on GNU
Date: Sat, 19 Nov 2011 19:23:16 +0100

Paul Eggert wrote:
> I found yet-another tricky bug in that id.c code,
> and fixed it as follows:
>
> Subject: [PATCH] id: fix bug when euid != ruid
>
> * src/id.c (main): Report an error if no args are given and getuid
> fails, because print_full_info needs ruid.  Redo code so that
> getuid and friends are invoked only when needed; this makes the
> code easier to follow, and is how I found the above bug.

Thanks.
I've marked this as done.

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

From: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: bug-hurd <at> gnu.org, 10021 <at> debbugs.gnu.org
Subject: Re: bug#10021: [PATCH id] Add error-checking on GNU
Date: Sun, 20 Nov 2011 14:13:25 -0800

No further comment to
<http://debbugs.gnu.org/cgi/bugreport.cgi?bug=10021#77>
so I pushed that patch to groups, install, su, test, whoami.

This should fix all the problems that coreutils has with
getuid etc. returning -1.  (Now all we have to do is fix
all the other packages that assume getuid etc. never fail.
:-)

Previous Next

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