From 61fa1b68bfe73f5cc8ccdfe4927917c0d9a4b270 Mon Sep 17 00:00:00 2001 From: Johnothan King Date: Tue, 14 Dec 2021 01:22:30 -0800 Subject: [PATCH] The `chown` builtin should fail with the same error consistently (#378) This bug was first reported at . 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: https://github.com/illumos/illumos-gate/commit/4162633a7c5961f388fd src/lib/libcmd/chgrp.c: - Remove NOID macro and check for a < 0 error status instead. This is different from the Illumos fix at 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 --- NEWS | 5 +++++ src/lib/libast/man/strgid.3 | 13 +++++++++---- src/lib/libast/man/struid.3 | 13 +++++++++---- src/lib/libcmd/chgrp.c | 32 +++++++++++++++----------------- 4 files changed, 38 insertions(+), 25 deletions(-) diff --git a/NEWS b/NEWS index a51945b49..70fbf10eb 100644 --- a/NEWS +++ b/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 diff --git a/src/lib/libast/man/strgid.3 b/src/lib/libast/man/strgid.3 index d7a26634c..657db7762 100644 --- a/src/lib/libast/man/strgid.3 +++ b/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 -.IR gid . +returns the group ID number for the given group name. .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) diff --git a/src/lib/libast/man/struid.3 b/src/lib/libast/man/struid.3 index 522deb5b8..9f42abd95 100644 --- a/src/lib/libast/man/struid.3 +++ b/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 -.IR uid . +returns the user ID number for the given user name. .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) diff --git a/src/lib/libcmd/chgrp.c b/src/lib/libcmd/chgrp.c index 259be9939..0c3b7234b 100644 --- a/src/lib/libcmd/chgrp.c +++ b/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)) {