GNU bug report logs - #3911
get-free-disk-space race condition on default-directory test

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 3911 in the body.
You can then email your comments to 3911 AT debbugs.gnu.org in the normal way.

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

Message #5 received at submit <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Kevin Ryde <user42 <at> zip.com.au>
To: emacs-pretest-bug <at> gnu.org
Subject: 23.1.50; get-free-disk-space relative to non-existent default-directory
Date: Fri, 24 Jul 2009 08:19:54 +1000

[Message part 1 (text/plain, inline)]

On a non-existent directory get-free-disk-space returns nil, eg.

    (get-free-disk-space "/no/such/dir")
    => nil

But if it's done by a relative path from a default-directory which
doesn't exist, then the answer is the space in your home directory

    (let ((default-directory "/no/such/dir"))
      (get-free-disk-space "."))
    => 3705637

I hoped the two answers would be the same.

This is with the "df"-running code in get-free-disk-space, ie. no
`file-system-info' function.

Perhaps absolutize per below with an inserted setq, the diff being just
re-indent.

(I still think the use of the home dir there is either complicated and
unnecessary, or complicated and necessary everywhere.  Or maybe better
call-process could not attempt a chdir at all if a caller doesn't care
about the current directory.  But leaving that unchanged as yet.)

2009-07-24  Kevin Ryde  <user42 <at> zip.com.au>

	* files.el (get-free-disk-space): Absolutize the dir before
	changing default-directory, so as not to answer about a different
	directory.

[files.el.disk-space-absolutize.diff (text/x-diff, inline)]

*** files.el	24 Jul 2009 07:35:37 +1000	1.1063
--- files.el	24 Jul 2009 08:07:01 +1000	
***************
*** 5394,5410 ****
        (save-match-data
  	(with-temp-buffer
  	  (when (and directory-free-space-program
! 		     (let ((default-directory
! 			     (if (and (not (file-remote-p default-directory))
! 				      (file-directory-p default-directory)
! 				      (file-readable-p default-directory))
! 				 default-directory
! 			       (expand-file-name "~/"))))
! 		       (eq (call-process directory-free-space-program
! 					 nil t nil
! 					 directory-free-space-args
! 					 dir)
! 			   0)))
  	    ;; Usual format is a header line followed by a line of
  	    ;; numbers.
  	    (goto-char (point-min))
--- 5394,5413 ----
        (save-match-data
  	(with-temp-buffer
  	  (when (and directory-free-space-program
!                      (progn
!                        ;; absolutize before changing default-directory
!                        (setq dir (expand-file-name dir))
!                        (let ((default-directory
!                                (if (and (not (file-remote-p default-directory))
!                                         (file-directory-p default-directory)
!                                         (file-readable-p default-directory))
!                                    default-directory
!                                  (expand-file-name "~/"))))
!                          (eq (call-process directory-free-space-program
!                                            nil t nil
!                                            directory-free-space-args
!                                            dir)
!                              0))))
  	    ;; Usual format is a header line followed by a line of
  	    ;; numbers.
  	    (goto-char (point-min))

[Message part 3 (text/plain, inline)]

In GNU Emacs 23.1.50.4 (i586-pc-linux-gnu, GTK+ Version 2.16.4)
 of 2009-07-24 on blah.blah
configured using `configure  'CFLAGS=-O -g' '--prefix=/down/emacs/b/inst' '--with-x-toolkit=gtk''

Important settings:
  value of $LC_ALL: nil
  value of $LC_COLLATE: nil
  value of $LC_CTYPE: nil
  value of $LC_MESSAGES: nil
  value of $LC_MONETARY: nil
  value of $LC_NUMERIC: nil
  value of $LC_TIME: nil
  value of $LANG: en_AU
  value of $XMODIFIERS: nil
  locale-coding-system: iso-latin-1-unix
  default-enable-multibyte-characters: t

Message #10 received at 3911-done <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Chong Yidong <cyd <at> stupidchicken.com>
To: Kevin Ryde <user42 <at> zip.com.au>
Cc: 2631-done <at> debbugs.gnu.org, 3911-done <at> debbugs.gnu.org
Subject: Re: get-free-disk-space when default-directory doesn't exist
Date: Sat, 15 Aug 2009 20:34:52 -0400

