GNU bug report logs - #8836
mktemp-related race condition in movemail

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 8836 in the body.
You can then email your comments to 8836 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: bug-gnu-emacs <at> gnu.org
Subject: mktemp-related race condition in movemail
Date: Fri, 10 Jun 2011 10:46:47 -0700

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 bug-gnu-emacs <at> prep.ai.mit.edu 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);

Message #10 received at 8836-done <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: 8836-done <at> debbugs.gnu.org
Subject: Re: bug#8836: Acknowledgement (mktemp-related race condition in
	movemail)
Date: Fri, 10 Jun 2011 10:50:58 -0700

I committed the fix in bzr 104555 on the trunk.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.