Re: PR/35619 CVS commit: src/libexec/makewhatis

[David Holland <dholland-bugs%netbsd.org@localhost>]

The following reply was made to PR bin/35619; it has been noted by GNATS.

From: David Holland <dholland-bugs%netbsd.org@localhost>
To: gnats-bugs%netbsd.org@localhost, netbsd-bugs%netbsd.org@localhost, 
uwe%NetBSD.org@localhost
Cc: 
Subject: Re: PR/35619 CVS commit: src/libexec/makewhatis
Date: Tue, 27 May 2008 02:21:19 +0000

 On Tue, May 27, 2008 at 01:45:01AM +0000, David A. Holland wrote:
  > Urgh, part of another patch for PR 35619 (which is for after the freeze)
  > [...]
 
 Here's that patch, which I'll commit after the freeze is over. This
 should thoroughly take care of the race condition.
 
 diff -r f75028e978ef makewhatis.c
 --- a/makewhatis.c     Mon May 26 21:52:34 2008 -0400
 +++ b/makewhatis.c     Mon May 26 22:24:53 2008 -0400
 @@ -248,6 +248,8 @@
        whatis  *dest;
        FILE    *out;
        size_t  sdoff, sdlen;
 +      int     outfd;
 +      struct stat st_before, st_after;
  
        if ((fts = fts_open(manpath, FTS_LOGICAL, NULL)) == NULL)
                err(EXIT_FAILURE, "Cannot open `%s'", *manpath);
 @@ -328,15 +330,74 @@
        if (chdir(manpath[0]) == -1)
                err(EXIT_FAILURE, "Cannot change dir to `%s'", manpath[0]);
  
 -      (void)unlink(whatisdb_new);
 -      if ((out = fopen(whatisdb_new, "w")) == NULL)
 +      /*
 +       * makewhatis runs unattended, so it needs to be able to
 +       * recover if the last run crashed out. Therefore, if
 +       * whatisdb_new exists and is more than (arbitrarily) sixteen
 +       * hours old, nuke it. If it exists but is not so old, refuse
 +       * to run until it's cleaned up, in case another makewhatis is
 +       * already running. Also, open the output with O_EXCL to make
 +       * sure we get our own, in case two copies start exactly at
 +       * once. (Unlikely? Maybe, maybe not, if two copies of cron
 +       * end up running.)
 +       *
 +       * Similarly, before renaming the file after we finish writing
 +       * to it, make sure it's still the same file we opened. This
 +       * can't be completely race-free, but getting caught by it
 +       * would require an unexplained sixteen-hour-or-more lag
 +       * between the last mtime update when we wrote to it and when
 +       * we get to the stat call *and* another makewhatis starting
 +       * out to write at exactly the wrong moment. Not impossible,
 +       * but not likely enough to worry about.
 +       *
 +       * This is maybe unnecessarily elaborate, but generating
 +       * corrupted output isn't so good either.
 +       */
 +
 +      if (stat(whatisdb_new, &st_before) == 0) {
 +              if (st_before.st_mtime - time(NULL) > 16*60*60) {
 +                      /* Don't complain if someone else just removed it. */
 +                      if (unlink(whatisdb_new) == -1 && errno != ENOENT) {
 +                              err(EXIT_FAILURE, "Could not remove `%s'",
 +                                  whatisdb_new);
 +                      } else {
 +                              warnx("Removed stale `%s'", whatisdb_new);
 +                      }
 +              } else {
 +                      errx(EXIT_FAILURE, "The file `%s' already exists "
 +                          "-- am I already running?", whatisdb_new);
 +              }
 +      } else if (errno != ENOENT) {
 +              /* Something unexpected happened. */
 +              err(EXIT_FAILURE, "Cannot stat `%s'", whatisdb_new);
 +      }
 +
 +      outfd = open(whatisdb_new, O_WRONLY|O_CREAT|O_EXCL,
 +          S_IRUSR|S_IRGRP|S_IROTH);
 +      if (outfd < 0)
                err(EXIT_FAILURE, "Cannot open `%s'", whatisdb_new);
  
 +      if (fstat(outfd, &st_before) == -1)
 +              err(EXIT_FAILURE, "Cannot fstat `%s'", whatisdb_new);
 +
 +      if ((out = fdopen(outfd, "w")) == NULL)
 +              err(EXIT_FAILURE, "Cannot fdopen `%s'", whatisdb_new);
 +
        dumpwhatis(out, dest);
        if (fchmod(fileno(out), S_IRUSR|S_IRGRP|S_IROTH) == -1)
                err(EXIT_FAILURE, "Cannot chmod `%s'", whatisdb_new);
        if (fclose(out) != 0)
                err(EXIT_FAILURE, "Cannot close `%s'", whatisdb_new);
 +
 +      if (stat(whatisdb_new, &st_after) == -1)
 +              err(EXIT_FAILURE, "Cannot stat `%s' (after writing)",
 +                  whatisdb_new);
 +
 +      if (st_before.st_dev != st_after.st_dev ||
 +          st_before.st_ino != st_after.st_ino) {
 +              errx(EXIT_FAILURE, "The file `%s' changed under me; giving up",
 +                  whatisdb_new);
 +      }
  
        if (rename(whatisdb_new, whatisdb) == -1)
                err(EXIT_FAILURE, "Could not rename `%s' to `%s'",
 
 
 
 -- 
 David A. Holland
 dholland%netbsd.org@localhost