Date: Tue, 18 Sep 2001 19:02:22 +0300 From: Ruslan Ermilov <ru@FreeBSD.ORG> To: "Andrey A. Chernov" <ache@nagual.pp.ru>, Garrett Wollman <wollman@khavrinen.lcs.mit.edu> Cc: arch@FreeBSD.ORG, freebsd-standards@bostonradio.org Subject: Re: Fixing chown(8) and friends to handle symlinks properly Message-ID: <20010918190222.F4648@sunbay.com> In-Reply-To: <20010914060800.A27869@nagual.pp.ru>; from ache@nagual.pp.ru on Fri, Sep 14, 2001 at 06:08:02AM %2B0400 References: <20010913202742.C2458@sunbay.com> <20010917191657.A19072@sunbay.com> <200109172143.f8HLhkU41334@khavrinen.lcs.mit.edu> <20010913202742.C2458@sunbay.com> <20010914060800.A27869@nagual.pp.ru>
next in thread | previous in thread | raw e-mail | index | archive | help
--DocE+STaALJfprDB Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Hi, After re-reading the POSIX.1-200x draft and the symlink(7) manpage, I have developed another patch (the winner!) that implements what I think is required by POSIX, while still preserving the basic ideas of symlink(7). As Garrett properly pointed out, in POSIX, as well as in symlink(7), -h is required to affect the lchown(2)/chown(2) selection only in the non-recursive case. Note also that POSIX differentiates between a symlink referencing a file of type directory and a symlink referencing a non-directory type object. In the -RH case, if symlink to a directory is specified on the command line, POSIX says we should change the ownership of the directory referenced by a symlink and all files in the hierarchy below it. FreeBSD currently follows a symlink referencing a non-directory object as well. In the -RL case, all symlinks to directories, either specified on the command line or encountered during the tree traversal, should be followed. FreeBSD currently follows any symlinks it encounters during the tree traversal, regardless of the type of a file they point to. In the -RP case, no symlinks should be followed and chown(8) should change the ownership of symlinks with lchown(2). This applies to any type of a symlink, whether it points to a directory or not. POSIX doesn't say anything about what to do with symlinks to non-directory objects encountered during a tree traversal in the -RH and -RL cases. Also, it doesn't say what to do with symlinks to directories in the -RH case that were encountered during a tree traversal. The attached patch resolves this as follows. For every symlink that is NOT followed, we call lchown(2). For -RP, this is what is required by POSIX. It is my guess that this is the behavior that was actually planned by the POSIX authors. Note also that we now never follow symlinks to non-directories during the -R traversal. Currently, in the -RL case, FreeBSD follows ALL symlinks. Now, only symlinks to directories are followed, as regulated by -H and -L. This is a good security change, indeed! This patch also restores the optimization based on the contents of the ``fts_statp'' structure member. To summarize, the algorithm operates as follows: +---------+-----------------------+-----------------------+ | Options | Directory symlink | Non-directory symlink | +---------+-----------------------+-----------------------+ | -- | follow | follow | +---------+-----------------------+-----------------------+ | -h | lchown(2) | lchown(2) | +---------+-----------------------+-----------------------+ | -RP | lchown(2) | lchown(2) | +---------+-----------------------+-----------------------+ | -RH | follow if on cmdline, | lchown(2) | | | lchown(2) otherwise | | +---------+-----------------------+-----------------------+ | -RL | follow | lchown(2) | +---------+-----------------------+-----------------------+ Notes for the reviewers. 1. We now always perform the FTS_PHYSICAL search, and only follow symlinks that reference directories, as determined by the -H and -L flags. 2. In the non-recursive case, if the file operand designated a symlink and no -h was specified, we follow it. At first glance it may seem we could just chown(2) it. This would be undesirable for two reasons. First, this would result in a ENOENT (like in the previous version of the patch) if the underlying object does not exist -- OTOH, FreeBSD currently silently ignores this. Second, we need to make sure that ``fts_statp'' is properly initialized by either stat(2) or lstat(2) depending on whether we are going to chown(2) or lchown(2) the object because the heuristic code relies on the contents of ``fts_statp'' to avoid unnecessary syscalls. For non-symlinks this is irrelevant, as chown(2) and lchown(2) return the same information. But in this case we are going to call chown(2) on a symlink, and since we perform a physical search, fts_read() initializes ``fts_statp'' with lchown(2). Here we simply follow it and kill two birds at once -- if the link points to a non-existing object, the next call to fts_read() will return FTS_SLNONE, and we silently skip it, as in the -R case. Otherwise, fts_read() reinitializes the ``fts_statp'' by a call to stat(2), and our heuristics code (commented out in the DEBUG case) can rely on the contents of the ``struct stat'' buffer. On Fri, Sep 14, 2001 at 06:08:02AM +0400, Andrey A. Chernov wrote: > On Thu, Sep 13, 2001 at 20:27:42 +0300, Ruslan Ermilov wrote: > > > NOTE: This implementation is still not quite POSIX compliant, as > > the latter says to follow a symlink (in the -R case) only if it > > points to an object of type directory. Our fts(3) routines are > > unable of doing this. > > > Is there a way to fix fts(3) to implement this? I.e. modify fts(3) to make > directory test before following anywhere. On Mon, Sep 17, 2001 at 05:43:46PM -0400, Garrett Wollman wrote: > <<On Mon, 17 Sep 2001 19:16:57 +0300, Ruslan Ermilov <ru@FreeBSD.org> said: > > > Unless I hear from somebody that they are wishing to review > > this patch, I will take it as if noone currently has enough > > spare time to review this, and will just commit this patch > > on Thursday, 20th. :-) > > >> | -RP | SKIP | > >> | -RP -h | lchown | > >> | | Notes: only FTS_SL symlinks may end here | > > This is not right. Draft 7 states: > > -P If the -R option is specified and a symbolic link is specified > on the command line or encountered during the traversal of a > file hierarchy, chown shall change the owner ID (and group ID, > if specified) of the symbolic link if the system supports this > operation. The chown utility shall not follow the symbolic link > to any other part of the file hierarchy. > > That is to say, when `-R -P' is specified, the owner (and group) shall > be changed, regardless of whether `-h' has been specified. > > My interepretation of the description in draft 7 is that `-h' applies > *only* to command-line operands, and is not relevant to recursive > operation, where `-P', and only `-P', can result in lchown(). -- Ruslan Ermilov Oracle Developer/DBA, ru@sunbay.com Sunbay Software AG, ru@FreeBSD.org FreeBSD committer, +380.652.512.251 Simferopol, Ukraine http://www.FreeBSD.org The Power To Serve http://www.oracle.com Enabling The Information Age --DocE+STaALJfprDB Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename=p Index: chown.c =================================================================== RCS file: /home/ncvs/src/usr.sbin/chown/chown.c,v retrieving revision 1.19 diff -u -p -u -p -r1.19 chown.c --- chown.c 2001/09/13 14:55:59 1.19 +++ chown.c 2001/09/18 15:56:37 @@ -78,8 +78,10 @@ main(argc, argv) FTS *ftsp; FTSENT *p; int Hflag, Lflag, Rflag, fflag, hflag, vflag; - int ch, fts_options, rval; + int ch, rval; char *cp; + struct stat sb; + int (*change_owner) __P((const char *, uid_t, gid_t)); cp = strrchr(argv[0], '/'); cp = (cp != NULL) ? cp + 1 : argv[0]; @@ -121,20 +123,6 @@ main(argc, argv) if (argc < 2) usage(); - if (Rflag) { - fts_options = FTS_PHYSICAL; - if (hflag && (Hflag || Lflag)) - errx(1, "the -R%c and -h options may not be " - "specified together", Hflag ? 'H' : 'L'); - if (Hflag) - fts_options |= FTS_COMFOLLOW; - else if (Lflag) { - fts_options &= ~FTS_PHYSICAL; - fts_options |= FTS_LOGICAL; - } - } else - fts_options = hflag ? FTS_PHYSICAL : FTS_LOGICAL; - uid = (uid_t)-1; gid = (gid_t)-1; if (ischown) { @@ -152,10 +140,11 @@ main(argc, argv) } else a_gid(*argv); - if ((ftsp = fts_open(++argv, fts_options, 0)) == NULL) + if ((ftsp = fts_open(++argv, FTS_PHYSICAL, 0)) == NULL) err(1, NULL); for (rval = 0; (p = fts_read(ftsp)) != NULL;) { + change_owner = chown; switch (p->fts_info) { case FTS_D: /* Change it at FTS_DP. */ if (!Rflag) @@ -171,30 +160,65 @@ main(argc, argv) rval = 1; continue; case FTS_SL: + /* + * If not traversing a file tree and -h was not + * specified, follow symlinks. This is done to + * reinitialize the p->fts_statp with stat(2). + * + * If traversing a file tree, check if we were + * asked to follow symlinks to directories. + */ + if ((!Rflag && !hflag) || + (Rflag && (Lflag || (Hflag && p->fts_level == 0)) && + stat(p->fts_accpath, &sb) == 0 && + S_ISDIR(sb.st_mode))) { + fts_set(ftsp, p, FTS_FOLLOW); + continue; + } + if (hflag || Rflag) + change_owner = lchown; + break; case FTS_SLNONE: /* * The only symlinks that end up here are ones that - * don't point to anything and ones that we found - * doing a physical walk. + * don't point to anything. Note that since we are + * doing a physical walk, we never reach here unless + * we asked to follow explicitly. + * + * When following a symlink to a directory above, + * the directory may be deleted (or renamed) in a + * small window between the calls to fts_set() and + * fts_read(), causing FTS_SLNONE to be returned. */ - if (hflag) - break; - else - continue; + continue; default: break; } +#define DEBUG +#ifdef DEBUG + if ((change_owner == chown ? stat : lstat)(p->fts_accpath, &sb) == -1) + abort(); + if (sb.st_ino != p->fts_statp->st_ino) + abort(); +#else if ((uid == (uid_t)-1 || uid == p->fts_statp->st_uid) && (gid == (gid_t)-1 || gid == p->fts_statp->st_gid)) continue; - if ((hflag ? lchown : chown)(p->fts_accpath, uid, gid) == -1) { +#endif + if ((*change_owner)(p->fts_accpath, uid, gid) == -1) { if (!fflag) { chownerr(p->fts_path); rval = 1; } } else { if (vflag) +#ifdef DEBUG + printf("%s %s\n", + (change_owner == chown) ? "chown" : "lchown", + p->fts_path); +#else printf("%s\n", p->fts_path); +#endif } } if (errno) @@ -275,11 +299,12 @@ usage() if (ischown) (void)fprintf(stderr, "%s\n%s\n", - "usage: chown [-fhv] [-R [-H | -L | -P]] owner[:group]" + "usage: chown [-fv] [-h | -R [-H | -L | -P]] owner[:group]" " file ...", - " chown [-fhv] [-R [-H | -L | -P]] :group file ..."); + " chown [-fv] [-h | -R [-H | -L | -P]] :group" + " file ..."); else (void)fprintf(stderr, "%s\n", - "usage: chgrp [-fhv] [-R [-H | -L | -P]] group file ..."); + "usage: chgrp [-fv] [-h | -R [-H | -L | -P]] group file ..."); exit(1); } Index: chown.8 =================================================================== RCS file: /home/ncvs/src/usr.sbin/chown/chown.8,v retrieving revision 1.17 diff -u -p -u -p -r1.17 chown.8 --- chown.8 2001/09/11 13:07:03 1.17 +++ chown.8 2001/09/18 15:56:38 @@ -32,7 +32,7 @@ .\" @(#)chown.8 8.3 (Berkeley) 3/31/94 .\" $FreeBSD: src/usr.sbin/chown/chown.8,v 1.17 2001/09/11 13:07:03 dd Exp $ .\" -.Dd March 31, 1994 +.Dd September 18, 2001 .Dt CHOWN 8 .Os .Sh NAME @@ -40,59 +40,89 @@ .Nd change file owner and group .Sh SYNOPSIS .Nm -.Op Fl fhv -.Oo -.Fl R -.Op Fl H | Fl L | Fl P -.Oc +.Op Fl fv +.Op Fl h | R Op Fl H | L | P .Ar owner Ns Op : Ns Ar group .Ar .Nm -.Op Fl fhv -.Oo -.Fl R -.Op Fl H | Fl L | Fl P -.Oc +.Op Fl fv +.Op Fl h | R Op Fl H | L | P .No : Ns Ar group .Ar .Sh DESCRIPTION -.Nm Chown -sets the user ID and/or the group ID of the specified files. +The +.Nm +utility changes the user ID and/or the group ID of the specified files. .Pp -The options are as follows: -.Bl -tag -width Ds +The following options are available: +.Bl -tag -width indent .It Fl H If the .Fl R -option is specified, symbolic links on the command line are followed. -(Symbolic links encountered in the tree traversal are not followed.) +option is specified +and a symbolic link +referencing a file of type directory +is specified on the command line, +change the user ID and/or the group ID +of the directory referenced by the link +and all files in the subtree below it. +Otherwise, +change the user ID and/or the group ID +of the link itself +rather than the file that the link points to. .It Fl L If the .Fl R -option is specified, all symbolic links are followed. +option is specified +and a symbolic link +referencing a file of type directory +is specified on the command line +or encountered during the tree traversal, +change the user ID and/or the group ID +of the directory referenced by the link +and all files in the subtree below it. +Otherwise, +change the user ID and/or the group ID +of the link itself +rather than the file that the link points to. .It Fl P If the .Fl R -option is specified, no symbolic links are followed. -This is the default. +option is specified +and a symbolic link +is specified on the command line +or encountered during the tree traversal, +change the user ID and/or the group ID +of the link itself +rather than the file that the link points to. .It Fl R -Change the user ID and/or the group ID for the file hierarchies rooted -in the files instead of just the files themselves. +If +.Ar file +argument designates a directory, +change the user ID and/or the group ID +of the directory +and all files in the subtree below it. .It Fl f -Don't report any failure to change file owner or group, nor modify -the exit status to reflect such failures. +Do not report any failure to +change the user ID and/or the group ID +of the file, +nor modify the exit status to reflect such failures. .It Fl h -If the file is a symbolic link, change the user ID and/or the group ID -of the link itself rather than the file that the link points to. +If the +.Fl R +option is not specified +and +.Ar file +argument is a symbolic link, +change the user ID and/or the group ID +of the link itself +rather than the file that the link points to. .It Fl v -Cause -.Nm -to be verbose, showing files as the owner is modified. +Be verbose, showing files as the owner and/or group is modified. .El .Pp The -.Fl H , -.Fl L +.Fl H , L and .Fl P options are ignored unless the @@ -100,6 +130,9 @@ options are ignored unless the option is specified. In addition, these options override each other and the command's actions are determined by the last one specified. +If none are specified, the +default is +.Fl P . .Pp The .Ar owner @@ -108,7 +141,9 @@ and operands are both optional, however, one must be specified. If the .Ar group -operand is specified, it must be preceded by a colon (``:'') character. +operand is specified, it must be preceded by a colon +.Pq Dq Li \&: +character. .Pp The .Ar owner @@ -128,8 +163,12 @@ obvious security reasons. .Sh COMPATIBILITY Previous versions of the .Nm -utility used the dot (``.'') character to distinguish the group name. -This has been changed to be a colon (``:'') character so that user and +utility used the dot +.Pq Dq Li \&. +character to distinguish the group name. +This has been changed to be a colon +.Pq Dq Li \&: +character so that user and group names may contain the dot character. .Pp On previous versions of this system, symbolic links did not have @@ -143,15 +182,18 @@ option is non-standard and its use in sc .Xr find 1 , .Xr chown 2 , .Xr fts 3 , +.Xr group 5 , +.Xr passwd 5 , .Xr symlink 7 .Sh STANDARDS The .Nm -command is expected to be -.St -p1003.2 +utility is expected to be +.\" .St -p1003.1-200x +.Tn IEEE No Std 1003.1-200x Pq Dq Tn POSIX Ns .1 compliant. .Sh HISTORY A .Nm -command appeared in +utility appeared in .At v1 . Index: chgrp.1 =================================================================== RCS file: /home/ncvs/src/usr.sbin/chown/chgrp.1,v retrieving revision 1.13 diff -u -p -u -p -r1.13 chgrp.1 --- chgrp.1 2001/08/15 09:09:46 1.13 +++ chgrp.1 2001/09/18 15:56:38 @@ -35,7 +35,7 @@ .\" @(#)chgrp.1 8.3 (Berkeley) 3/31/94 .\" $FreeBSD: src/usr.sbin/chown/chgrp.1,v 1.13 2001/08/15 09:09:46 ru Exp $ .\" -.Dd March 31, 1994 +.Dd September 18, 2001 .Dt CHGRP 1 .Os .Sh NAME @@ -43,56 +43,84 @@ .Nd change group .Sh SYNOPSIS .Nm -.Op Fl fhv -.Oo -.Fl R -.Op Fl H | Fl L | Fl P -.Oc +.Op Fl fv +.Op Fl h | R Op Fl H | L | P .Ar group .Ar .Sh DESCRIPTION The .Nm -utility sets the group ID of the file named by each -.Ar file -operand to the -.Ar group -ID specified by the group operand. +utility changes the group ID of the specified files. .Pp The following options are available: .Bl -tag -width indent .It Fl H If the .Fl R -option is specified, symbolic links on the command line are followed. -(Symbolic links encountered in the tree traversal are not followed). +option is specified +and a symbolic link +referencing a file of type directory +is specified on the command line, +change the group ID +of the directory referenced by the link +and all files in the subtree below it. +Otherwise, +change the group ID +of the link itself +rather than the file that the link points to. .It Fl L If the .Fl R -option is specified, all symbolic links are followed. +option is specified +and a symbolic link +referencing a file of type directory +is specified on the command line +or encountered during the tree traversal, +change the group ID +of the directory referenced by the link +and all files in the subtree below it. +Otherwise, +change the group ID +of the link itself +rather than the file that the link points to. .It Fl P If the .Fl R -option is specified, no symbolic links are followed. -This is the default. +option is specified +and a symbolic link +is specified on the command line +or encountered during the tree traversal, +change the group ID +of the link itself +rather than the file that the link points to. .It Fl R -Change the group ID for the file hierarchies rooted -in the files instead of just the files themselves. +If +.Ar file +argument designates a directory, +change the group ID +of the directory +and all files in the subtree below it. .It Fl f -The force option ignores errors, except for usage errors and doesn't -query about strange modes (unless the user does not have proper permissions). +Do not report any failure to +change the group ID +of the file, +nor modify the exit status to reflect such failures. .It Fl h -If the file is a symbolic link, the group ID of the link itself is changed -rather than the file that is pointed to. +If the +.Fl R +option is not specified +and +.Ar file +argument is a symbolic link, +change the group ID +of the link itself +rather than the file that the link points to. .It Fl v -Cause -.Nm -to be verbose, showing files as the group is modified. +Be verbose, showing files as the group is modified. .El .Pp The -.Fl H , -.Fl L +.Fl H , L and .Fl P options are ignored unless the @@ -100,11 +128,13 @@ options are ignored unless the option is specified. In addition, these options override each other and the command's actions are determined by the last one specified. +If none are specified, the +default is +.Fl P . .Pp The .Ar group -operand can be either a group name from the group database, -or a numeric group ID. +may be either a numeric group ID or a group name. If a group name is also a numeric group ID, the operand is used as a group name. .Pp @@ -115,17 +145,19 @@ or be the super-user. .Sh DIAGNOSTICS .Ex -std .Sh COMPATIBILITY -In previous versions of this system, symbolic links did not have groups. +On previous versions of this system, symbolic links did not have +groups. .Pp The .Fl v option is non-standard and its use in scripts is not recommended. .Sh FILES -.Bl -tag -width /etc/group -compact +.Bl -tag -width ".Pa /etc/group" -compact .It Pa /etc/group group ID file .El .Sh SEE ALSO +.Xr find 1 , .Xr chown 2 , .Xr fts 3 , .Xr group 5 , @@ -136,5 +168,6 @@ group ID file The .Nm utility is expected to be -.St -p1003.2 -compatible. +.\" .St -p1003.1-200x +.Tn IEEE No Std 1003.1-200x Pq Dq Tn POSIX Ns .1 +compliant. --DocE+STaALJfprDB-- To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-arch" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20010918190222.F4648>