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

[Paul Eggert]

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)
       {