install(1) could fail due to race

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

install(1) could fail due to race

Lauri Tirkkonen-2
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, but I actually ran into this when
parallel building gcc 6.4.0 (on another OS, but we use OpenBSD
install(1)): 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...

Easy way to repro this failure is to use a Makefile like:
    all: 1 2 3 4 5 6 7 8

    1:
            install /etc/passwd .
    2:
            install /etc/passwd .
    3:
            install /etc/passwd .
    4:
            install /etc/passwd .
    5:
            install /etc/passwd .
    6:
            install /etc/passwd .
    7:
            install /etc/passwd .
    8:
            install /etc/passwd .

and run while make -j8; do :; done

The below diff essentially removes the -S option and makes install
always use temp files (ie. -S is always on), eliminating the race since
rename(2) cannot fail like this.

diff --git a/usr.bin/xinstall/install.1 b/usr.bin/xinstall/install.1
index fd5db0abc37..f7d8707ce6f 100644
--- a/usr.bin/xinstall/install.1
+++ b/usr.bin/xinstall/install.1
@@ -38,7 +38,7 @@
 .Nd install binaries
 .Sh SYNOPSIS
 .Nm install
-.Op Fl bCcDdFpSs
+.Op Fl bCcDdFps
 .Op Fl B Ar suffix
 .Op Fl f Ar flags
 .Op Fl g Ar group
@@ -139,17 +139,6 @@ Copy the file, as if the
 (compare and copy) option is specified,
 except if the target file doesn't already exist or is different,
 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.
 .It Fl s
 .Nm
 exec's the command
@@ -186,18 +175,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
diff --git a/usr.bin/xinstall/xinstall.c b/usr.bin/xinstall/xinstall.c
index c08d82eeed4..b6f5c595646 100644
--- a/usr.bin/xinstall/xinstall.c
+++ b/usr.bin/xinstall/xinstall.c
@@ -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_int);
 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);
@@ -128,9 +127,6 @@ main(int argc, char *argv[])
  case 'p':
  docompare = dopreserve = 1;
  break;
- case 'S':
- safecopy = 1;
- break;
  case 's':
  dostrip = 1;
  break;
@@ -148,17 +144,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 +257,30 @@ install(char *from_name, char *to_name, u_long fset, u_int flags)
  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 +330,13 @@ install(char *from_name, char *to_name, u_long fset, u_int flags)
  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 +346,7 @@ 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",
-    safecopy ? tempfile :to_name, strerror(errno));
+ warnx("%s: chflags: %s", tempfile, strerror(errno));
  }
 
  if (flags & USEFSYNC)
@@ -391,10 +356,10 @@ install(char *from_name, char *to_name, u_long fset, u_int flags)
  (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 just 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));
@@ -632,7 +597,7 @@ void
 usage(void)
 {
  (void)fprintf(stderr, "\
-usage: install [-bCcDdFpSs] [-B suffix] [-f flags] [-g group] [-m mode] [-o owner]\n       source ... target ...\n");
+usage: install [-bCcDdFps] [-B suffix] [-f flags] [-g group] [-m mode] [-o owner]\n       source ... target ...\n");
  exit(1);
  /* NOTREACHED */
 }
