The chown builtin should fail with the same error consistently (#378)

This bug was first reported at <https://www.illumos.org/issues/3782>.
The chown builtin when used on illumos can fail with different error
messages after running the same command twice:

  $ touch /tmp/x
  $ /opt/ast/bin/chown -h 433:434 /tmp/px
  chown: /tmp/x: cannot change owner and group [Not owner]
  $ /opt/ast/bin/chown -h 433:434 /tmp/px
  chown: /tmp/x: cannot change owner and group [Invalid argument]

The error messages differ because the libast struid and strgid
functions will return -2 if the same nonexistent ID is used twice.

The fix for this bug has been ported from here:
4162633a7c

src/lib/libcmd/chgrp.c:
- Remove NOID macro and check for a < 0 error status instead.
  This is different from the Illumos fix at
  <4162633a7c>
  which added another macro.

src/lib/libast/man/{strgid,struid}.3:
- Correct errors in the strgid and struid documentation.
- Document that the strgid and struid functions will return -2 if
  the same invalid name is used twice.

Co-authored-by: Martijn Dekker <martijn@inlv.org>

NEWS

@@ -8,6 +8,11 @@ Any uppercase BUG_* names are modernish shell bug IDs.
 - Fixed a bug introduced on 2020-08-09 that prevented '.' and '..' from
   being completed when using file name tab completion.
 
+- Fixed a bug on illumos that caused the chown builtin to fail with 'Invalid
+  argument' after failing to change the ownership of a file twice using an ID
+  that doesn't exist in /etc/passwd. (Note that the chown builtin is not
+  enabled by default.)
+
 2021-12-11:
 
 - Fixed two more crashing bugs that occurred if ksh received a signal (such

src/lib/libast/man/strgid.3

@@ -38,16 +38,21 @@
 ..
 .TH STRGID 3
 .SH NAME
-strgid \- return group name given group number
+strgid \- return group ID number for given group name
 .SH SYNOPSIS
-.L "char* strgid(int gid)"
+.L "int strgid(const char* name)"
 .SH DESCRIPTION
 .I strgid
-returns the group id name string given the group number
+returns the group ID number for the given group name.
-.IR gid .
 .I strgid
 maintains an internal cache to avoid repeated password database scans
 by the low level
 .IR getgrgid (3).
+On the first error for a given name,
+.I strgid
+returns -1.
+Subsequent errors for the same name will result in
+.I strgid
+returning -2 instead.
 .SH "SEE ALSO"
 getgrent(3)

src/lib/libast/man/struid.3

@@ -38,16 +38,21 @@
 ..
 .TH STRUID 3
 .SH NAME
-struid \- return user name given user number
+struid \- return user ID number for given user name
 .SH SYNOPSIS
-.L "char* struid(int uid)"
+.L "int struid(const char* name)"
 .SH DESCRIPTION
 .I struid
-returns the user id name string given the user number
+returns the user ID number for the given user name.
-.IR uid .
 .I struid
 maintains an internal cache to avoid repeated password database scans
 by the low level
 .IR getpwuid (3).
+On the first error for a given name,
+.I struid
+returns -1.
+Subsequent errors for the same name will result in
+.I struid
+returning -2 instead.
 .SH "SEE ALSO"
 getpwent(3)

src/lib/libcmd/chgrp.c

@@ -131,8 +131,6 @@ typedef struct Map_s			/* uid/gid map			*/
 	Key_t		to;		/* map to these			*/
 } Map_t;
 
-#define NOID		(-1)
-
 #define OPT_CHOWN	0x0001		/* chown			*/
 #define OPT_FORCE	0x0002		/* ignore errors		*/
 #define OPT_GID		0x0004		/* have gid			*/
@@ -159,7 +157,7 @@ getids(register char* s, char** e, Key_t* key, int options)
 	char*		z;
 	char		buf[64];
 
-	key->uid = key->gid = NOID;
+	key->uid = key->gid = -1;
 	while (isspace(*s))
 		s++;
 	for (t = s; (n = *t) && n != ':' && n != '.' && !isspace(n); t++);
