"Dmitry V. Levin" <[EMAIL PROTECTED]> writes: > $ chown "`id -u`.`id -g`" . > chown: `567.567': invalid user > $ chown "`id -un`.`id -gn`" . > chown: `me.me': invalid user > > Proposed fix along with new testcase is attached.
Thanks for finding that. While looking at your patch, I noticed a similar problem with invalid numeric groups. The code is pretty crufty with that weird goto (and I think I wrote some of it -- sorry!) and this cruftiness contributed to the bug. Also, this reminded me of a BSD-compatibility glitch in this area that I volunteered to fix in May <http://lists.gnu.org/archive/html/bug-coreutils/2004-05/msg00174.html>. I installed the following patch to try to fix all these problems at once. It adds your test cases and a bunch of others (though probably not enough :-). 2004-08-19 Paul Eggert <[EMAIL PROTECTED]> * NEWS: "chown : file", "chown '' file", and "chgrp '' file" now succeed without changing the uid and gid, like FreeBSD. * src/chgrp.c (parse_group): Return gid_t rather than storing it through a pointer. Treat "chgrp '' file" as a no-op change, as FreeBSD does. (main): Set chopt.group_name to NULL if the group is the empty string. * src/chown-core.c (describe_change): Describe changes to -1:-1 without using "to OWNERSHIP" phrase. * src/chown.c (usage): "chown '' file" is now allowed. (main): Do not set user name to the empty string if the group name is null. * tests/chgrp/basic: Test "chgrp '' file". * tests/chown/Makefile.am (TESTS): Add separator. * tests/chown/separator: New file, partly taken from Dmitry V. Levin's suggestion in <http://lists.gnu.org/archive/html/bug-coreutils/2004-08/msg00102.html> * doc/coreutils.texi (chown invocation): Fix synopsis: group must always be preceded by separator. "chown : file" and "chown '' file" don't change the owner or group. Update the explanation of what happens to the set-user-ID or set-group-ID bits, e.g., they sometimes are not cleared if they denote mandatory locking. Change "find"-oriented examples to use chown -h. * lib/userspec.c: Don't use <alloca.h>, so that we don't use alloca on strings on unbounded length. alloca's performance benefits aren't that important here. (V_STRDUP): Remove. (parse_with_separator): New function, with most of the internals of the old parse_user_spec. Allow user to omit both user and group, for compatibility with FreeBSD. Clone only the user name, not the entire spec. Do not set *uid, *gid unless entirely successful. Avoid memory leak in some failing cases. Fix regression for USER.GROUP reported by Dmitry V. Levin in <http://lists.gnu.org/archive/html/bug-coreutils/2004-08/msg00102.html> (parse_user_spec): Rewrite to use parse_with_separator. Index: NEWS =================================================================== RCS file: /home/eggert/coreutils/cu/NEWS,v retrieving revision 1.228 diff -p -u -r1.228 NEWS --- NEWS 10 Aug 2004 22:05:47 -0000 1.228 +++ NEWS 19 Aug 2004 02:23:34 -0000 @@ -32,6 +32,9 @@ GNU coreutils NEWS incorrect, as it failed to update the last-changed time and reset special permission bits, as POSIX requires. + "chown : file", "chown '' file", and "chgrp '' file" now succeed + without changing the uid or gid, instead of reporting an error. + Do not report an error if the owner or group of a recursively-encountered symbolic link cannot be updated because the file system does not support it. Index: doc/coreutils.texi =================================================================== RCS file: /home/eggert/coreutils/cu/doc/coreutils.texi,v retrieving revision 1.200 diff -p -u -r1.200 coreutils.texi --- doc/coreutils.texi 18 Aug 2004 20:22:32 -0000 1.200 +++ doc/coreutils.texi 19 Aug 2004 03:05:37 -0000 @@ -7948,7 +7948,7 @@ If used, @var{new-owner} specifies the n (with no embedded white space): @example [EMAIL PROTECTED] [ [:] [EMAIL PROTECTED] ] [EMAIL PROTECTED] [ : [EMAIL PROTECTED] ] @end example Specifically: @@ -7959,21 +7959,25 @@ If only an @var{owner} (a user name or n user is made the owner of each given file, and the files' group is not changed. [EMAIL PROTECTED] [EMAIL PROTECTED]:}group [EMAIL PROTECTED] [EMAIL PROTECTED]:}group If the @var{owner} is followed by a colon and a @var{group} (a group name or numeric group id), with no spaces between them, the group ownership of the files is changed as well (to @var{group}). [EMAIL PROTECTED] [EMAIL PROTECTED]:} [EMAIL PROTECTED] [EMAIL PROTECTED]:} 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 @var{owner}'s login group. [EMAIL PROTECTED] @samp{:}group [EMAIL PROTECTED] @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, @command{chown} performs the same function as @command{chgrp}. [EMAIL PROTECTED] @samp{:} +If only a colon is given, or if @var{new-owner} is empty, neither the +owner nor the group is changed. + @end table Some older scripts may still use @samp{.} in place of the @samp{:} separator. @@ -7985,16 +7989,14 @@ portable, and because it has undesirable @[EMAIL PROTECTED] happens to identify a user whose name contains @samp{.}. -Warning: The @command{chown} command may clear the set-user-ID or -set-group-ID bits on some systems. The @command{chown} command is -dependent upon the policy and functionality of the underlying system -which may make system-dependent file mode modifications outside the -control of the @command{chown} command. On some systems (e.g., Linux) -the @command{chown} command clears the set-UID and set-GID bits because -the underlying, system @code{chown} function clears them. On other -systems (e.g., HP-UX and Solaris) the @command{chown} command does not -affect those bits when operated as the superuser. On systems which allow -non-privileged use of chown those bits are always cleared by the system. +The @command{chown} command sometimes clears the set-user-ID or +set-group-ID permission bits. This behavior depends on the policy and +functionality of the underlying @code{chown} system call, which may +make system-dependent file mode modifications outside the control of +the @command{chown} command. For example, the @command{chown} command +might not affect those bits when operated as the superuser, or if the +bits signify some function other than executable permission (e.g., +mandatory locking). When in doubt, check the underlying system behavior. The program accepts the following options. Also see @ref{Common options}. @@ -8031,7 +8033,7 @@ For example, to reflect a UID numbering without an option like this, @code{root} might run @smallexample -find / -owner OLDUSER -print0 | xargs -0 chown NEWUSER +find / -owner OLDUSER -print0 | xargs -0 chown -h NEWUSER @end smallexample But that is dangerous because the interval between when the @command{find} @@ -8041,7 +8043,7 @@ One way to narrow the gap would be to in as it is found: @example -find / -owner OLDUSER -exec chown NEWUSER @[EMAIL PROTECTED] \; +find / -owner OLDUSER -exec chown -h NEWUSER @[EMAIL PROTECTED] \; @end example But that is very slow if there are many affected files. @@ -8049,7 +8051,7 @@ With this option, it is safer (the gap i though still not perfect: @example -chown -R --from=OLDUSER NEWUSER / +chown -h -R --from=OLDUSER NEWUSER / @end example @item --dereference Index: lib/userspec.c =================================================================== RCS file: /home/eggert/coreutils/cu/lib/userspec.c,v retrieving revision 1.43 diff -p -u -r1.43 userspec.c --- lib/userspec.c 9 Aug 2004 18:44:50 -0000 1.43 +++ lib/userspec.c 19 Aug 2004 14:09:22 -0000 @@ -25,8 +25,6 @@ /* Specification. */ #include "userspec.h" -#include <alloca.h> - #include <stdbool.h> #include <stdio.h> #include <sys/types.h> @@ -92,18 +90,6 @@ struct group *getgrgid (); # define MAXGID GID_T_MAX #endif -/* Perform the equivalent of the statement `dest = strdup (src);', - but obtaining storage via alloca instead of from the heap. */ - -#define V_STRDUP(dest, src) \ - do \ - { \ - size_t size = strlen (src) + 1; \ - (dest) = (char *) alloca (size); \ - memcpy (dest, src, size); \ - } \ - while (0) - /* ISDIGIT differs from isdigit, as follows: - Its arg may be any int or unsigned int; it need not be an unsigned char. - It's guaranteed to evaluate its argument exactly once. @@ -131,78 +117,52 @@ is_number (const char *str) } #endif -/* Extract from NAME, which has the form "[user][:.][group]", - a USERNAME, UID U, GROUPNAME, and GID G. - Either user or group, or both, must be present. - If the group is omitted but the ":" separator is given, - use the given user's login group. - If SPEC_ARG contains a `:', then use that as the separator, ignoring - any `.'s. If there is no `:', but there is a `.', then first look - up the entire SPEC_ARG as a login name. If that look-up fails, then - try again interpreting the `.' as a separator. - - USERNAME and GROUPNAME will be in newly malloc'd memory. - 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. */ - -const char * -parse_user_spec (const char *spec_arg, uid_t *uid, gid_t *gid, - char **username_arg, char **groupname_arg) +static char const * +parse_with_separator (char const *spec, char const *separator, + uid_t *uid, gid_t *gid, + char **username, char **groupname) { 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_("cannot get the login group of a numeric UID"); - static const char *E_cannot_omit_both = - N_("cannot omit both user and group"); const char *error_msg; - char *spec; /* A copy we can write on. */ struct passwd *pwd; struct group *grp; - char *g, *u, *separator; - char *groupname; - char *dot = NULL; + char *u; + char const *g; + char *gname = NULL; + uid_t unum = *uid; + gid_t gnum = *gid; error_msg = NULL; - *username_arg = *groupname_arg = NULL; - groupname = NULL; - - V_STRDUP (spec, spec_arg); + *username = *groupname = NULL; - /* Find the POSIX `:' separator if there is one. */ - separator = strchr (spec, ':'); + /* Set U and G to nonzero length strings corresponding to user and + group specifiers or to NULL. If U is not NULL, it is a newly + allocated string. */ - /* If there is no colon, then see if there's a `.'. */ + u = NULL; if (separator == NULL) { - dot = strchr (spec, '.'); - /* If there's no colon but there is a `.', then first look up the - whole spec, in case it's an OWNER name that includes a dot. - If that fails, then we'll try again, but interpreting the `.' - as a separator. This is a compatible extension to POSIX, since - the POSIX-required behavior is always tried first. */ + if (*spec) + u = xstrdup (spec); + } + else + { + size_t ulen = separator - spec; + if (ulen != 0) + { + u = xclone (spec, ulen + 1); + u[ulen] = '\0'; + } } - - retry: - - /* Replace separator with a NUL. */ - if (separator != NULL) - *separator = '\0'; - - /* Set U and G to non-zero length strings corresponding to user and - group specifiers or to NULL. */ - u = (*spec == '\0' ? NULL : spec); g = (separator == NULL || *(separator + 1) == '\0' ? NULL : separator + 1); - if (u == NULL && g == NULL) - return _(E_cannot_omit_both); - #ifdef __DJGPP__ /* Pretend that we are the user U whose group is G. This makes pwd and grp functions ``know'' about the UID and GID of these. */ @@ -222,32 +182,25 @@ parse_user_spec (const char *spec_arg, u error_msg = E_bad_spec; else { - unsigned long int tmp_long; - if (! (xstrtoul (u, NULL, 10, &tmp_long, "") == LONGINT_OK - && tmp_long <= MAXUID)) - return _(E_invalid_user); - *uid = tmp_long; + unsigned long int tmp; + if (xstrtoul (u, NULL, 10, &tmp, "") == LONGINT_OK + && tmp <= MAXUID) + unum = tmp; + else + error_msg = E_invalid_user; } } else { - *uid = pwd->pw_uid; + unum = pwd->pw_uid; if (g == NULL && separator != NULL) { /* A separator was given, but a group was not specified, so get the login group. */ - *gid = pwd->pw_gid; - grp = getgrgid (pwd->pw_gid); - if (grp == NULL) - { - char buf[INT_BUFSIZE_BOUND (uintmax_t)]; - char const *num = umaxtostr (pwd->pw_gid, buf); - V_STRDUP (groupname, num); - } - else - { - V_STRDUP (groupname, grp->gr_name); - } + char buf[INT_BUFSIZE_BOUND (uintmax_t)]; + gnum = pwd->pw_gid; + grp = getgrgid (gnum); + gname = xstrdup (grp ? grp->gr_name : umaxtostr (gnum, buf)); endgrent (); } } @@ -260,38 +213,72 @@ parse_user_spec (const char *spec_arg, u grp = getgrnam (g); if (grp == NULL) { - unsigned long int tmp_long; - if (! (xstrtoul (g, NULL, 10, &tmp_long, "") == LONGINT_OK - && tmp_long <= MAXGID)) - return _(E_invalid_group); - *gid = tmp_long; + unsigned long int tmp; + if (xstrtoul (g, NULL, 10, &tmp, "") == LONGINT_OK && tmp <= MAXGID) + gnum = tmp; + else + error_msg = E_invalid_group; } else - *gid = grp->gr_gid; + gnum = grp->gr_gid; endgrent (); /* Save a file descriptor. */ - - if (error_msg == NULL) - V_STRDUP (groupname, g); + gname = xstrdup (g); } if (error_msg == NULL) { - if (u != NULL) - *username_arg = xstrdup (u); - - if (groupname != NULL) - *groupname_arg = xstrdup (groupname); + *uid = unum; + *gid = gnum; + *username = u; + *groupname = gname; + u = NULL; } + else + free (gname); - if (error_msg && dot) - { - separator = dot; - dot = NULL; - error_msg = NULL; - goto retry; + free (u); + return _(error_msg); +} + +/* Extract from SPEC, which has the form "[user][:.][group]", + a USERNAME, UID U, GROUPNAME, and GID G. + Either user or group, or both, must be present. + If the group is omitted but the separator is given, + use the given user's login group. + If SPEC contains a `:', then use that as the separator, ignoring + any `.'s. If there is no `:', but there is a `.', then first look + up the entire SPEC as a login name. If that look-up fails, then + try again interpreting the `.' as a separator. + + USERNAME and GROUPNAME will be in newly malloc'd memory. + 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. */ + +char const * +parse_user_spec (char const *spec, uid_t *uid, gid_t *gid, + char **username, char **groupname) +{ + char const *colon = strchr (spec, ':'); + char const *error_msg = + parse_with_separator (spec, colon, uid, gid, username, groupname); + + if (!colon && error_msg) + { + /* If there's no colon but there is a dot, and if looking up the + whole spec failed (i.e., the spec is not a owner name that + includes a dot), then try again, but interpret the dot as a + separator. This is a compatible extension to POSIX, since + the POSIX-required behavior is always tried first. */ + + char const *dot = strchr (spec, '.'); + if (dot + && ! parse_with_separator (spec, dot, uid, gid, username, groupname)) + error_msg = NULL; } - return _(error_msg); + return error_msg; } #ifdef TEST Index: src/chgrp.c =================================================================== RCS file: /home/eggert/coreutils/cu/src/chgrp.c,v retrieving revision 1.113 diff -p -u -r1.113 chgrp.c --- src/chgrp.c 28 Jul 2004 23:36:59 -0000 1.113 +++ src/chgrp.c 19 Aug 2004 16:27:19 -0000 @@ -75,28 +75,30 @@ static struct option const long_options[ {0, 0, 0, 0} }; -/* Set *G according to NAME. */ +/* Return the group ID of NAME, or -1 if no name was specified. */ -static void -parse_group (const char *name, gid_t *g) +static gid_t +parse_group (const char *name) { - struct group *grp; + gid_t gid = -1; - if (*name == '\0') - error (EXIT_FAILURE, 0, _("cannot change to null group")); - - grp = getgrnam (name); - if (grp == NULL) + if (*name) { - unsigned long int tmp_long; - if (! (xstrtoul (name, NULL, 10, &tmp_long, "") == LONGINT_OK - && tmp_long <= GID_T_MAX)) - error (EXIT_FAILURE, 0, _("invalid group %s"), quote (name)); - *g = tmp_long; + struct group *grp = getgrnam (name); + if (grp) + gid = grp->gr_gid; + else + { + unsigned long int tmp; + if (! (xstrtoul (name, NULL, 10, &tmp, "") == LONGINT_OK + && tmp <= GID_T_MAX)) + error (EXIT_FAILURE, 0, _("invalid group %s"), quote (name)); + gid = tmp; + } + endgrent (); /* Save a file descriptor. */ } - else - *g = grp->gr_gid; - endgrent (); /* Save a file descriptor. */ + + return gid; } void @@ -281,8 +283,9 @@ main (int argc, char **argv) } else { - chopt.group_name = argv[optind++]; - parse_group (chopt.group_name, &gid); + char *group_name = argv[optind++]; + chopt.group_name = (*group_name ? group_name : NULL); + gid = parse_group (group_name); } ok = chown_files (argv + optind, bit_flags, Index: src/chown-core.c =================================================================== RCS file: /home/eggert/coreutils/cu/src/chown-core.c,v retrieving revision 1.27 diff -p -u -r1.27 chown-core.c --- src/chown-core.c 28 Jul 2004 23:37:49 -0000 1.27 +++ src/chown-core.c 19 Aug 2004 16:52:01 -0000 @@ -124,19 +124,19 @@ describe_change (const char *file, enum switch (changed) { case CH_SUCCEEDED: - fmt = (user - ? _("changed ownership of %s to %s\n") - : _("changed group of %s to %s\n")); + fmt = (user ? _("changed ownership of %s to %s\n") + : group ? _("changed group of %s to %s\n") + : _("no change to ownership of %s\n")); break; case CH_FAILED: - fmt = (user - ? _("failed to change ownership of %s to %s\n") - : _("failed to change group of %s to %s\n")); + fmt = (user ? _("failed to change ownership of %s to %s\n") + : group ? _("failed to change group of %s to %s\n") + : _("failed to change ownership of %s\n")); break; case CH_NO_CHANGE_REQUESTED: - fmt = (user - ? _("ownership of %s retained as %s\n") - : _("group of %s retained as %s\n")); + fmt = (user ? _("ownership of %s retained as %s\n") + : group ? _("group of %s retained as %s\n") + : _("ownership of %s retained\n")); break; default: abort (); Index: src/chown.c =================================================================== RCS file: /home/eggert/coreutils/cu/src/chown.c,v retrieving revision 1.118 diff -p -u -r1.118 chown.c --- src/chown.c 28 Jul 2004 23:37:21 -0000 1.118 +++ src/chown.c 19 Aug 2004 16:39:58 -0000 @@ -92,11 +92,10 @@ usage (int status) else { printf (_("\ -Usage: %s [OPTION]... OWNER[:[GROUP]] FILE...\n\ - or: %s [OPTION]... :GROUP FILE...\n\ +Usage: %s [OPTION]... [OWNER][:[GROUP]] FILE...\n\ or: %s [OPTION]... --reference=RFILE FILE...\n\ "), - program_name, program_name, program_name); + program_name, program_name); fputs (_("\ Change the owner and/or group of each FILE to OWNER and/or GROUP.\n\ With --reference, change the owner and group of each FILE to those of RFILE.\n\ @@ -146,8 +145,8 @@ one takes effect.\n\ fputs (_("\ \n\ Owner is unchanged if missing. Group is unchanged if missing, but changed\n\ -to login group if implied by a `:'. OWNER and GROUP may be numeric as well\n\ -as symbolic.\n\ +to login group if implied by a `:' following a symbolic OWNER.\n\ +OWNER and GROUP may be numeric as well as symbolic.\n\ "), stdout); printf (_("\nReport bugs to <%s>.\n"), PACKAGE_BUGREPORT); } @@ -313,8 +312,10 @@ main (int argc, char **argv) if (e) error (EXIT_FAILURE, 0, "%s: %s", quote (argv[optind]), e); - /* FIXME: set it to the empty string? */ - if (chopt.user_name == NULL) + /* If a group is specified but no user, set the user name to the + empty string so that diagnostics say "ownership :GROUP" + rather than "group GROUP". */ + if (!chopt.user_name && chopt.group_name) chopt.user_name = ""; optind++; Index: tests/chgrp/basic =================================================================== RCS file: /home/eggert/coreutils/cu/tests/chgrp/basic,v retrieving revision 1.18 diff -p -u -r1.18 basic --- tests/chgrp/basic 23 Jun 2004 15:07:04 -0000 1.18 +++ tests/chgrp/basic 19 Aug 2004 16:52:46 -0000 @@ -43,6 +43,7 @@ test "$VERBOSE" = yes && set +x chgrp -c $g1 f chgrp -c $g2 f chgrp -c $g2 f + chgrp --verbose '' f chgrp --verbose $g1 f chgrp --verbose $g1 f chgrp --verbose --reference=f2 f @@ -80,6 +81,7 @@ test "$VERBOSE" = yes && set +x chgrp $g2 g sleep 1 chgrp $g1 f + chgrp '' f ls -c -t f g ) 2>&1 | sed "s/\([ :]\)$g1$/\1G1/;s/\([ :]\)$g2$/\1G2/" > actual @@ -87,6 +89,7 @@ test "$VERBOSE" = yes && set +x cat <<\EOF > expected changed group of `f' to G1 changed group of `f' to G2 +ownership of `f' retained changed group of `f' to G1 group of `f' retained as G1 changed group of `f' to G2 Index: tests/chown/Makefile.am =================================================================== RCS file: /home/eggert/coreutils/cu/tests/chown/Makefile.am,v retrieving revision 1.4 diff -p -u -r1.4 Makefile.am --- tests/chown/Makefile.am 16 May 2004 14:10:17 -0000 1.4 +++ tests/chown/Makefile.am 18 Aug 2004 23:31:59 -0000 @@ -2,8 +2,9 @@ AUTOMAKE_OPTIONS = 1.4 gnits TESTS = \ + basic \ deref \ - basic + separator EXTRA_DIST = $(TESTS) TESTS_ENVIRONMENT = \ PATH="`pwd`/../../src$(PATH_SEPARATOR)$$PATH" --- /dev/null 2003-03-18 13:55:57 -0800 +++ tests//chown/separator 2004-08-18 19:24:08 -0700 @@ -0,0 +1,55 @@ +#!/bin/sh +# Make sure "chown USER:GROUP FILE" works, and similar tests with separators. + +if test "$VERBOSE" = yes; then + set -x + chown --version +fi + +pwd=`pwd` +t0=`echo "$0" |sed 's,.*/,,'`.tmp; tmp=$t0/$$ +trap 'status=$?; cd $pwd; chmod -R u+rwx $t0; rm -rf $t0 && exit $status' 0 +trap 'status=$?; (exit $status); exit $status' 1 2 13 15 + +framework_failure=0 + +mkdir -p $tmp || framework_failure=1 +cd $tmp || framework_failure=1 + +id_u=`id -u` || framework_failure=1 +test -n "$id_u" || framework_failure=1 + +id_un=`id -un` || framework_failure=1 +test -n "$id_un" || framework_failure=1 + +id_g=`id -g` || framework_failure=1 +test -n "$id_g" || framework_failure=1 + +id_gn=`id -gn` || framework_failure=1 +test -n "$id_gn" || framework_failure=1 + +if test $framework_failure = 1; then + echo "$0: failure in testing framework" 1>&2 + (exit 1); exit 1 +fi + +fail=0 + +chown '' . || fail=1 + +for u in $id_u $id_un ''; do + for g in $id_g $id_gn ''; do + case $u$g in + *.*) seps=':' ;; + *) 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 + done + done +done + +(exit $fail); exit $fail _______________________________________________ Bug-coreutils mailing list [EMAIL PROTECTED] http://lists.gnu.org/mailman/listinfo/bug-coreutils