> What is it that's not good about "/"?  It doesn't have to be readable
> or anything I don't think, just somewhere capable of a chdir()...  If
> both default-directory and home directory both don't exist
> get-free-disk-space still gets an error

You're right; it's probably better to fall back on / instead of ~.  I've
just checked in your suggested change.

> (let ((default-directory "/no/such/dir"))
>   (get-free-disk-space "."))
> => 3705637

I checked in your suggestion to do expand-file-name before changing the
default-directory, as well.  Thanks.

Message #15 received at 3911 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Kevin Ryde <user42 <at> zip.com.au>
To: Chong Yidong <cyd <at> stupidchicken.com>
Cc: 3911 <at> debbugs.gnu.org
Subject: Re: get-free-disk-space when default-directory doesn't exist
Date: Tue, 18 Aug 2009 10:23:08 +1000

[Message part 1 (text/plain, inline)]

Chong Yidong <cyd <at> stupidchicken.com> writes:
>
> You're right; it's probably better to fall back on / instead of ~.

I think the test for file-remote-p is covered by the first line of the
func so it's not needed in the dir condition bit, per below.


I still think "/" might as well be used unconditionally though.  The
reason I'm still banging on about that is that there's probably 3 or 4
other places in emacs which also don't care about the current directory
when spawning a program.  They would make use of the same idiom,
whatever it might best be.  (I arrived at this first when some of my own
code bombed in a non-existent default-directory.  I've seen it in other
people's code too.)

[files.el.disk-space-noremote.diff (text/x-diff, inline)]

*** files.el	18 Aug 2009 08:51:19 +1000	1.1068
--- files.el	18 Aug 2009 10:12:22 +1000	
***************
*** 5400,5407 ****
  		     ;; not exist (Bug#2631, Bug#3911).
  		     (let ((default-directory default-directory))
  		       (setq dir (expand-file-name dir))
! 		       (unless (and (not (file-remote-p default-directory))
! 				    (file-directory-p default-directory)
  				    (file-readable-p default-directory))
  			 (setq default-directory "/"))
  		       (eq (call-process directory-free-space-program
--- 5400,5406 ----
  		     ;; not exist (Bug#2631, Bug#3911).
  		     (let ((default-directory default-directory))
  		       (setq dir (expand-file-name dir))
! 		       (unless (and (file-directory-p default-directory)
  				    (file-readable-p default-directory))
  			 (setq default-directory "/"))
  		       (eq (call-process directory-free-space-program

Message #26 received at 3911 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Chong Yidong <cyd <at> stupidchicken.com>
To: Kevin Ryde <user42 <at> zip.com.au>
Cc: 3911 <at> debbugs.gnu.org
Subject: Re: get-free-disk-space when default-directory doesn't exist
Date: Mon, 17 Aug 2009 21:02:27 -0400

Kevin Ryde <user42 <at> zip.com.au> writes:

> I think the test for file-remote-p is covered by the first line of the
> func so it's not needed in the dir condition bit, per below.

But that applies to DIR, not default-directory.

> I still think "/" might as well be used unconditionally though.

Principle of least surprise.  If we don't have to change the default
directory, let's not, even if it seems harmless (because we can never be
sure of every eventuality).

Message #31 received at 3911 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Kevin Ryde <user42 <at> zip.com.au>
To: Chong Yidong <cyd <at> stupidchicken.com>
Cc: 3911 <at> debbugs.gnu.org
Subject: Re: get-free-disk-space when default-directory doesn't exist
Date: Thu, 20 Aug 2009 07:43:32 +1000

Chong Yidong <cyd <at> stupidchicken.com> writes:
>
> But that applies to DIR, not default-directory.

Oops, I confused myself thinking the given dir could be used there.

> Principle of least surprise.  If we don't have to change the default
> directory, let's not,

Err, well, I think the unhappy surprise may be if an operation about one
directory depends on what's happening in another directory ...

Message #36 received at 3911 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Kevin Ryde <user42 <at> zip.com.au>
To: control <at> bugs.debian.org, 3911 <at> debbugs.gnu.org
Cc: Chong Yidong <cyd <at> stupidchicken.com>
Subject: Re: get-free-disk-space when default-directory doesn't exist
Date: Thu, 20 Aug 2009 08:03:42 +1000

[Message part 1 (text/plain, inline)]

reopen 3911
retitle 3911 get-free-disk-space race condition on default-directory test
tags 3911 +patch
thanks

If the default-directory is flicking in and out of existence then the
code in get-free-disk-space which chooses between default-directory and
"/" can suffer a race condition.  default-directory can exist for the
test, but then be gone by the time call-process tries to use it.

I can provoke this on my pc with one emacs making a directory flicker,

    (while t
      (make-directory "/tmp/somedir")
      (delete-directory "/tmp/somedir"))

And in another emacs a loop

    (while t
      (let ((default-directory "/tmp/somedir/"))
        (get-free-disk-space "/")))

The latter loop errors out after 5 or 10 minutes.  I hoped that asking
get-free-disk-space about the space on some directory would not depend
on the value of default-directory.

The problem can be provoked much quicker with a (sit-for 0.1) between
the test and the call-process.  Otherwise I presume it requires a random
context switch just at the inopportune time.


The perils of "test before use" come up in shell scripts often (because
it's hard to do better), and in sloppy perl code.  Alas the tendency is
to work most of the time, but lack robustness in special circumstances.

I suppose the rule for call-process would be: if the operation is not
related to default-directory then go to "/" so as to be independent of
default-directory's existence, or bouncing in and out of existence.  I
expect that's why other places have "/".  It sounds fairly simple, dunno
if that's too obvious to have a crib in the manual (under "Subprocess
Creation").

At any rate, umm, third time lucky with what I posted first ...


2009-08-20  Kevin Ryde  <user42 <at> zip.com.au>

	* files.el (get-free-disk-space): Bind default-directory to "/", to be
	independent of default-directory existence.  (Cannot test usability of
	default-directory first since it may disappear in between the test and
	the call-process.)

[files.el.disk-space-root-2.diff (text/x-diff, inline)]

*** files.el	18 Aug 2009 08:51:19 +1000	1.1068
--- files.el	20 Aug 2009 08:03:13 +1000	
***************
*** 5398,5409 ****
  	  (when (and directory-free-space-program
  		     ;; Avoid failure if the default directory does
  		     ;; not exist (Bug#2631, Bug#3911).
! 		     (let ((default-directory default-directory))
! 		       (setq dir (expand-file-name dir))
! 		       (unless (and (not (file-remote-p default-directory))
! 				    (file-directory-p default-directory)
! 				    (file-readable-p default-directory))
! 			 (setq default-directory "/"))
  		       (eq (call-process directory-free-space-program
  					 nil t nil
  					 directory-free-space-args
--- 5398,5405 ----
  	  (when (and directory-free-space-program
  		     ;; Avoid failure if the default directory does
  		     ;; not exist (Bug#2631, Bug#3911).
! 		     (setq dir (expand-file-name dir))
! 		     (let ((default-directory "/"))
  		       (eq (call-process directory-free-space-program
  					 nil t nil
  					 directory-free-space-args

Message #41 received at 3911 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Kevin Ryde <user42 <at> zip.com.au>
Cc: 3911 <at> debbugs.gnu.org, control <at> bugs.debian.org,
        Chong Yidong <cyd <at> stupidchicken.com>
Subject: Re: bug#3911: get-free-disk-space when default-directory doesn't exist
Date: Fri, 21 Aug 2009 23:57:20 -0400

How 'bout this patch instead (which could even work on Tramp
directories)?


        Stefan


--- files.el.~1.1069.~	2009-08-21 23:47:30.000000000 -0400
+++ files.el	2009-08-21 23:56:57.000000000 -0400
@@ -782,9 +782,12 @@
 ;;       nil)))
 
 (defun locate-dominating-file (file name)
-  "Look up the directory hierarchy from FILE for a file named NAME.
+  "Look up the directory hierarchy from FILE for a directory containing NAME.
 Stop at the first parent directory containing a file NAME,
-and return the directory.  Return nil if not found."
+and return the directory.  Return nil if not found.
+Instead of a string, NAME can also be a predicate taking one argument
+\(a directory) and returning a non-nil value if that directory is the one for
+which we're looking."
   ;; We used to use the above locate-dominating-files code, but the
   ;; directory-files call is very costly, so we're much better off doing
   ;; multiple calls using the code in here.
@@ -812,13 +815,15 @@
                     ;;   (setq user (nth 2 (file-attributes file)))
                     ;;   (and prev-user (not (equal user prev-user))))
                     (string-match locate-dominating-stop-dir-regexp file)))
-      (setq try (file-exists-p (expand-file-name name file)))
+      (setq try (if (stringp name)
+                    (file-exists-p (expand-file-name name file))
+                  (funcall name file)))
       (cond (try (setq root file))
             ((equal file (setq prev-file file
                                file (file-name-directory
                                      (directory-file-name file))))
              (setq file nil))))
-    root))
+    (file-name-as-directory root)))
 
 
 (defun executable-find (command)
@@ -5398,21 +5403,18 @@
 	(let ((fsinfo (file-system-info dir)))
 	  (if fsinfo
 	      (format "%.0f" (/ (nth 2 fsinfo) 1024))))
+      (setq dir (expand-file-name dir))
       (save-match-data
 	(with-temp-buffer
 	  (when (and directory-free-space-program
 		     ;; Avoid failure if the default directory does
 		     ;; not exist (Bug#2631, Bug#3911).
-		     (let ((default-directory default-directory))
-		       (setq dir (expand-file-name dir))
-		       (unless (and (not (file-remote-p default-directory))
-				    (file-directory-p default-directory)
-				    (file-readable-p default-directory))
-			 (setq default-directory "/"))
+		     (let ((default-directory
+                             (locate-dominating-file dir 'file-directory-p)))
 		       (eq (call-process directory-free-space-program
 					 nil t nil
 					 directory-free-space-args
-					 dir)
+					 (file-relative-name dir))
 			   0)))
 	    ;; Usual format is a header line followed by a line of
 	    ;; numbers.

Message #46 received at 3911 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Kevin Ryde <user42 <at> zip.com.au>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 3911 <at> debbugs.gnu.org, Chong Yidong <cyd <at> stupidchicken.com>
Subject: Re: bug#3911: get-free-disk-space when default-directory doesn't exist
Date: Tue, 25 Aug 2009 09:45:37 +1000

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>
> How 'bout this patch instead (which could even work on Tramp
> directories)?

I don't think so.  It's still a race condition in between choosing an
apparently existent directory and then making use of that directory,
isn't it?  I think "/" is the only place certain to exist and not go
away.  It's also the simplest approach.

`sha1-string-external' is another place this comes up.  In that case the
operation is completely unrelated to any directory, and so really should
go to "/" so as to be independent of default-directory or whether that
dir is coming or going.

Message #51 received at 3911 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
To: Kevin Ryde <user42 <at> zip.com.au>
Cc: 3911 <at> debbugs.gnu.org, Chong Yidong <cyd <at> stupidchicken.com>
Subject: Re: bug#3911: get-free-disk-space when default-directory doesn't exist
Date: Tue, 25 Aug 2009 14:29:37 -0400

>> How 'bout this patch instead (which could even work on Tramp
>> directories)?
> I don't think so.  It's still a race condition in between choosing an
> apparently existent directory and then making use of that directory,
> isn't it?

Emacs is made of such race conditions.  But there's a big difference
between DIR and/or its parents appearing/disappearing (as in my patch),
vs $HOME or some other unrealted directory appearing/disappearing.

> I think "/" is the only place certain to exist and not go
> away.  It's also the simplest approach.

I'd like to make it work for Tramp dirs as well.

> `sha1-string-external' is another place this comes up.  In that case the
> operation is completely unrelated to any directory, and so really should
> go to "/" so as to be independent of default-directory or whether that
> dir is coming or going.

For that one, yes, "/" is clearly a better choice.


        Stefan

Message #56 received at 3911 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Kevin Ryde <user42 <at> zip.com.au>
To: Stefan Monnier <monnier <at> IRO.UMontreal.CA>
Cc: 3911 <at> debbugs.gnu.org, Chong Yidong <cyd <at> stupidchicken.com>
Subject: Re: bug#3911: get-free-disk-space when default-directory doesn't exist
Date: Sat, 29 Aug 2009 11:32:45 +1000

Stefan Monnier <monnier <at> IRO.UMontreal.CA> writes:
>
> Emacs is made of such race conditions.

But this one is easy to avoid.  I really don't understand why this bug
has gone on and on.  What's the objection to "/"?  As far as I can tell
it's smaller, faster, and certain to work.  (Faster especially if
there's some nfs or ftpfs or similar involved where a system chdir may
do some evil remote query.)

> I'd like to make it work for Tramp dirs as well.

Sounds good.  Tramp holds open a session doesn't it, so at least there'd
be no call-process to worry about for it. :)

Message #61 received at 3911 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Kevin Ryde <user42 <at> zip.com.au>
Cc: 3911 <at> debbugs.gnu.org, Chong Yidong <cyd <at> stupidchicken.com>
Subject: Re: bug#3911: get-free-disk-space when default-directory doesn't exist
Date: Fri, 28 Aug 2009 23:37:26 -0400

>> Emacs is made of such race conditions.
> But this one is easy to avoid.  I really don't understand why this bug
> has gone on and on.  What's the objection to "/"?  As far as I can tell
> it's smaller, faster, and certain to work.  (Faster especially if
> there's some nfs or ftpfs or similar involved where a system chdir may
> do some evil remote query.)

The objection to / is only that it doesn't work for Tramp.

>> I'd like to make it work for Tramp dirs as well.
> Sounds good.  Tramp holds open a session doesn't it, so at least there'd
> be no call-process to worry about for it. :)

I don't know what you're referring to.


        Stefan

Message #66 received at 3911 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Kevin Ryde <user42 <at> zip.com.au>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 3911 <at> debbugs.gnu.org
Subject: Re: bug#3911: get-free-disk-space when default-directory doesn't exist
Date: Tue, 01 Sep 2009 11:37:21 +1000

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>
> The objection to / is only that it doesn't work for Tramp.

Umm, now it's me that doesn't understand! - the "df"-running part is for
local queries, ie. not-tramp, isn't it?  (No remotes at all, so not
ange-ftp or whatnot either ...?)

Message #71 received at 3911 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Kevin Ryde <user42 <at> zip.com.au>
Cc: 3911 <at> debbugs.gnu.org
Subject: Re: bug#3911: get-free-disk-space when default-directory doesn't exist
Date: Tue, 01 Sep 2009 02:05:09 -0400

>> The objection to / is only that it doesn't work for Tramp.
> Umm, now it's me that doesn't understand! - the "df"-running part is for
> local queries, ie. not-tramp, isn't it?  (No remotes at all, so not
> ange-ftp or whatnot either ...?)

Right now, it is.  But using my patch, together with changing
`call-process' with `process-file' would make it possible to remove the
file-remote-p check.


        Stefan

Message #76 received at 3911 <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Kevin Ryde <user42 <at> zip.com.au>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 3911 <at> debbugs.gnu.org
Subject: Re: bug#3911: get-free-disk-space when default-directory doesn't exist
Date: Mon, 07 Sep 2009 10:28:47 +1000

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>
> Right now, it is.

Prospective remote support isn't a reason to leave it dodgy for locals
now is it?

> `process-file'

The cd in tramp-handle-process-file will have the same problem as the
local cd in call-process.  If there has to be a cd then it should be a
safe place like the root dir or similar.

Perhaps default-directory as the remote root dir would get close.  Is
there a func to figure that?  Except I think remote support should go
through the file-name-handler thingie to allow for remotes with their
own free-space query mechanism, rather than running "df" (or assuming
that `directory-free-space-program' for the local machine is also good
on the remote, etc).

Message #81 received at 3911-done <at> emacsbugs.donarmstrong.com (full text, mbox):

From: Kevin Ryde <user42 <at> zip.com.au>
To: 3911-done <at> debbugs.gnu.org
Subject: Re: bug#3911: get-free-disk-space when default-directory doesn't exist
Date: Fri, 02 Oct 2009 10:24:49 +1000

Installed by Stefan.

Previous Next

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