@@ -657,36 +622,6 @@ create_tempfile(char *path, char *temp, size_t 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));
-}
-
 /*
  * file_write()
  * Write/copy a file (during copy or archive extract). This routine knows

--
Lauri Tirkkonen | lotheac @ IRCnet

Reply | Threaded
Open this post in threaded view
|

Re: install(1) could fail due to race

Lauri Tirkkonen-2
On Mon, Jan 07 2019 20:13:09 +0200, Lauri Tirkkonen wrote:

> @@ -128,9 +127,6 @@ main(int argc, char *argv[])
>   case 'p':
>   docompare = dopreserve = 1;
>   break;
> - case 'S':
> - safecopy = 1;
> - break;
>   case 's':
>   dostrip = 1;
>   break;

I realized -S is used in many places throughout src (including in
xinstall/Makefile itself, and in bsd.lib.mk, bsd.prog.mk), so -S should
probably be accepted for compatibility until those -S's can be removed.
diff revised as such.

diff --git a/usr.bin/xinstall/install.1 b/usr.bin/xinstall/install.1
index fd5db0abc37..f7d8707ce6f 100644
--- a/usr.bin/xinstall/install.1
+++ b/usr.bin/xinstall/install.1
@@ -38,7 +38,7 @@
 .Nd install binaries
 .Sh SYNOPSIS
 .Nm install
-.Op Fl bCcDdFpSs
+.Op Fl bCcDdFps
 .Op Fl B Ar suffix
 .Op Fl f Ar flags
 .Op Fl g Ar group
@@ -139,17 +139,6 @@ Copy the file, as if the
 (compare and copy) option is specified,
 except if the target file doesn't already exist or is different,
 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.
 .It Fl s
 .Nm
 exec's the command
@@ -186,18 +175,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
diff --git a/usr.bin/xinstall/xinstall.c b/usr.bin/xinstall/xinstall.c
index c08d82eeed4..be7311b354a 100644
--- a/usr.bin/xinstall/xinstall.c
+++ b/usr.bin/xinstall/xinstall.c
@@ -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_int);
 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,6 @@ main(int argc, char *argv[])
  docompare = dopreserve = 1;
  break;
  case 'S':
- safecopy = 1;
  break;
  case 's':
  dostrip = 1;
@@ -148,17 +146,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 +259,30 @@ install(char *from_name, char *to_name, u_long fset, u_int flags)
  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 +332,13 @@ install(char *from_name, char *to_name, u_long fset, u_int flags)
  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 +348,7 @@ 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",
-    safecopy ? tempfile :to_name, strerror(errno));
+ warnx("%s: chflags: %s", tempfile, strerror(errno));
  }
 
  if (flags & USEFSYNC)
@@ -391,10 +358,10 @@ install(char *from_name, char *to_name, u_long fset, u_int flags)
  (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 just 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));
@@ -632,7 +599,7 @@ void
 usage(void)
 {
  (void)fprintf(stderr, "\
-usage: install [-bCcDdFpSs] [-B suffix] [-f flags] [-g group] [-m mode] [-o owner]\n       source ... target ...\n");
+usage: install [-bCcDdFps] [-B suffix] [-f flags] [-g group] [-m mode] [-o owner]\n       source ... target ...\n");
  exit(1);
  /* NOTREACHED */
 }