@@ -177,7 +175,7 @@ getids(register char* s, char** e, Key_t* key, int options)
 			n = (int)strtol(s, &z, 0);
 			if (*z || !(options & OPT_NUMERIC))
 			{
-				if ((m = struid(s)) != NOID)
+				if ((m = struid(s)) >= 0)
 					n = m;
 				else if (*z)
 				{
@@ -200,7 +198,7 @@ getids(register char* s, char** e, Key_t* key, int options)
 		n = (int)strtol(s, &z, 0);
 		if (*z || !(options & OPT_NUMERIC))
 		{
-			if ((m = strgid(s)) != NOID)
+			if ((m = strgid(s)) >= 0)
 				n = m;
 			else if (*z)
 			{
@@ -387,21 +385,21 @@ b_chgrp(int argc, char** argv, Shbltin_t* context)
 					UNREACHABLE();
 				}
 				m->key = key;
-				m->to.uid = m->to.gid = NOID;
+				m->to.uid = m->to.gid = -1;
 				dtinsert(map, m);
 			}
 			getids(t, NiL, &m->to, options);
 		}
 		if (sp != sfstdin)
 			sfclose(sp);
-		keys[1].gid = keys[2].uid = NOID;
+		keys[1].gid = keys[2].uid = -1;
 	}
 	else if (!(options & (OPT_UID|OPT_GID)))
 	{
 		getids(s, NiL, &key, options);
-		if ((uid = key.uid) != NOID)
+		if ((uid = key.uid) >= 0)
 			options |= OPT_UID;
-		if ((gid = key.gid) != NOID)
+		if ((gid = key.gid) >= 0)
 			options |= OPT_GID;
 	}
 	switch (options & (OPT_UID|OPT_GID))
@@ -455,7 +453,7 @@ b_chgrp(int argc, char** argv, Shbltin_t* context)
 			if (map)
 			{
 				options &= ~(OPT_UID|OPT_GID);
-				uid = gid = NOID;
+				uid = gid = -1;
 				keys[0].uid = keys[1].uid = ent->fts_statp->st_uid;
 				keys[0].gid = keys[2].gid = ent->fts_statp->st_gid;
 				i = 0;
@@ -463,18 +461,18 @@ b_chgrp(int argc, char** argv, Shbltin_t* context)
 				{
 					if (m = (Map_t*)dtmatch(map, &keys[i]))
 					{
-						if (uid == NOID && m->to.uid != NOID)
+						if (uid < 0 && m->to.uid >= 0)
 						{
 							uid = m->to.uid;
 							options |= OPT_UID;
 						}
-						if (gid == NOID && m->to.gid != NOID)
+						if (gid < 0 && m->to.gid >= 0)
 						{
 							gid = m->to.gid;
 							options |= OPT_GID;
 						}
 					}
-				} while (++i < elementsof(keys) && (uid == NOID || gid == NOID));
+				} while (++i < elementsof(keys) && (uid < 0 || gid < 0));
 			}
 			else
 			{
@@ -483,16 +481,16 @@ b_chgrp(int argc, char** argv, Shbltin_t* context)
 				if (!(options & OPT_GID))
 					gid = ent->fts_statp->st_gid;
 			}
-			if ((options & OPT_UNMAPPED) && (uid == NOID || gid == NOID))
+			if ((options & OPT_UNMAPPED) && (uid < 0 || gid < 0))
 			{
-				if (uid == NOID && gid == NOID)
+				if (uid < 0 && gid < 0)
 					error(ERROR_warn(0), "%s: uid and gid not mapped", ent->fts_path);
-				else if (uid == NOID)
+				else if (uid < 0)
 					error(ERROR_warn(0), "%s: uid not mapped", ent->fts_path);
 				else
 					error(ERROR_warn(0), "%s: gid not mapped", ent->fts_path);
 			}
-			if (uid != ent->fts_statp->st_uid && uid != NOID || gid != ent->fts_statp->st_gid && gid != NOID)
+			if (uid != ent->fts_statp->st_uid && uid >= 0 || gid != ent->fts_statp->st_gid && gid >= 0)
 			{
 				if (options & (OPT_SHOW|OPT_VERBOSE))
 				{