-S (safe mode) would copy the file over, rename it to the target, then

do the chown/chmod dance.  This created a race where the new file would
be in place, but with the incorrect permissions.  Fix so the rename is
the last thing, instead of a middle thing.

looks ok to krw@, deraadt@
OK beck@

1 files changed, 43 insertions, 43 deletions

diff --git a/usr.bin/xinstall/xinstall.c b/usr.bin/xinstall/xinstall.c
index 53fa6a70495..7386ecdd0ab 100644
--- a/usr.bin/xinstall/xinstall.c
+++ b/usr.bin/xinstall/xinstall.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: xinstall.c,v 1.47 2007/09/05 08:58:34 jsg Exp $	*/
+/*	$OpenBSD: xinstall.c,v 1.48 2009/03/27 07:31:27 phessler Exp $	*/
 /*	$NetBSD: xinstall.c,v 1.9 1995/12/20 10:25:17 jonathan Exp $	*/
 
 /*
@@ -40,7 +40,7 @@ static char copyright[] =
 #if 0
 static char sccsid[] = "@(#)xinstall.c	8.1 (Berkeley) 7/21/93";
 #endif
-static char rcsid[] = "$OpenBSD: xinstall.c,v 1.47 2007/09/05 08:58:34 jsg Exp $";
+static char rcsid[] = "$OpenBSD: xinstall.c,v 1.48 2009/03/27 07:31:27 phessler Exp $";
 #endif /* not lint */
 
 #include <sys/param.h>
@@ -330,41 +330,9 @@ install(char *from_name, char *to_name, u_long fset, u_int flags)
 				files_match = 1;
 				(void)unlink(tempfile);
 			}
-			(void) close(temp_fd);
 		}
-	}
-
-	/*
-	 * Move the new file into place if doing a safe copy
-	 * and the files are different (or just not compared).
-	 */
-	if (safecopy && !files_match) {
-		/* Try to turn off the immutable bits. */
-		if (to_sb.st_flags & (NOCHANGEBITS))
-			(void)chflags(to_name, to_sb.st_flags & ~(NOCHANGEBITS));
-		if (dobackup) {
-			char backup[MAXPATHLEN];
-			(void)snprintf(backup, MAXPATHLEN, "%s%s", to_name,
-			    suffix);
-			/* It is ok for the target file not to exist. */
-			if (rename(to_name, backup) < 0 && errno != ENOENT) {
-				serrno = errno;
-				unlink(tempfile);
-				errx(EX_OSERR, "rename: %s to %s: %s", to_name,
-				     backup, strerror(serrno));
-			}
-		}
-		if (rename(tempfile, to_name) < 0 ) {
-			serrno = errno;
-			unlink(tempfile);
-			errx(EX_OSERR, "rename: %s to %s: %s", tempfile,
-			     to_name, strerror(serrno));
-		}
-
-		/* Re-open to_fd so we aren't hosed by the rename(2). */
-		(void) close(to_fd);
-		if ((to_fd = open(to_name, O_RDONLY, 0)) < 0)
-			err(EX_OSERR, "%s", to_name);
+		(void)close(to_fd);
+		to_fd = temp_fd;
 	}
 
 	/*
@@ -373,22 +341,25 @@ install(char *from_name, char *to_name, u_long fset, u_int flags)
 	if (dopreserve && !files_match) {
 		utb.actime = from_sb.st_atime;
 		utb.modtime = from_sb.st_mtime;
-		(void)utime(to_name, &utb);
+		(void)utime(safecopy ? tempfile : to_name, &utb);
 	}
 
 	/*
 	 * Set owner, group, mode for target; do the chown first,
 	 * chown may lose the setuid bits.
 	 */
-	if ((gid != (gid_t)-1 || uid != (uid_t)-1) && fchown(to_fd, uid, gid)) {
+	if ((gid != (gid_t)-1 || uid != (uid_t)-1) &&
+	    fchown(to_fd, uid, gid)) {
 		serrno = errno;
-		(void)unlink(to_name);
-		errx(EX_OSERR, "%s: chown/chgrp: %s", to_name, strerror(serrno));
+		(void)unlink(safecopy ? tempfile : to_name);
+		errx(EX_OSERR, "%s: chown/chgrp: %s", 
+		    safecopy ? tempfile : to_name, strerror(serrno));
 	}
 	if (fchmod(to_fd, mode)) {
 		serrno = errno;
-		(void)unlink(to_name);
-		errx(EX_OSERR, "%s: chmod: %s", to_name, strerror(serrno));
+		(void)unlink(safecopy ? tempfile : to_name);
+		errx(EX_OSERR, "%s: chmod: %s", safecopy ? tempfile : to_name,
+		    strerror(serrno));
 	}
 
 	/*
@@ -398,12 +369,41 @@ install(char *from_name, char *to_name, u_long fset, u_int flags)
 	if (fchflags(to_fd,
 	    flags & SETFLAGS ? fset : from_sb.st_flags & ~UF_NODUMP)) {
 		if (errno != EOPNOTSUPP || (from_sb.st_flags & ~UF_NODUMP) != 0)
-			warnx("%s: chflags: %s", to_name, strerror(errno));
+			warnx("%s: chflags: %s", 
+			    safecopy ? tempfile :to_name, strerror(errno));
 	}
 
 	(void)close(to_fd);
 	if (!devnull)
 		(void)close(from_fd);
+
+	/*
+	 * Move the new file into place if doing a safe copy
+	 * and the files are different (or just not compared).
+	 */
+	if (safecopy && !files_match) {
+		/* Try to turn off the immutable bits. */
+		if (to_sb.st_flags & (NOCHANGEBITS))
+			(void)chflags(to_name, to_sb.st_flags & ~(NOCHANGEBITS));
+		if (dobackup) {
+			char backup[MAXPATHLEN];
+			(void)snprintf(backup, MAXPATHLEN, "%s%s", to_name,
+			    suffix);
+			/* It is ok for the target file not to exist. */
+			if (rename(to_name, backup) < 0 && errno != ENOENT) {
+				serrno = errno;
+				unlink(tempfile);
+				errx(EX_OSERR, "rename: %s to %s: %s", to_name,
+				     backup, strerror(serrno));
+			}
+		}
+		if (rename(tempfile, to_name) < 0 ) {
+			serrno = errno;
+			unlink(tempfile);
+			errx(EX_OSERR, "rename: %s to %s: %s", tempfile,
+			     to_name, strerror(serrno));
+		}
+	}
 }
 
 /*