@@ -657,36 +624,6 @@ create_tempfile(char *path, char *temp, size_t 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));
-}
-
 /*
  * file_write()
  * Write/copy a file (during copy or archive extract). This routine knows
--
Lauri Tirkkonen | lotheac @ IRCnet

Reply | Threaded
Open this post in threaded view
|

Re: install(1) could fail due to race

Ted Unangst-6
In reply to this post by Lauri Tirkkonen-2
Lauri Tirkkonen wrote:
> 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.

> The below diff essentially removes the -S option and makes install
> always use temp files (ie. -S is always on), eliminating the race since
> rename(2) cannot fail like this.

I don't know. Presumably if there weren't any downside to safecopy, it would
already have been made the default. This doubles the number of synchronous
file operations.

You should probably fix your project to use -S instead.

Reply | Threaded
Open this post in threaded view
|

Re: install(1) could fail due to race

Lauri Tirkkonen-2
On Mon, Jan 07 2019 15:48:25 -0500, Ted Unangst wrote:

> Lauri Tirkkonen wrote:
> > 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.
>
> > The below diff essentially removes the -S option and makes install
> > always use temp files (ie. -S is always on), eliminating the race since
> > rename(2) cannot fail like this.
>
> I don't know. Presumably if there weren't any downside to safecopy, it would
> already have been made the default.

I had this thought, but could not think of any real downsides...

> This doubles the number of synchronous
> file operations.

Does it? Without safecopy, the operations performed are:

    unlink(targetfile);
    open(targetfile, O_CREAT|O_EXCL);
    write();
    fchmod();
    close();

with safecopy, they are:

    open(tempfile, O_CREAT|O_EXCL);
    write();
    fchmod();
    close();
    rename(tempfile, targetfile);

which to me seems identical in the number of file syscalls made.

--
Lauri Tirkkonen | lotheac @ IRCnet

Reply | Threaded
Open this post in threaded view
|

Re: install(1) could fail due to race

Todd C. Miller-3
The main difference with -S is that you end up with two copies of
the target file on disk for a short period of time.  That was a
bigger deal back when -S was added.  Today, it probably doesn't
matter.

 - todd

Reply | Threaded
Open this post in threaded view
|

Re: install(1) could fail due to race

Ted Unangst-6
In reply to this post by Lauri Tirkkonen-2
Lauri Tirkkonen wrote:

> On Mon, Jan 07 2019 15:48:25 -0500, Ted Unangst wrote:
> > Lauri Tirkkonen wrote:
> > > 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.
> >
> > > The below diff essentially removes the -S option and makes install
> > > always use temp files (ie. -S is always on), eliminating the race since
> > > rename(2) cannot fail like this.
> >
> > I don't know. Presumably if there weren't any downside to safecopy, it would
> > already have been made the default.
>
> I had this thought, but could not think of any real downsides...
>
> > This doubles the number of synchronous
> > file operations.
>
> Does it? Without safecopy, the operations performed are:
>
>     unlink(targetfile);
>     open(targetfile, O_CREAT|O_EXCL);
>     write();
>     fchmod();
>     close();
>
> with safecopy, they are:
>
>     open(tempfile, O_CREAT|O_EXCL);
>     write();
>     fchmod();
>     close();
>     rename(tempfile, targetfile);
>
> which to me seems identical in the number of file syscalls made.

oh, I think I forgot to count the unlink(). rename() within a directory is
about the same cost as unlink(), so the two cases do seem equal.


Reply | Threaded
Open this post in threaded view
|

Re: install(1) could fail due to race

Lauri Tirkkonen-2
On Mon, Jan 07 2019 16:32:31 -0500, Ted Unangst wrote:

> > > This doubles the number of synchronous
> > > file operations.
> >
> > Does it? Without safecopy, the operations performed are:
> >
> >     unlink(targetfile);
> >     open(targetfile, O_CREAT|O_EXCL);
> >     write();
> >     fchmod();
> >     close();
> >
> > with safecopy, they are:
> >
> >     open(tempfile, O_CREAT|O_EXCL);
> >     write();
> >     fchmod();
> >     close();
> >     rename(tempfile, targetfile);
> >
> > which to me seems identical in the number of file syscalls made.
>
> oh, I think I forgot to count the unlink(). rename() within a directory is
> about the same cost as unlink(), so the two cases do seem equal.

great, we're on the same page :)

here's an additional manpage adjustment on top, that I somehow missed
the first time around.

diff --git a/usr.bin/xinstall/install.1 b/usr.bin/xinstall/install.1
index f7d8707ce6f..64f99504752 100644
--- a/usr.bin/xinstall/install.1
+++ b/usr.bin/xinstall/install.1
@@ -101,7 +101,7 @@ Create directories.
 Missing parent directories are created as required.
 This option cannot be used with the
 .Fl B , b , C , c ,
-.Fl f , p , S ,
+.Fl f , p ,
 or
 .Fl s
 options.
@@ -198,9 +198,8 @@ The
 .Fl C ,
 .Fl D ,
 .Fl F ,
-.Fl p ,
 and
-.Fl S
+.Fl p
 flags are non-standard and should not be relied upon for portability.
 .Pp
 Temporary files may be left in the target directory if

--
Lauri Tirkkonen | lotheac @ IRCnet

Reply | Threaded
Open this post in threaded view
|

Re: install(1) could fail due to race

Christian Weisgerber
In reply to this post by Todd C. Miller-3
On 2019-01-07, "Todd C. Miller" <[hidden email]> wrote:

> The main difference with -S is that you end up with two copies of
> the target file on disk for a short period of time.

Oooh, that short period can be the time between disk flushes (30s?)
with softupdates.  I think that's one of the scenarios where you
can run out of disk space if you have softupdates enabled.

--
Christian "naddy" Weisgerber                          [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: install(1) could fail due to race

Todd C. Miller-3
On Mon, 07 Jan 2019 22:55:54 +0000, Christian Weisgerber wrote:

> Oooh, that short period can be the time between disk flushes (30s?)
> with softupdates.  I think that's one of the scenarios where you
> can run out of disk space if you have softupdates enabled.

Yes, I think I've had that happen in the (now rather distant) past.

 - todd

Reply | Threaded
Open this post in threaded view
|

Re: install(1) could fail due to race

Ted Unangst-6
Todd C. Miller wrote:
> On Mon, 07 Jan 2019 22:55:54 +0000, Christian Weisgerber wrote:
>
> > Oooh, that short period can be the time between disk flushes (30s?)
> > with softupdates.  I think that's one of the scenarios where you
> > can run out of disk space if you have softupdates enabled.
>
> Yes, I think I've had that happen in the (now rather distant) past.

Has anybody seen this recently? Many years ago I had a diff to fix it. Lost
the diff, but I remember it's not super complicated. I figured the problem
went away (due to less tight partitioning) and haven't seen it myself. If
people are still having to workaround it, I can cook something up.

Reply | Threaded
Open this post in threaded view
|

Re: install(1) could fail due to race

Theo de Raadt-2
Ted Unangst <[hidden email]> wrote:

> Todd C. Miller wrote:
> > On Mon, 07 Jan 2019 22:55:54 +0000, Christian Weisgerber wrote:
> >
> > > Oooh, that short period can be the time between disk flushes (30s?)
> > > with softupdates.  I think that's one of the scenarios where you
> > > can run out of disk space if you have softupdates enabled.
> >
> > Yes, I think I've had that happen in the (now rather distant) past.
>
> Has anybody seen this recently? Many years ago I had a diff to fix it. Lost
> the diff, but I remember it's not super complicated. I figured the problem
> went away (due to less tight partitioning) and haven't seen it myself. If
> people are still having to workaround it, I can cook something up.

Wait.  Stop.

Some architectures triggered this "delayed release of softdep
buffers" worse.  In order of danger: macppc, hppa, sgi, ...

To me, it seems to be related to "running servicing functions".

I don't think this needs a workaround.  I despise the word.  The proper bug
needs to be found, and fixed.  Not some workaround installed which seems
to fix the issue on machines people don't have, and golly gee it still remains
or surfaces in some other way.. and then OH WE KNOW THE REAL PROBLEM, here
is the real fix, and years later "someone forgot their broken workaround
still remains in the tree causing other effects".

Don't "workaround it".  Workaround it by fixing it.

I think macppc and hppa are teaching a lesson.  deep rm -rf on hppa is super
slow.  On macppc, softdep is very slow at releasing buffers.  Where are the MD
mistakes to cause this?

Reply | Threaded
Open this post in threaded view
|

Re: install(1) could fail due to race

Lauri Tirkkonen-2
In reply to this post by Todd C. Miller-3
On Mon, Jan 07 2019 14:27:29 -0700, Todd C. Miller wrote:
> The main difference with -S is that you end up with two copies of
> the target file on disk for a short period of time.  That was a
> bigger deal back when -S was added.  Today, it probably doesn't
> matter.

so, how about this patch then? I think the discussion about softupdates
is a little orthogonal.

--
Lauri Tirkkonen | lotheac @ IRCnet

Reply | Threaded
Open this post in threaded view
|

Re: install(1) could fail due to race

Ingo Schwarze
In reply to this post by Lauri Tirkkonen-2
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));
 }
 
 /*

Reply | Threaded
Open this post in threaded view
|

Re: install(1) could fail due to race

Lauri Tirkkonen-2
On Wed, Jan 16 2019 11:00:04 +0100, Ingo Schwarze wrote:

> 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.

Yes, you're right.

> > 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.

The second diff I sent removes the documentation, but keeps accepting
the option as a no-op just for that reason. I don't see much point in
documenting it since you cannot turn it off, but it's your call, of
course.

> > 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.

Right, that's why I said I think softupdates is orthogonal. Besides the
default mk files already use -S, so there should be no behavior change
for building base.

--
Lauri Tirkkonen | lotheac @ IRCnet