Date: Sun, 9 Dec 2001 06:51:44 -0800 (PST) From: <mckay@FreeBSD.org> To: freebsd-current@freebsd.org Subject: Patch to cp to correct PR#27970 and PR#31633, for review Message-ID: <200112091451.fB9Epi902020@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200112091451.fB9Epi902020>