From owner-freebsd-current Sun Dec 9 6:51:50 2001 Delivered-To: freebsd-current@freebsd.org Received: from freefall.freebsd.org (freefall.FreeBSD.org [216.136.204.21]) by hub.freebsd.org (Postfix) with ESMTP id AB8C737B417 for ; Sun, 9 Dec 2001 06:51:44 -0800 (PST) Received: (from mckay@localhost) by freefall.freebsd.org (8.11.6/8.11.6) id fB9Epi902020 for freebsd-current@freebsd.org; Sun, 9 Dec 2001 06:51:44 -0800 (PST) (envelope-from mckay) Date: Sun, 9 Dec 2001 06:51:44 -0800 (PST) From: Message-Id: <200112091451.fB9Epi902020@freefall.freebsd.org> To: freebsd-current@freebsd.org Subject: Patch to cp to correct PR#27970 and PR#31633, for review Sender: owner-freebsd-current@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG Hi! Normally, I'd just commit this and wait for the flak, but since I'm changing the default behaviour when copying directories, I thought people might care. This patch fixes PR#27970 (directory times not preserved with -p) and PR#31633 (non-empty read-only directories not copied). It does so by setting times and permissions on directories in the post-order phase of the file hierarchy traversal. Review point 1: there is a minor functional change with this patch: umask is now applied to copied directories (assuming -p not specified) $ umask 027 $ mkdir foo $ chmod 777 foo $ cp -r foo bar1 $ patchedcp -r foo bar2 $ ls -ld foo bar? drwxrwxrwx 2 mckay wheel 512 10 Dec 00:29 foo drwxrwxrwx 2 mckay wheel 512 10 Dec 00:29 bar1 drwxr-x--- 2 mckay wheel 512 10 Dec 00:29 bar2 $ I believe the new behaviour is correct. It follows SUSv2, and matches GNU cp. I know of nothing that will fail with this change. Review point 2: in order to avoid a chmod() per directory in the normal case, there is a complicated conditional to set curr->fts_number. If I changed this to simply: curr->fts_number = dne; then readability and testability would be enhanced, at the cost of a couple of unnecessary chmod() system calls. I'm leaning towards ditching the conditional. Anybody against? I'll commit this is a day or two, unless there are any problems. Also, there are a number of other open PRs against cp which I hope to commit fixes for. In particular PR#17389 should be fixed. Oh, and the typo fix to util.c is sort of a freebie. :-) Stephen. PS Some people use -audit for code reviews. But the charter claims it is for security auditing. Which is right? Index: cp.c =================================================================== RCS file: /cvs/src/bin/cp/cp.c,v retrieving revision 1.27 diff -u -r1.27 cp.c --- cp.c 2001/06/19 15:41:54 1.27 +++ cp.c 2001/12/09 14:50:39 @@ -248,7 +248,15 @@ FTSENT *curr; int base = 0, dne, badcp, nlen, rval; char *p, *target_mid; + mode_t mask; + /* + * Keep an inverted copy of the umask, for use in correcting + * permissions on created directories when not using -p. + */ + mask = ~umask(0777); + umask(~mask); + if ((ftsp = fts_open(argv, fts_options, mastercmp)) == NULL) err(1, NULL); for (badcp = rval = 0; (curr = fts_read(ftsp)) != NULL; badcp = 0) { @@ -264,8 +272,6 @@ warnx("%s: directory causes a cycle", curr->fts_path); badcp = rval = 1; continue; - case FTS_DP: /* Ignore, continue. */ - continue; } /* @@ -323,6 +329,25 @@ STRIP_TRAILING_SLASH(to); } + if (curr->fts_info == FTS_DP) { + /* + * We are finished copying to this directory. If + * -p is in effect, set permissions and timestamps. + * Otherwise, if we created this directory, set the + * correct permissions, limited by the umask. + */ + if (pflag) + rval = setfile(curr->fts_statp, 0); + else if (curr->fts_number) { + mode_t perm = curr->fts_statp->st_mode & mask; + if (chmod(to.p_path, perm)) { + warn("chmod: %s", to.p_path); + rval = 1; + } + } + continue; + } + /* Not an error but need to remember it happened */ if (stat(to.p_path, &to_stat) == -1) dne = 1; @@ -376,16 +401,19 @@ err(1, "%s", to.p_path); } /* - * If not -p and directory didn't exist, set it to be - * the same as the from directory, umodified by the - * umask; arguably wrong, but it's been that way - * forever. + * Arrange to correct directory permissions later + * (in the post-order phase) if this is a new + * directory and the permissions aren't the final + * ones we want yet. Note that mkdir() does not + * honour setuid, setgid nor sticky bits, but we + * normally want to preserve them on directories. */ - if (pflag && setfile(curr->fts_statp, 0)) - badcp = rval = 1; - else if (dne) - (void)chmod(to.p_path, - curr->fts_statp->st_mode); + { + mode_t mode = curr->fts_statp->st_mode; + curr->fts_number = dne && + ((mode & (S_ISUID|S_ISGID|S_ISTXT)) || + ((mode | S_IRWXU) & mask) != (mode & mask)); + } break; case S_IFBLK: case S_IFCHR: Index: utils.c =================================================================== RCS file: /cvs/src/bin/cp/utils.c,v retrieving revision 1.31 diff -u -r1.31 utils.c --- utils.c 2001/06/19 15:41:54 1.31 +++ utils.c 2001/12/09 14:17:28 @@ -293,7 +293,7 @@ if (!gotstat || fs->st_mode != ts.st_mode) if (fd ? fchmod(fd, fs->st_mode) : chmod(to.p_path, fs->st_mode)) { - warn("chown: %s", to.p_path); + warn("chmod: %s", to.p_path); rval = 1; } To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-current" in the body of the message