Re: BSD mail.local has race condition - fix

[travis () EvTech com (Travis Hassloch x231)]

This fix is relative to:
static char sccsid[] = "from: @(#)mail.local.c        5.6 (Berkeley) 6/19/91";
as modified into:
  "$Id: mail.local.c,v 1.1.1.1 1995/10/18 08:43:19 deraadt Exp $"

The problem I described does _not_ occur in BSD/OS.

I have cc'ed everyone I think might want to integrate it ('cept OpenBSD,
Theo already has a copy); the rest is up to you.  I believe it to fix
the problem but have not even compiled it as I lack a BSD environment here.

--- mail.local.c~       Tue Jul 16 19:00:07 1996
+++ mail.local.c        Tue Jul 16 21:02:15 1996
@@ -168,9 +168,9 @@
        char *name;
        int lockfile;
 {
-       struct stat sb;
+       struct stat sb, fsb;
        struct passwd *pw;
-       int created, mbfd, nr, nw, off, rval=0, lfd=-1;
+       int mbfd=-1, nr, nw, off, rval=1, lfd=-1;
        char biffmsg[100], buf[8*1024], path[MAXPATHLEN], lpath[MAXPATHLEN];
        off_t curoff;

@@ -194,59 +194,94 @@
                        return(1);
                }
        }
-
-       if (!(created = lstat(path, &sb)) &&
-           (sb.st_nlink != 1 || S_ISLNK(sb.st_mode))) {
-               err(NOTFATAL, "%s: linked file", path);
-               return(1);
-       }
-       if((mbfd = open(path, O_APPEND|O_WRONLY|O_EXLOCK,
-           S_IRUSR|S_IWUSR)) < 0) {
+       /* after this point, always exit via bad to remove lockfile */
+retry:
+       if (lstat(path, &sb)) {
+               if (errno != ENOENT) {
+                       err(NOTFATAL, "%s: %s", path, strerror(errno));
+                       goto bad;
+               }
                if ((mbfd = open(path, O_APPEND|O_CREAT|O_WRONLY|O_EXLOCK,
-                   S_IRUSR|S_IWUSR)) < 0) {
-               err(NOTFATAL, "%s: %s", path, strerror(errno));
-               return(1);
+                    S_IRUSR|S_IWUSR)) < 0) {
+                       if (errno == EEXIST) {
+                               /* file appeared since lstat */
+                               goto retry;
+                       }
+                       else {
+                               err(NOTFATAL, "%s: %s", path, strerror(errno));
+                               goto bad;
+                       }
+               }
+       /*
+        * Set the owner and group.  Historically, binmail repeated this at
+        * each mail delivery.  We no longer do this, assuming that if the
+        * ownership or permissions were changed there was a reason for doing
+        * so.
+        */
+               if (fchown(mbfd, pw->pw_uid, pw->pw_gid) < 0) {
+                       err(NOTFATAL, "chown %u:%u: %s",
+                           pw->pw_uid, pw->pw_gid, name);
+                       goto bad;
+               }
        }
+       else {
+               if (sb.st_nlink != 1 || S_ISLNK(sb.st_mode)) {
+                       err(NOTFATAL, "%s: linked file", path);
+                       goto bad;
+               }
+               if ((mbfd = open(path, O_APPEND|O_WRONLY|O_EXLOCK,
+                   S_IRUSR|S_IWUSR)) < 0) {
+                       err(NOTFATAL, "%s: %s", path, strerror(errno));
+                       goto bad;
+               }
+               if (fstat(mbfd, &fsb)) {
+                       /* relating error to path may be bad style */
+                       err(NOTFATAL, "%s: %s", path, strerror(errno));
+                       goto bad;
+               }
+               if (sb.st_dev != fsb.st_dev || sb.st_ino != fsb.st_ino) {
+                       err(NOTFATAL, "%s: changed after open", path);
+                       goto bad;
+               }
+               /* paranoia? */
+               if (fsb.st_nlink != 1 || S_ISLNK(fsb.st_mode)) {
+                       err(NOTFATAL, "%s: linked file", path);
+                       goto bad;
+               }
        }

        curoff = lseek(mbfd, 0, SEEK_END);
        (void)sprintf(biffmsg, "%s@%qd\n", name, curoff);
        if (lseek(fd, 0, SEEK_SET) == (off_t)-1) {
                err(FATAL, "temporary file: %s", strerror(errno));
-               rval = 1;
                goto bad;
        }

+       /* This section (to trunc) is ugly */
        while ((nr = read(fd, buf, sizeof(buf))) > 0)
                for (off = 0; off < nr;  off += nw)
                        if ((nw = write(mbfd, buf + off, nr - off)) < 0) {
                                err(NOTFATAL, "%s: %s", path, strerror(errno));
                                goto trunc;
                        }
-       if (nr < 0) {
+       if (!nr) {
+               rval = 0;
+       }
+       else {
                err(FATAL, "temporary file: %s", strerror(errno));
 trunc:         (void)ftruncate(mbfd, curoff);
-               rval = 1;
        }

-       /*
-        * Set the owner and group.  Historically, binmail repeated this at
-        * each mail delivery.  We no longer do this, assuming that if the
-        * ownership or permissions were changed there was a reason for doing
-        * so.
-        */
 bad:
-       if(lockfile) {
-               if(lfd >= 0) {
-                       unlink(lpath);
-                       close(lfd);
-               }
+       if(lfd >= 0) {
+               unlink(lpath);
+               close(lfd);
        }
-       if (created)
-               (void)fchown(mbfd, pw->pw_uid, pw->pw_gid);

-       (void)fsync(mbfd);              /* Don't wait for update. */
-       (void)close(mbfd);              /* Implicit unlock. */
+       if (mbfd >= 0) {
+               (void)fsync(mbfd);              /* Don't wait for update. */
+               (void)close(mbfd);              /* Implicit unlock. */
+       }

        if (!rval)
                notifybiff(biffmsg);
--
Travis Hassloch, Electronic Blacksmith | P=NP if (P=0 or N=1)
There is a fine line between an email message and it's signature.