Hi,
Lauri Tirkkonen wrote on Mon, Jan 07, 2019 at 08:13:09PM +0200:
> Hi, it seems install(1) has a race condition: in create_newfile, it
> first unlinks the target file and then tries to open it with
> O_CREAT|O_EXCL.
>
> Normally this would not be a problem,
A race condition is almost always a problem and can almost always
cause incorrect behaviour. In this case, *anything* might create
the file in the time window, causing spurious failures, in unusual
scenarios maybe even facilitating DOS attacks. The fact that some
build systems shoot themselves in the foot the way you describe
merely exacerbates the bug.
> they seem to install the same set of headers at least twice.
> Granted, that doesn't make any sense, but that's what their build
> seems to do...
You are right that installing stuff twice is bad style, but it isn't
as uncommon as a one might naively think. Getting Makefiles right
is not trivial, and having stuff done more than once is not an
unusual issue in real-world Makefiles according to my experience.
> The below diff essentially removes the -S option
We clearly cannot delete -S from the user interface. That would break
all kinds of build systems for no benefit. It is even used in base,
for example in bsd.lib.mk. I don't doubt it's used in ports, too,
and having to fix it there would be even more annoying than in base.
> and makes install always use temp files (ie. -S is always on),
> eliminating the race since rename(2) cannot fail like this.
I think that is the right thing to do. Even if it happens to
increase resource consumption with softupdates, that is of little
relevance because we advise against using softupdates in the first
place, for reasons of reliability. Also, bugs in one subsystem
must not prevent bugfixing in another.
Besides, when asked whether to prefer correctness or performance,
our strict answer is that good performance is totally useless unless
the result is also correct and reliable.
If people here agree with the general direction of making -S the
default and removing the fragile non-S mode (see the patch below),
i'll run a full make build and make release and then ask for OKs.
Yours,
Ingo
Index: install.1
===================================================================
RCS file: /cvs/src/usr.bin/xinstall/install.1,v
retrieving revision 1.30
diff -u -p -r1.30 install.1
--- install.1 13 May 2016 17:51:15 -0000 1.30
+++ install.1 16 Jan 2019 09:52:09 -0000
@@ -141,15 +141,12 @@ except if the target file doesn't alread
then preserve the modification time of the file.
.It Fl S
Safe copy.
-Normally,
-.Nm
-unlinks an existing target before installing the new file.
-With the
-.Fl S
-flag a temporary file is used and then renamed to be
-the target.
-The reason this is safer is that if the copy or
-rename fails, the existing target is left untouched.
+This is the default.
+This option has no effect and is supported only for compatibility.
+When installing a file, a temporary file is created and written first
+in the destination directory, then atomically renamed.
+This avoids both race conditions and the destruction of existing
+files in case of write failures.
.It Fl s
.Nm
exec's the command
@@ -186,18 +183,8 @@ Default is
.Sh FILES
.Bl -tag -width INS@XXXXXXXXXX -compact
.It Pa INS@XXXXXXXXXX
-If either
-.Fl S
-option is specified, or the
-.Fl C
-or
-.Fl p
-option is used in conjunction with the
-.Fl s
-option, temporary files named INS@XXXXXXXXXX,
-where XXXXXXXXXX is decided by
-.Xr mkstemp 3 ,
-are created in the target directory.
+Temporary files created in the target directory by
+.Xr mkstemp 3 .
.El
.Sh EXIT STATUS
.Ex -std install
Index: xinstall.c
===================================================================
RCS file: /cvs/src/usr.bin/xinstall/xinstall.c,v
retrieving revision 1.67
diff -u -p -r1.67 xinstall.c
--- xinstall.c 16 Sep 2018 02:44:07 -0000 1.67
+++ xinstall.c 16 Jan 2019 09:52:10 -0000
@@ -60,7 +60,7 @@
#define NOCHANGEBITS (UF_IMMUTABLE | UF_APPEND | SF_IMMUTABLE | SF_APPEND)
#define BACKUP_SUFFIX ".old"
-int dobackup, docompare, dodest, dodir, dopreserve, dostrip, safecopy;
+int dobackup, docompare, dodest, dodir, dopreserve, dostrip;
int mode = S_IRWXU|S_IRGRP|S_IXGRP|S_IROTH|S_IXOTH;
char pathbuf[PATH_MAX], tempfile[PATH_MAX];
char *suffix = BACKUP_SUFFIX;
@@ -73,7 +73,6 @@ void install(char *, char *, u_long, u_i
void install_dir(char *, int);
void strip(char *);
void usage(void);
-int create_newfile(char *, struct stat *);
int create_tempfile(char *, char *, size_t);
int file_write(int, char *, size_t, int *, int *, int);
void file_flush(int, int);
@@ -129,7 +128,7 @@ main(int argc, char *argv[])
docompare = dopreserve = 1;
break;
case 'S':
- safecopy = 1;
+ /* For backwards compatibility. */
break;
case 's':
dostrip = 1;
@@ -148,17 +147,13 @@ main(int argc, char *argv[])
argv += optind;
/* some options make no sense when creating directories */
- if ((safecopy || docompare || dostrip) && dodir)
+ if ((docompare || dostrip) && dodir)
usage();
/* must have at least two arguments, except when creating directories */
if (argc < 2 && !dodir)
usage();
- /* need to make a temp copy so we can compare stripped version */
- if (docompare && dostrip)
- safecopy = 1;
-
/* get group and owner id's */
if (group != NULL && gid_from_group(group, &gid) == -1) {
gid = strtonum(group, 0, GID_MAX, &errstr);
@@ -265,54 +260,30 @@ install(char *from_name, char *to_name,
err(1, "%s", from_name);
}
- if (safecopy) {
- to_fd = create_tempfile(to_name, tempfile, sizeof(tempfile));
- if (to_fd < 0)
- err(1, "%s", tempfile);
- } else if (docompare && !dostrip) {
- if ((to_fd = open(to_name, O_RDONLY, 0)) < 0)
- err(1, "%s", to_name);
- } else {
- if ((to_fd = create_newfile(to_name, &to_sb)) < 0)
- err(1, "%s", to_name);
- }
+ to_fd = create_tempfile(to_name, tempfile, sizeof(tempfile));
+ if (to_fd < 0)
+ err(1, "%s", tempfile);
- if (!devnull) {
- if (docompare && !safecopy) {
- files_match = !(compare(from_fd, from_name,
- from_sb.st_size, to_fd,
- to_name, to_sb.st_size));
-
- /* Truncate "to" file for copy unless we match */
- if (!files_match) {
- (void)close(to_fd);
- if ((to_fd = create_newfile(to_name, &to_sb)) <
0)
- err(1, "%s", to_name);
- }
- }
- if (!files_match)
- copy(from_fd, from_name, to_fd,
- safecopy ? tempfile : to_name, from_sb.st_size,
- ((off_t)from_sb.st_blocks * S_BLKSIZE <
from_sb.st_size));
- }
+ if (!devnull)
+ copy(from_fd, from_name, to_fd, tempfile, from_sb.st_size,
+ ((off_t)from_sb.st_blocks * S_BLKSIZE < from_sb.st_size));
if (dostrip) {
- strip(safecopy ? tempfile : to_name);
+ strip(tempfile);
/*
* Re-open our fd on the target, in case we used a strip
* that does not work in-place -- like gnu binutils strip.
*/
close(to_fd);
- if ((to_fd = open(safecopy ? tempfile : to_name, O_RDONLY,
- 0)) < 0)
+ if ((to_fd = open(tempfile, O_RDONLY, 0)) < 0)
err(1, "stripping %s", to_name);
}
/*
* Compare the (possibly stripped) temp file to the target.
*/
- if (safecopy && docompare) {
+ if (docompare) {
int temp_fd = to_fd;
struct stat temp_sb;
@@ -362,15 +333,13 @@ install(char *from_name, char *to_name,
if ((gid != (gid_t)-1 || uid != (uid_t)-1) &&
fchown(to_fd, uid, gid)) {
serrno = errno;
- (void)unlink(safecopy ? tempfile : to_name);
- errx(1, "%s: chown/chgrp: %s",
- safecopy ? tempfile : to_name, strerror(serrno));
+ (void)unlink(tempfile);
+ errx(1, "%s: chown/chgrp: %s", tempfile, strerror(serrno));
}
if (fchmod(to_fd, mode)) {
serrno = errno;
- (void)unlink(safecopy ? tempfile : to_name);
- errx(1, "%s: chmod: %s", safecopy ? tempfile : to_name,
- strerror(serrno));
+ (void)unlink(tempfile);
+ errx(1, "%s: chmod: %s", tempfile, strerror(serrno));
}
/*
@@ -380,8 +349,7 @@ install(char *from_name, char *to_name,
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",
- safecopy ? tempfile :to_name, strerror(errno));
+ warnx("%s: chflags: %s", tempfile, strerror(errno));
}
if (flags & USEFSYNC)
@@ -391,10 +359,10 @@ install(char *from_name, char *to_name,
(void)close(from_fd);
/*
- * Move the new file into place if doing a safe copy
- * and the files are different (or just not compared).
+ * Move the new file into place if the files are different
+ * or were not compared.
*/
- if (safecopy && !files_match) {
+ if (!files_match) {
/* Try to turn off the immutable bits. */
if (to_sb.st_flags & (NOCHANGEBITS))
(void)chflags(to_name, to_sb.st_flags &
~(NOCHANGEBITS));
@@ -655,36 +623,6 @@ create_tempfile(char *path, char *temp,
strlcat(p, "INS@XXXXXXXXXX", tsize);
return(mkstemp(temp));
-}
-
-/*
- * create_newfile --
- * create a new file, overwriting an existing one if necessary
- */
-int
-create_newfile(char *path, struct stat *sbp)
-{
- char backup[PATH_MAX];
-
- /*
- * Unlink now... avoid ETXTBSY errors later. Try and turn
- * off the append/immutable bits -- if we fail, go ahead,
- * it might work.
- */
- if (sbp->st_flags & (NOCHANGEBITS))
- (void)chflags(path, sbp->st_flags & ~(NOCHANGEBITS));
-
- if (dobackup) {
- (void)snprintf(backup, PATH_MAX, "%s%s", path, suffix);
- /* It is ok for the target file not to exist. */
- if (rename(path, backup) < 0 && errno != ENOENT)
- err(1, "rename: %s to %s (errno %d)", path, backup,
errno);
- } else {
- if (unlink(path) < 0 && errno != ENOENT)
- err(1, "%s", path);
- }
-
- return(open(path, O_CREAT | O_RDWR | O_EXCL, S_IRUSR | S_IWUSR));
}
/*
