Hello tech@,
I looked at the doas code and really like the concept and simplicity. I
did found the strtogid code a little odd for two reasons:
1) It reads like an mangled variety on the parseuid function.
2) It returns -1 on error, but gid_t is defined as a __uint32_t as per
/usr/include/sys/{,_}types.h.
Option 1 makes it strange to read and option 2 could potentially cause
an error when someone is stupid enough to assign GID_MAX as gid.
To make it more consistent I reformatted strtogid to parsegid to
resemble parseuid and solve the potential incorrect error.
Sincerely,
Martijn van Duren
Index: doas.c
===================================================================
RCS file: /cvs/src/usr.bin/doas/doas.c,v
retrieving revision 1.27
diff -u -p -r1.27 doas.c
--- doas.c 26 Jul 2015 22:44:33 -0000 1.27
+++ doas.c 27 Jul 2015 14:38:12 -0000
@@ -80,19 +80,20 @@ uidcheck(const char *s, uid_t desired)
return 0;
}
-static gid_t
-strtogid(const char *s)
+static int
+parsegid(const char *s, gid_t *gid)
{
struct group *gr;
const char *errstr;
- gid_t gid;
- if ((gr = getgrnam(s)) != NULL)
- return gr->gr_gid;
- gid = strtonum(s, 0, GID_MAX, &errstr);
+ if ((gr = getgrnam(s)) != NULL) {
+ *gid = gr->gr_gid;
+ return 0;
+ }
+ *gid = strtonum(s, 0, GID_MAX, &errstr);
if (errstr)
return -1;
- return gid;
+ return 0;
}
static int
@@ -102,8 +103,8 @@ match(uid_t uid, gid_t *groups, int ngro
int i;
if (r->ident[0] == ':') {
- gid_t rgid = strtogid(r->ident + 1);
- if (rgid == -1)
+ gid_t rgid;
+ if(parsegid(r->ident + 1, &rgid) == -1)
return 0;
for (i = 0; i < ngroups; i++) {
if (rgid == groups[i])
