GNU bug report logs - #10403
epg--make-temp-file permissions race condition fixes

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 10403 in the body.
You can then email your comments to 10403 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: epg--make-temp-file permissions race condition fixes
Date: Thu, 29 Dec 2011 14:22:56 -0800

Tags: patch

=== modified file 'lisp/ChangeLog'
--- lisp/ChangeLog	2011-12-29 21:55:33 +0000
+++ lisp/ChangeLog	2011-12-29 22:08:29 +0000
@@ -1,5 +1,8 @@
 2011-12-29  Paul Eggert  <eggert <at> cs.ucla.edu>
 
+	* epg.el (epg--make-temp-file): Avoid permission race conditions
+	when creating temporary directories and files on older Emacs.
+
 	* files.el (move-file-to-trash): Preserve default file modes on error.
 	(Bug#10401)
 

=== modified file 'lisp/epg.el'
--- lisp/epg.el	2011-11-23 07:03:56 +0000
+++ lisp/epg.el	2011-12-29 22:08:29 +0000
@@ -1951,14 +1951,16 @@
 of PREFIX, and expanding against `temporary-file-directory' if necessary),
 is guaranteed to point to a newly created empty file.
 You can then use `write-region' to write new data into the file."
-      (let (tempdir tempfile)
+      (let (tempdir tempfile orig-modes)
 	(setq prefix (expand-file-name prefix
 				       (if (featurep 'xemacs)
 					   (temp-directory)
 					 temporary-file-directory)))
+	(setq orig-modes (default-file-modes))
 	(unwind-protect
 	    (let (file)
 	      ;; First, create a temporary directory.
+	      (set-default-file-modes #o700)
 	      (while (condition-case ()
 			 (progn
 			   (setq tempdir (make-temp-name
@@ -1969,14 +1971,12 @@
 			   (make-directory tempdir))
 		       ;; let's try again.
 		       (file-already-exists t)))
-	      (set-file-modes tempdir 448)
 	      ;; Second, create a temporary file in the tempdir.
 	      ;; There *is* a race condition between `make-temp-name'
 	      ;; and `write-region', but we don't care it since we are
 	      ;; in a private directory now.
 	      (setq tempfile (make-temp-name (concat tempdir "/EMU")))
 	      (write-region "" nil tempfile nil 'silent)
-	      (set-file-modes tempfile 384)
 	      ;; Finally, make a hard-link from the tempfile.
 	      (while (condition-case ()
 			 (progn
@@ -1986,6 +1986,7 @@
 		       ;; let's try again.
 		       (file-already-exists t)))
 	      file)
+	  (set-default-file-modes orig-modes)
 	  ;; Cleanup the tempfile.
 	  (and tempfile
 	       (file-exists-p tempfile)

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

From: Chong Yidong <cyd <at> gnu.org>
To: Daiki Ueno <ueno <at> unixuser.org>
Cc: 10403 <at> debbugs.gnu.org, Paul Eggert <eggert <at> cs.ucla.edu>
Subject: Re: bug#10403: epg--make-temp-file permissions race condition fixes
Date: Sat, 07 Jan 2012 15:12:25 +0800

Could you review the patch posted by Paul?  Thanks.


Paul Eggert <eggert <at> cs.ucla.edu> writes:

> === modified file 'lisp/ChangeLog'
> --- lisp/ChangeLog	2011-12-29 21:55:33 +0000
> +++ lisp/ChangeLog	2011-12-29 22:08:29 +0000
> @@ -1,5 +1,8 @@
>  2011-12-29  Paul Eggert  <eggert <at> cs.ucla.edu>
>  
> +	* epg.el (epg--make-temp-file): Avoid permission race conditions
> +	when creating temporary directories and files on older Emacs.
> +
>  	* files.el (move-file-to-trash): Preserve default file modes on error.
>  	(Bug#10401)
>  
>
> === modified file 'lisp/epg.el'
> --- lisp/epg.el	2011-11-23 07:03:56 +0000
> +++ lisp/epg.el	2011-12-29 22:08:29 +0000
> @@ -1951,14 +1951,16 @@
>  of PREFIX, and expanding against `temporary-file-directory' if necessary),
>  is guaranteed to point to a newly created empty file.
>  You can then use `write-region' to write new data into the file."
> -      (let (tempdir tempfile)
> +      (let (tempdir tempfile orig-modes)
>  	(setq prefix (expand-file-name prefix
>  				       (if (featurep 'xemacs)
>  					   (temp-directory)
>  					 temporary-file-directory)))
> +	(setq orig-modes (default-file-modes))
>  	(unwind-protect
>  	    (let (file)
>  	      ;; First, create a temporary directory.
> +	      (set-default-file-modes #o700)
>  	      (while (condition-case ()
>  			 (progn
>  			   (setq tempdir (make-temp-name
> @@ -1969,14 +1971,12 @@
>  			   (make-directory tempdir))
>  		       ;; let's try again.
>  		       (file-already-exists t)))
> -	      (set-file-modes tempdir 448)
>  	      ;; Second, create a temporary file in the tempdir.
>  	      ;; There *is* a race condition between `make-temp-name'
>  	      ;; and `write-region', but we don't care it since we are
>  	      ;; in a private directory now.
>  	      (setq tempfile (make-temp-name (concat tempdir "/EMU")))
>  	      (write-region "" nil tempfile nil 'silent)
> -	      (set-file-modes tempfile 384)
>  	      ;; Finally, make a hard-link from the tempfile.
>  	      (while (condition-case ()
>  			 (progn
> @@ -1986,6 +1986,7 @@
>  		       ;; let's try again.
>  		       (file-already-exists t)))
>  	      file)
> +	  (set-default-file-modes orig-modes)
>  	  ;; Cleanup the tempfile.
>  	  (and tempfile
>  	       (file-exists-p tempfile)

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Daiki Ueno <ueno <at> unixuser.org>
Cc: 10403 <at> debbugs.gnu.org, Paul Eggert <eggert <at> cs.ucla.edu>,
	Chong Yidong <cyd <at> gnu.org>
Subject: Re: bug#10403: epg--make-temp-file permissions race condition fixes
Date: Sat, 07 Jan 2012 13:19:26 -0500

> Could you review the patch posted by Paul?  Thanks.
> Paul Eggert <eggert <at> cs.ucla.edu> writes:

BTW, why copy the code from poe.el rather than from Emacs itself?
Also, why use `eval-and-compile' since the function is not used
during compilation?
I think what you wanted to do instead is

(defalias (if (fboundp 'make-temp-file) 'make-temp-file
            (lambda (...) ...the copied fallback code...)))


-- Stefan

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

From: Daiki Ueno <ueno <at> unixuser.org>
To: Chong Yidong <cyd <at> gnu.org>
Cc: 10403 <at> debbugs.gnu.org, Paul Eggert <eggert <at> cs.ucla.edu>
Subject: Re: bug#10403: epg--make-temp-file permissions race condition fixes
Date: Tue, 10 Jan 2012 11:02:43 +0900

Chong Yidong <cyd <at> gnu.org> writes:

> Could you review the patch posted by Paul?  Thanks.

Thanks.  It looks good to me.
I don't quite remember the intent of that permission logic though...

For the record, the original code is:
http://cvs.m17n.org/viewcvs/root/apel/poe.el?revision=1.90&view=markup
(the copyright has been assigned to FSF just in case)

Regards,
-- 
Daiki Ueno

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

From: Daiki Ueno <ueno <at> unixuser.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 10403 <at> debbugs.gnu.org, Paul Eggert <eggert <at> cs.ucla.edu>,
	Chong Yidong <cyd <at> gnu.org>
Subject: Re: bug#10403: epg--make-temp-file permissions race condition fixes
Date: Tue, 10 Jan 2012 11:57:58 +0900

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:

>> Could you review the patch posted by Paul?  Thanks.
>> Paul Eggert <eggert <at> cs.ucla.edu> writes:
>
> BTW, why copy the code from poe.el rather than from Emacs itself?

Maybe because poe.el's code seems to provide better compatibility with
ancient Emacs and XEmacs.  For example, in the Emacs definition of
make-temp-file, I see MUSTBENEW arg of write-region is used, which is
missing in XEmacs 21.4.

> Also, why use `eval-and-compile' since the function is not used
> during compilation?

Right.  I think we could get rid of such a eval-and-compile from several
places.

Regards,
-- 
Daiki Ueno

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

From: Chong Yidong <cyd <at> gnu.org>
To: Daiki Ueno <ueno <at> unixuser.org>
Cc: 10403 <at> debbugs.gnu.org, Paul Eggert <eggert <at> cs.ucla.edu>
Subject: Re: bug#10403: epg--make-temp-file permissions race condition fixes
Date: Sat, 14 Jan 2012 15:09:37 +0800

Daiki Ueno <ueno <at> unixuser.org> writes:

> Chong Yidong <cyd <at> gnu.org> writes:
>
>> Could you review the patch posted by Paul?  Thanks.
>
> Thanks.  It looks good to me.
> I don't quite remember the intent of that permission logic though...

Thanks, committed.  Closing the bug.

Previous Next

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