From 23cca8268d21f5d58ed0209002d5673d0518c426 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Thu, 24 Feb 2022 17:10:12 -0800 Subject: [PATCH] userspec: warn about '.' separator MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Problem reported by Dan Jacobson (Bug#44770). * lib/userspec.c: Don’t include stdbool.h since it’s now in our API. (parse_user_spec_warn): New function, broken out of parse_user_spec and with a new PWARN arg. (parse_user_spec): Use it. * lib/userspec.h: Include stdbool.h and declare new function. * tests/test-userspec.c (struct test.in): Now a char array so that it can be modified. (T): Make the placeholder a valid test, as that simplifies the code. Omit NULL placeholder at the end, likewise. (main): Set up T in the new way, and test that the "." separator acts like the ":" separator except with a warning if it works. --- ChangeLog | 14 ++++++++ lib/userspec.c | 26 ++++++++++++--- lib/userspec.h | 8 +++-- tests/test-userspec.c | 77 ++++++++++++++++++++++++++++--------------- 4 files changed, 91 insertions(+), 34 deletions(-) diff --git a/ChangeLog b/ChangeLog index dda27c7eb9..3499d066e4 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,19 @@ 2022-02-24 Paul Eggert + userspec: warn about '.' separator + Problem reported by Dan Jacobson (Bug#44770). + * lib/userspec.c: Don’t include stdbool.h since it’s now in our API. + (parse_user_spec_warn): New function, broken out of parse_user_spec + and with a new PWARN arg. + (parse_user_spec): Use it. + * lib/userspec.h: Include stdbool.h and declare new function. + * tests/test-userspec.c (struct test.in): Now a char array + so that it can be modified. + (T): Make the placeholder a valid test, as that simplifies + the code. Omit NULL placeholder at the end, likewise. + (main): Set up T in the new way, and test that the "." separator + acts like the ":" separator except with a warning if it works. + userspec: no need for static vars * lib/userspec.c (parse_with_separator): Simplify. diff --git a/lib/userspec.c b/lib/userspec.c index e75feb255d..0635bf8426 100644 --- a/lib/userspec.c +++ b/lib/userspec.c @@ -22,7 +22,6 @@ /* Specification. */ #include "userspec.h" -#include #include #include #include @@ -251,15 +250,18 @@ parse_with_separator (char const *spec, char const *separator, Either one might be NULL instead, indicating that it was not given and the corresponding numeric ID was left unchanged. - Return NULL if successful, a static error message string if not. */ + Return NULL if successful, a static error message string if not. + If PWARN is null, return NULL instead of a warning; + otherwise, set *PWARN to true depending on whether returning a warning. */ char const * -parse_user_spec (char const *spec, uid_t *uid, gid_t *gid, - char **username, char **groupname) +parse_user_spec_warn (char const *spec, uid_t *uid, gid_t *gid, + char **username, char **groupname, bool *pwarn) { char const *colon = gid ? strchr (spec, ':') : NULL; char const *error_msg = parse_with_separator (spec, colon, uid, gid, username, groupname); + bool warn = false; if (gid && !colon && error_msg) { @@ -272,12 +274,26 @@ parse_user_spec (char const *spec, uid_t *uid, gid_t *gid, char const *dot = strchr (spec, '.'); if (dot && ! parse_with_separator (spec, dot, uid, gid, username, groupname)) - error_msg = NULL; + { + warn = true; + error_msg = pwarn ? N_("warning: '.' should be ':'") : NULL; + } } + if (pwarn) + *pwarn = warn; return error_msg; } +/* Like parse_user_spec_warn, but generate only errors; no warnings. */ + +char const * +parse_user_spec (char const *spec, uid_t *uid, gid_t *gid, + char **username, char **groupname) +{ + return parse_user_spec_warn (spec, uid, gid, username, groupname, NULL); +} + #ifdef TEST # define NULL_CHECK(s) ((s) == NULL ? "(null)" : (s)) diff --git a/lib/userspec.h b/lib/userspec.h index fcfa4b9b74..7d5d063e7e 100644 --- a/lib/userspec.h +++ b/lib/userspec.h @@ -19,10 +19,14 @@ #ifndef USERSPEC_H # define USERSPEC_H 1 +# include # include -const char * -parse_user_spec (const char *spec_arg, uid_t *uid, gid_t *gid, +char const * +parse_user_spec (char const *spec_arg, uid_t *uid, gid_t *gid, char **username_arg, char **groupname_arg); +char const * +parse_user_spec_warn (char const *spec_arg, uid_t *uid, gid_t *gid, + char **username_arg, char **groupname_arg, bool *pwarn); #endif diff --git a/tests/test-userspec.c b/tests/test-userspec.c index c2e36ffb50..65287a592d 100644 --- a/tests/test-userspec.c +++ b/tests/test-userspec.c @@ -35,7 +35,7 @@ struct test { - const char *in; + char in[100]; uid_t uid; gid_t gid; const char *user_name; @@ -48,6 +48,7 @@ static struct test T[] = { "", -1, -1, "", "", NULL}, { ":", -1, -1, "", "", NULL}, { "+0:+0", 0, 0, "", "", NULL}, + { "+0:+0", 0, 0, "", "", NULL}, { ":+1", -1, 1, "", "", NULL}, { "+1", 1, -1, "", "", NULL}, { ":+0", -1, 0, "", "", NULL}, @@ -73,9 +74,7 @@ static struct test T[] = /* "username:" must expand to UID:GID where GID is username's login group */ /* Add an entry like the following to the table, if possible. { "U_NAME:", UID,GID, U_NAME, G_NAME, NULL}, */ - { NULL, 0, 0, NULL, NULL, ""}, /* place-holder */ - - { NULL, 0, 0, NULL, NULL, ""} + { "" /* placeholder */, -1, -1, "", "", NULL}, }; #define STREQ(a, b) (strcmp (a, b) == 0) @@ -114,19 +113,14 @@ main (void) size_t len; if (!pw || !pw->pw_name || !(gr = getgrgid (pw->pw_gid)) || !gr->gr_name) continue; - j = ARRAY_CARDINALITY (T) - 2; - assert (T[j].in == NULL); - assert (T[j+1].in == NULL); + j = ARRAY_CARDINALITY (T) - 1; len = strlen (pw->pw_name); + if (sizeof T[j].in - 2 < len) + continue; /* Store "username:" in T[j].in. */ - { - char *t = xmalloc (len + 1 + 1); - memcpy (t, pw->pw_name, len); - t[len] = ':'; - t[len+1] = '\0'; - T[j].in = t; - } + memcpy(T[j].in, pw->pw_name, len); + strcpy(T[j].in + len, ":"); T[j].uid = uid; T[j].gid = gr->gr_gid; @@ -137,16 +131,17 @@ main (void) } } - for (i = 0; T[i].in; i++) + char *user_name = NULL; + char *group_name = NULL; + + for (i = 0; i < ARRAY_CARDINALITY (T); i++) { uid_t uid = (uid_t) -1; gid_t gid = (gid_t) -1; - char *user_name; - char *group_name; - char const *diag = parse_user_spec (T[i].in, &uid, &gid, - &user_name, &group_name); free (user_name); free (group_name); + char const *diag = parse_user_spec (T[i].in, &uid, &gid, + &user_name, &group_name); if (!same_diag (diag, T[i].result)) { printf ("%s return value mismatch: got %s, expected %s\n", @@ -170,14 +165,42 @@ main (void) fail = 1; } - if (!T[i].result) - continue; - - /* Expected a non-NULL result diagnostic, yet got NULL. */ - diag = "NULL"; - printf ("%s diagnostic mismatch (-: expected diagnostic; +:actual)\n" - "-%s\n+%s\n", T[i].in, T[i].result, diag); - fail = 1; + if (T[i].result) + { + /* Expected a non-NULL result diagnostic, yet got NULL. */ + diag = "NULL"; + printf ("%s diagnostic mismatch (-: expected diagnostic; +:actual)\n" + "-%s\n+%s\n", T[i].in, T[i].result, diag); + fail = 1; + } + else + { + /* Should get the same result, but with a warning, if replacing + ':' with '.'. */ + char *colon = strchr (T[i].in, ':'); + if (colon) + { + *colon = '.'; + uid_t uid2 = -1; + gid_t gid2 = -1; + char *user_name2 = NULL; + char *group_name2 = NULL; + bool warn; + if (! (parse_user_spec_warn (T[i].in, &uid2, &gid2, + &user_name2, &group_name2, &warn) + && warn)) + printf ("%s did not warn\n", T[i].in); + else if (! (uid == uid2 && gid == gid2 + && !!user_name == !!user_name2 + && (!user_name || STREQ (user_name, user_name2)) + && !!group_name == !!group_name2 + && (!group_name || STREQ (group_name, group_name2)))) + printf ("%s treated differently than with colon\n", T[i].in); + + free (user_name2); + free (group_name2); + } + } } /* Ensure NULL parameters are ignored. */ -- 2.39.5