chown, cpio: proposed change for userspec handling of USER: (original) (raw)

[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

From: Jim Meyering
Subject: chown, cpio: proposed change for userspec handling of USER:
Date: Wed, 02 Dec 2009 20:40:37 +0100

Hello,

While writing a few tests for userspec (below), I was surprised to re-learn that chown USER_NAME: has a special meaning. It is a shorthand for chown USER_NAME:+$(id -g USER_NAME) ... I had expected it to be equivalent to this: chown USER_NAME ...

Since the above behavior is not specified by POSIX, and is IMHO, counter-intuitive, I propose to change it. However, it is documented both in coreutils and in cpio's manuals.

Here's the patch, followed by the coreutils part, but I suppose I can't really apply them as-is. Rather, I may make "chown USER_NAME: ..." warn-for-now yet continue to work and "chown NUMERIC: ..." work like "chown NUMERIC ...". Then, in say two years, I'd make the actual change.

Sergio, would you accept a similar preparatory patch for cpio?

Opinions?

BTW, just realized that changing userspec semantics like this would require a NEWS update, too... But we're not there yet.

Also, regardless, I now expect to adjust this new test module to test the current behavior.

From b86a5bd9960c33b753f7963ceb1502a34255f3f4 Mon Sep 17 00:00:00 2001 From: Jim Meyering <address@hidden> Date: Wed, 2 Dec 2009 20:01:36 +0100 Subject: [PATCH 1/2] userspec: do not treat USER: specially

ChangeLog | 11 +++++++++++ lib/userspec.c | 21 +++++---------------- 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/ChangeLog b/ChangeLog index 6eec830..c4098c0 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,14 @@ +2009-12-02 Jim Meyering <address@hidden> +

diff --git a/lib/userspec.c b/lib/userspec.c index 0388cb1..e12b567 100644 --- a/lib/userspec.c +++ b/lib/userspec.c @@ -105,7 +105,6 @@ parse_with_separator (char const *spec, char const *separator, { static const char *E_invalid_user = N_("invalid user"); static const char *E_invalid_group = N_("invalid group");

@@ -158,22 +157,12 @@ parse_with_separator (char const *spec, char const *separator, pwd = (u == '+' ? NULL : getpwnam (u)); if (pwd == NULL) { - bool use_login_group = (separator != NULL && g == NULL); - if (use_login_group) - { - / If there is no group, - then there may not be a trailing ":", either. */ - error_msg = E_bad_spec; - } + unsigned long int tmp; + if (xstrtoul (u, NULL, 10, &tmp, "") == LONGINT_OK + && tmp <= MAXUID && (uid_t) tmp != (uid_t) -1) + unum = tmp; else - { - unsigned long int tmp; - if (xstrtoul (u, NULL, 10, &tmp, "") == LONGINT_OK - && tmp <= MAXUID && (uid_t) tmp != (uid_t) -1) - unum = tmp; - else - error_msg = E_invalid_user; - } + error_msg = E_invalid_user; } else {

1.6.6.rc0.324.gb5bf2

From 33cf9804ac5593a267d9c76482d5a5c85b01db84 Mon Sep 17 00:00:00 2001 From: Jim Meyering <address@hidden> Date: Sat, 28 Nov 2009 11:51:08 +0100 Subject: [PATCH 2/2] userspec-tests: test the userspec module

ChangeLog | 4 ++ modules/userspec-tests | 10 ++++ tests/test-userspec.c | 139 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 153 insertions(+), 0 deletions(-) create mode 100644 modules/userspec-tests create mode 100644 tests/test-userspec.c

diff --git a/ChangeLog b/ChangeLog index c4098c0..20720d0 100644 --- a/ChangeLog +++ b/ChangeLog @@ -34,6 +34,10 @@

2009-11-28 Jim Meyering <address@hidden>

diff --git a/modules/userspec-tests b/modules/userspec-tests new file mode 100644 index 0000000..97bcbdb --- /dev/null +++ b/modules/userspec-tests @@ -0,0 +1,10 @@ +Files: +tests/test-userspec.c + +Depends-on: + +configure.ac: + +Makefile.am: +TESTS += test-userspec +check_PROGRAMS += test-userspec diff --git a/tests/test-userspec.c b/tests/test-userspec.c new file mode 100644 index 0000000..7cc9909 --- /dev/null +++ b/tests/test-userspec.c @@ -0,0 +1,139 @@ +/* Test userspec.c + Copyright (C) 2009 Free Software Foundation, Inc. + + This program is free software: you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. / + +/ Written by Jim Meyering. */ + +#include <config.h> + +#include "userspec.h" + +#include <stdio.h> +#include <string.h> +#include <stdlib.h> +#include <stdbool.h> +#include <sys/types.h> + +struct test +{ + char const *in; + uid_t uid; + gid_t gid; + char *user_name; + char *group_name; + char const *result; +}; + +static struct test T[] = + { + { "", -1, -1, "", "", NULL}, + { ":", -1, -1, "", "", NULL}, + { "0:0", 0, 0, "", "", NULL}, + { ":1", -1, 1, "", "", NULL}, + { "1:", 1, -1, "", "", NULL}, + { "1", 1, -1, "", "", NULL}, + { "+0:", 0, -1, "", "", NULL}, + { ":+0", -1, 0, "", "", NULL}, + { "22:42", 22, 42, "", "", NULL}, + { "4294967295:4294967295", 0, 0, NULL, NULL, "invalid user"}, + { "0:4294967295", 0, 0, NULL, NULL, "invalid group"}, + { ":4294967295", 0, 0, NULL, NULL, "invalid group"}, + { "4294967295:0", 0, 0, NULL, NULL, "invalid user"}, + { "4294967295:", 0, 0, NULL, NULL, "invalid user"}, + { "4294967296:4294967296", 0, 0, NULL, NULL, "invalid user"}, + { "0:4294967296", 0, 0, NULL, NULL, "invalid group"}, + { ":4294967296", 0, 0, NULL, NULL, "invalid group"}, + { "4294967296:0", 0, 0, NULL, NULL, "invalid user"}, + { "4294967296:", 0, 0, NULL, NULL, "invalid user"}, + + { NULL, 0, 0, NULL, NULL, ""} + }; + +#define STREQ(a, b) (strcmp (a, b) == 0) + +static char const * +maybe_null (char const *s) +{ + return s ? s : "NULL"; +} + +static bool +same_diag (char const *s, char const *t) +{ + if (s == NULL && t == NULL) + return true; + if (s == NULL || t == NULL) + return false; + return STREQ (s, t); +} + +int +main (void) +{ + unsigned int i; + int fail = 0; + + for (i = 0; T[i].in; 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); + if (!same_diag (diag, T[i].result)) + { + printf ("%s return value mismatch: got %s, expected %s\n", + T[i].in, maybe_null (diag), maybe_null (T[i].result)); + fail = 1; + continue; + } + + if (diag) + continue; + + if (uid != T[i].uid || gid != T[i].gid) + { + printf ("%s mismatch (-: expected uid,gid; +:actual)\n" + "-%3lu,%3lu\n+%3lu,%3lu\n", + T[i].in, + (unsigned long int) T[i].uid, + (unsigned long int) T[i].gid, + (unsigned long int) uid, + (unsigned long int) gid); + fail = 1; + } + + if (!diag && !T[i].result) + continue; + + { + printf ("%s diagnostic mismatch (-: expected uid,gid; +:actual)\n" + "-%s\n+%s\n", + T[i].in, T[i].result, diag); + fail = 1; + } + } + + return fail; +} + +/ +Local Variables: +indent-tabs-mode: nil +End: +/

1.6.6.rc0.324.gb5bf2

From 7be609101fbbaf325f6dc91908fb61f7d7a62590 Mon Sep 17 00:00:00 2001 From: Jim Meyering <address@hidden> Date: Wed, 2 Dec 2009 19:33:40 +0100 Subject: [PATCH] chown: do not treat a "USER:" spec specially

GNU chown has had a "feature" whereby specifying "USER:" would make it change owner to the non-numeric "USER", and at the same time change the group to the login group of USER. As a consequence, since it's not always possible to map a numeric USER to a user name, chown would reject a numeric "USER:". Now, "USER:" is treated the same as "USER", for both numeric and non-numeric USER. * src/chown.c (usage): Adjust description. * tests/chown/separator: Do not require "chown 0: ." to fail. * doc/coreutils.texi (chown invocation): Remove a paragraph. * NEWS (Changes in behavior): Mention it.

NEWS | 6 ++++++ doc/coreutils.texi | 5 ----- src/chown.c | 3 +-- tests/chown/separator | 5 +---- 4 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/NEWS b/NEWS index 9f7bf2e..36e95ee 100644 --- a/NEWS +++ b/NEWS @@ -10,6 +10,12 @@ GNU coreutils NEWS -- outline -- the presence of the empty string argument. [bug introduced in coreutils-8.0]

+** Changes in behavior +

diff --git a/doc/coreutils.texi b/doc/coreutils.texi index 3721bee..50c85dc 100644 --- a/doc/coreutils.texi +++ b/doc/coreutils.texi @@ -9453,11 +9453,6 @@ chown invocation group name or numeric group ID), with no spaces between them, the group ownership of the files is changed as well (to @var{group}).

address@hidden address@hidden:} -If a colon but no group name follows @var{owner}, that user is -made the owner of the files and the group of the files is changed to address@hidden's login group.

@item @samp{:}group If the colon and following @var{group} are given, but the owner is omitted, only the group of the files is changed; in this case, diff --git a/src/chown.c b/src/chown.c index 8104afb..2d5f3d5 100644 --- a/src/chown.c +++ b/src/chown.c @@ -130,8 +130,7 @@ one takes effect.\n
fputs (VERSION_OPTION_DESCRIPTION, stdout); fputs (("
\n
-Owner is unchanged if missing. Group is unchanged if missing, but changed\n
-to login group if implied by a `:' following a symbolic OWNER.\n
+Owner is unchanged if missing. Group is unchanged if missing.\n
OWNER and GROUP may be numeric as well as symbolic.\n
"), stdout); printf (("
diff --git a/tests/chown/separator b/tests/chown/separator index 6e717f6..6c3ed17 100755 --- a/tests/chown/separator +++ b/tests/chown/separator @@ -56,10 +56,7 @@ for u in idu"id_u "idu​"id_un" ''; do ) seps=': .' ;; esac for sep in $seps; do - case uuusep$g in - [0-9]$sep) chown "$u$sep$g" . 2> /dev/null && fail=1 ;; - *) chown "$u$sep$g" . || fail=1 ;; - esac + chown "$u$sep$g" . || fail=1 done done done

1.6.6.rc0.324.gb5bf2