Re: bug#10021: [PATCH id] Add error-checking on GNU

[Jim Meyering]

Ludovic Courtès wrote:
> Thanks for the quick review!
>
> Jim Meyering <jim@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@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