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

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

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