Subject: bug#8836: closed (Re: bug#8836: Acknowledgement (mktemp-related race condition in movemail))

From: [email-masked] (GNU bug Tracking System)
Date: Fri, 10 Jun 2011 17:52:02 +0000

Your bug report

#8836: mktemp-related race condition in movemail

which was filed against the emacs package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
8836: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=8836
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#8836: Acknowledgement (mktemp-related race condition in movemail)
From: Paul Eggert <[email-masked]>
To: [email-masked]
Date: Fri, 10 Jun 2011 10:50:58 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

I committed the fix in bzr 104555 on the trunk.

Subject: mktemp-related race condition in movemail
From: Paul Eggert <[email-masked]>
To: [email-masked]
Date: Fri, 10 Jun 2011 10:46:47 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

There is a race condition in lib-src/movemail.c, and some
related bugs.  I plan to commit the following patch.
This one has security implications, so I'm filing a bug
report to give others a bigger heads-up.

I found this one via GCC 4.6.0's static analysis.

* movemail.c: Fix race condition and related bugs.
(main) [!MAIL_USE_SYSTEM_LOCK]: Prefer mkstemp to mktemp, as this
fixes some race conditions.  Report mkstemp/mktemp errno rather
than a possibly-garbage errno.  Reinitialize the template each
time through the loop, as earlier mkstemp/mktemp calls could have
trashed it.  Pass 0600 (not 0666) to mktemp, for consistency
with mkstemp; the permissions don't matter anyway.
=== modified file 'lib-src/movemail.c'
--- lib-src/movemail.c	2011-04-16 21:20:25 +0000
+++ lib-src/movemail.c	2011-06-10 17:30:52 +0000
@@ -168,8 +168,9 @@
 #ifndef MAIL_USE_SYSTEM_LOCK
   struct stat st;
   int tem;
-  char *lockname, *p;
+  char *lockname;
   char *tempname;
+  size_t inname_dirlen;
   int desc;
 #endif /* not MAIL_USE_SYSTEM_LOCK */

@@ -298,26 +299,38 @@
 	 to [email-masked] so we can fix it.  */

       lockname = concat (inname, ".lock", "");
-      tempname = (char *) xmalloc (strlen (inname) + strlen ("EXXXXXX") + 1);
-      strcpy (tempname, inname);
-      p = tempname + strlen (tempname);
-      while (p != tempname && !IS_DIRECTORY_SEP (p[-1]))
-	p--;
-      *p = 0;
-      strcpy (p, "EXXXXXX");
-      mktemp (tempname);
-      unlink (tempname);
+      for (inname_dirlen = strlen (inname);
+	   inname_dirlen && !IS_DIRECTORY_SEP (inname[inname_dirlen - 1]);
+	   inname_dirlen--)
+	continue;
+      tempname = (char *) xmalloc (inname_dirlen + sizeof "EXXXXXX");

       while (1)
 	{
 	  /* Create the lock file, but not under the lock file name.  */
 	  /* Give up if cannot do that.  */
-	  desc = open (tempname, O_WRONLY | O_CREAT | O_EXCL, 0666);
+
+	  memcpy (tempname, inname, inname_dirlen);
+	  strcpy (tempname + inname_dirlen, "EXXXXXX");
+#ifdef HAVE_MKSTEMP
+	  desc = mkstemp (tempname);
+#else
+	  mktemp (tempname);
+	  if (!*tempname)
+	    desc = -1;
+	  else
+	    {
+	      unlink (tempname);
+	      desc = open (tempname, O_WRONLY | O_CREAT | O_EXCL, 0600);
+	    }
+#endif
 	  if (desc < 0)
 	    {
+	      int mkstemp_errno = errno;
 	      char *message = (char *) xmalloc (strlen (tempname) + 50);
 	      sprintf (message, "creating %s, which would become the lock file",
 		       tempname);
+	      errno = mkstemp_errno;
 	      pfatal_with_name (message);
 	    }
 	  close (desc);