chown, cpio: proposed change for userspec handling of USER:

[Jim Meyering]

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

* lib/userspec.c: Adjust semantics not to treat USER: specially.
GNU chown has had a "feature" whereby specifying "USER:" would make
chown change owner to the non-numeric "USER", and at the same time
change the group to the login group of USER.  By a similar token,
since it's not generally 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.
---
 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>
+
+       userspec: do not treat USER: specially
+       * lib/userspec.c: Adjust semantics not to treat USER: specially.
+       GNU chown has had a "feature" whereby specifying "USER:" would make
+       chown change owner to the non-numeric "USER", and at the same time
+       change the group to the login group of USER.  By a similar token,
+       since it's not generally 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.
+
 2009-12-01  Jim Meyering  <address@hidden>

        fts: fts_open: do not let an empty string cause immediate failure
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");
-  static const char *E_bad_spec = N_("invalid spec");

   const char *error_msg;
   struct passwd *pwd;
@@ -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

* tests/test-userspec.c: New file.
* modules/userspec-tests: Likewise.
---
 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>

+       userspec: add unit tests
+       * tests/test-userspec.c: New file.
+       * modules/userspec-tests: Likewise.
+
        userspec: depend on the inttostr module, too
        * modules/userspec (Depends-on): Add inttostr.

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
+
+  chown no longer rejects a spec of the form "NUMERIC_USER_ID:",
+  and no longer treats "U:" as "U:G" where U is a non-numeric user name
+  and G is that user's login group.  Now, "U:" is treated the same as "U".
+

 * Noteworthy changes in release 8.1 (2009-11-18) [stable]

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 $id_u "$id_un" ''; do
       *)   seps=': .' ;;
     esac
     for sep in $seps; do
-      case $u$sep$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