GNU bug report logs - #28023
fix make-temp-file race on local host

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 28023 in the body.
You can then email your comments to 28023 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: Emacs bug reports and feature requests <bug-gnu-emacs <at> gnu.org>
Subject: fix make-temp-file race on local host
Date: Tue, 8 Aug 2017 22:38:05 -0700

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

Tags: patch
Severity: important

I plan to install the attached patch in master soon, and am sending it for 
review to bug-gnu-emacs first in case there's some problem with it on 
MS-Windows. Although this patch does not fix all instances of the race in Gnu 
Emacs (notably, Tramp access is still affected) it's a good start.

[0001-Fix-make-temp-file-race-on-local-files.patch (text/x-patch, attachment)]

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

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 28023 <at> debbugs.gnu.org
Subject: Re: bug#28023: fix make-temp-file race on local host
Date: Wed, 09 Aug 2017 08:51:25 +0200

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

Hi Paul,

> I plan to install the attached patch in master soon, and am sending it
> for review to bug-gnu-emacs first in case there's some problem with it
> on MS-Windows. Although this patch does not fix all instances of the
> race in Gnu Emacs (notably, Tramp access is still affected) it's a
> good start.

I haven't followed the whole thread, sorry. Could you pls elaborate,
what you do expect from Tramp?

Best regards, Michael.

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

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: 28023 <at> debbugs.gnu.org
Subject: Re: bug#28023: fix make-temp-file race on local host
Date: Wed, 9 Aug 2017 02:22:37 -0700

Michael Albinus wrote:
> what you do expect from Tramp?

Tramp needs to support a new method make-temp-file that creates a file (or 
directory) atomically, as make-temp-file does locally now. Tramp also needs to 
support the excl flag of write-region (currently it ignores that flag). I was 
planning to write this up as a bug report after the patch goes in.

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

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 28023 <at> debbugs.gnu.org
Subject: Re: bug#28023: fix make-temp-file race on local host
Date: Wed, 09 Aug 2017 13:21:28 +0200

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

> Tramp needs to support a new method make-temp-file that creates a file
> (or directory) atomically, as make-temp-file does locally now.

That means, make-temp-file shall be converted into a magic file name
operation. I'll do my best, but I don't know whether we could get an
implementation for all Tramp methods w/o a race condition.

> Tramp also needs to support the excl flag of write-region (currently
> it ignores that flag).

This sounds trivial. And yes, it will help.

> I was planning to write this up as a bug report after the patch goes
> in.

Pls do.

Best regards, Michael.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 28023 <at> debbugs.gnu.org
Subject: Re: bug#28023: fix make-temp-file race on local host
Date: Wed, 09 Aug 2017 19:05:16 +0300

> From: Paul Eggert <eggert <at> cs.ucla.edu>
> Date: Tue, 8 Aug 2017 22:38:05 -0700
> 
> I plan to install the attached patch in master soon, and am sending it for 
> review to bug-gnu-emacs first in case there's some problem with it on 
> MS-Windows.

Thanks for the heads-up.  Unfortunately, it isn't as simple as we'd
like it to be, because Gnulib doesn't support file names encoded in
UTF-8 (or any other encoding except the current system codepage) on
MS-Windows, while Emacs does.

We could resolve this problem by providing a way for the Windows build
to specify its own replacements for 'open', 'mkdir', and 'lstat'
(Emacs already have such replacements in w32.c), which tempname.c
calls, but that would require minor changes in Gnulib's tempname.c.
Or we could steal the relevant code from tempname.c into a
Windows-specific implementation in w32.c, and refrain from using the
Gnulib version on Windows.  Which way would you prefer to go?  Or
maybe you have yet another idea?

> @@ -1915,9 +1913,7 @@ endif
>  ## begin gnulib module secure_getenv
>  ifeq (,$(OMIT_GNULIB_MODULE_secure_getenv))
>  
> -ifneq (,$(gl_GNULIB_ENABLED_secure_getenv))
>  
> -endif
>  EXTRA_DIST += secure_getenv.c

This seems to say that we will sometimes use secure_getenv, but the
relevant code in tempname.c is conditioned on _LIBC, which AFAIU is
only true when building glibc, so why do we need that?  Or did I miss
something?

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

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 28023 <at> debbugs.gnu.org
Subject: Re: bug#28023: fix make-temp-file race on local host
Date: Wed, 9 Aug 2017 11:47:36 -0700

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

On 08/09/2017 09:05 AM, Eli Zaretskii wrote:
> We could resolve this problem by providing a way for the Windows build
> to specify its own replacements for 'open', 'mkdir', and 'lstat'
> (Emacs already have such replacements in w32.c), which tempname.c
> calls, but that would require minor changes in Gnulib's tempname.c.

This sounds like a better way to go. What minor changes would be needed? 
tempname.c already includes config.h, which includes ms-w32.h, which 
redefines open, mkdir, and lstat, so I thought it would work as-is.

> This seems to say that we will sometimes use secure_getenv, but the
> relevant code in tempname.c is conditioned on _LIBC, which AFAIU is
> only true when building glibc, so why do we need that?  Or did I miss
> something?
No, good catch, Emacs never needs secure_getenv. I fixed Gnulib, 
installed the attached patch to propagate that fix, and will adjust the 
proposed make-temp-file patch accordingly.

[0001-Merge-from-gnulib.patch (text/x-patch, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 28023 <at> debbugs.gnu.org
Subject: Re: bug#28023: fix make-temp-file race on local host
Date: Wed, 09 Aug 2017 22:09:41 +0300

> Cc: 28023 <at> debbugs.gnu.org
> From: Paul Eggert <eggert <at> cs.ucla.edu>
> Date: Wed, 9 Aug 2017 11:47:36 -0700
> 
> On 08/09/2017 09:05 AM, Eli Zaretskii wrote:
> > We could resolve this problem by providing a way for the Windows build
> > to specify its own replacements for 'open', 'mkdir', and 'lstat'
> > (Emacs already have such replacements in w32.c), which tempname.c
> > calls, but that would require minor changes in Gnulib's tempname.c.
> 
> This sounds like a better way to go. What minor changes would be needed? 
> tempname.c already includes config.h, which includes ms-w32.h, which 
> redefines open, mkdir, and lstat, so I thought it would work as-is.

Perhaps you are right, and it will "just work".  I will have to try
(for now, I just looked at the sources and the changes, but didn't try
building).

> > This seems to say that we will sometimes use secure_getenv, but the
> > relevant code in tempname.c is conditioned on _LIBC, which AFAIU is
> > only true when building glibc, so why do we need that?  Or did I miss
> > something?
> No, good catch, Emacs never needs secure_getenv. I fixed Gnulib, 
> installed the attached patch to propagate that fix, and will adjust the 
> proposed make-temp-file patch accordingly.

Thanks.

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

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 28023 <at> debbugs.gnu.org
Subject: Re: bug#28023: fix make-temp-file race on local host
Date: Wed, 9 Aug 2017 16:36:55 -0700

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

Eli Zaretskii wrote:
> Perhaps you are right, and it will "just work".  I will have to try
> (for now, I just looked at the sources and the changes, but didn't try
> building).

It looks to me like it should just work. You might also consider the second 
attached patch: it removes the MS-Windows implementation of mkostemp, since the 
generic code should just work as well. I've tested only the first attached 
patch, though: it is basically the same as before except it's rebased to current 
master.

[0001-Fix-make-temp-file-race-on-local-files.patch (text/x-patch, attachment)]

[0002-Remove-ms-w32-mkostemp.patch (text/x-patch, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 28023 <at> debbugs.gnu.org
Subject: Re: bug#28023: fix make-temp-file race on local host
Date: Thu, 10 Aug 2017 18:35:05 +0300

> Cc: 28023 <at> debbugs.gnu.org
> From: Paul Eggert <eggert <at> cs.ucla.edu>
> Date: Wed, 9 Aug 2017 16:36:55 -0700
> 
> Eli Zaretskii wrote:
> > Perhaps you are right, and it will "just work".  I will have to try
> > (for now, I just looked at the sources and the changes, but didn't try
> > building).
> 
> It looks to me like it should just work.

Well, it's not that easy after all.  First, nt/gnulib-cfg.mk needs to
be fixed to remove the lines that disable building tempname.c and
mkostmp.c, and nt/mingw-cfg.site needs to remove the line which claims
that mkostmp is available.  Otherwise, tempname.c will not be
compiled.  This is easy to fix.

Then it turns out ms-w32.h only makes its redirections when compiling
with -Demacs, which is not what happens when Gnulib sources are
compiled.  I can fix this, but it will need a few changes in lib-src,
which currently doesn't expect 'open' to be redirected.

And finally, tempname.c calls mkdir with 2 arguments, whereas mkdir on
Windows accepts only one.  (This means Gnulib lacks a dependency,
since the tempname module should then depend on mkdir, which isn't in
lib/.)  Since we don't want the Gnulib replacement for mkdir, we will
have to change w32.c:sys_mkdir to accept 2 arguments to fix this,
unless you have a better idea.

How do you propose to proceed with this?

> You might also consider the second 
> attached patch: it removes the MS-Windows implementation of mkostemp, since the 
> generic code should just work as well.

Once the above problems are taken care of, the mkostemp part should be
easy.

Thanks.

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

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 28023 <at> debbugs.gnu.org
Subject: Re: bug#28023: fix make-temp-file race on local host
Date: Thu, 10 Aug 2017 15:24:13 -0700

Eli Zaretskii wrote:


> And finally, tempname.c calls mkdir with 2 arguments, whereas mkdir on
> Windows accepts only one.  (This means Gnulib lacks a dependency,
> since the tempname module should then depend on mkdir, which isn't in
> lib/.)  Since we don't want the Gnulib replacement for mkdir, we will
> have to change w32.c:sys_mkdir to accept 2 arguments to fix this,
> unless you have a better idea.

No, your ideas all sound good. It's simpler for the generic code, if the mkdir 
replacement acts like POSIX mkdir.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 28023 <at> debbugs.gnu.org
Subject: Re: bug#28023: fix make-temp-file race on local host
Date: Fri, 11 Aug 2017 09:21:42 +0300

> Cc: 28023 <at> debbugs.gnu.org
> From: Paul Eggert <eggert <at> cs.ucla.edu>
> Date: Thu, 10 Aug 2017 15:24:13 -0700
> 
> No, your ideas all sound good. It's simpler for the generic code, if the mkdir 
> replacement acts like POSIX mkdir.

So how to proceed?  Do you want to wait until I make all those changes
on master?  Or push a branch with the changes?  Something else?

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

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 28023 <at> debbugs.gnu.org
Subject: Re: bug#28023: fix make-temp-file race on local host
Date: Fri, 11 Aug 2017 00:44:36 -0700

Eli Zaretskii wrote:
> So how to proceed?  Do you want to wait until I make all those changes
> on master?  Or push a branch with the changes?  Something else?

I'd rather not mess with a branch. If it's easy to make those changes to master 
please do so. If not, I can modify the patch to keep the current algorithm for 
DOS_NT, though this will be ugly.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 28023 <at> debbugs.gnu.org
Subject: Re: bug#28023: fix make-temp-file race on local host
Date: Fri, 11 Aug 2017 11:03:04 +0300

> Cc: 28023 <at> debbugs.gnu.org
> From: Paul Eggert <eggert <at> cs.ucla.edu>
> Date: Fri, 11 Aug 2017 00:44:36 -0700
> 
> If it's easy to make those changes to master please do so.

OK, will do.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: eggert <at> cs.ucla.edu
Cc: 28023 <at> debbugs.gnu.org
Subject: Re: bug#28023: fix make-temp-file race on local host
Date: Sat, 12 Aug 2017 11:31:52 +0300

> Date: Fri, 11 Aug 2017 11:03:04 +0300
> From: Eli Zaretskii <eliz <at> gnu.org>
> Cc: 28023 <at> debbugs.gnu.org
> 
> > Cc: 28023 <at> debbugs.gnu.org
> > From: Paul Eggert <eggert <at> cs.ucla.edu>
> > Date: Fri, 11 Aug 2017 00:44:36 -0700
> > 
> > If it's easy to make those changes to master please do so.
> 
> OK, will do.

Done.

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

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 28023 <at> debbugs.gnu.org
Subject: Re: bug#28023: fix make-temp-file race on local host
Date: Sat, 12 Aug 2017 12:35:26 +0200

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

Hi Paul,

> Tramp also needs to support the excl flag of write-region (currently
> it ignores that flag).

I've implemented this, commit ec5cfaa456.

Best regards, Michael.

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

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: 28023 <at> debbugs.gnu.org
Subject: Re: bug#28023: fix make-temp-file race on local host
Date: Sat, 12 Aug 2017 08:55:31 -0700

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

Michael Albinus wrote:
> I've implemented this, commit ec5cfaa456.

Thanks. Unfortunately this caused "make check" to fail on Fedora 26. I installed 
the attached patch, which causes the test to pass. Can you please look over it 
to see whether it makes sense, and look for similar problems elsewhere? (I'm not 
that familiar with Tramp.) Thanks.

[0001-Adjust-jka-compr-to-recent-Tramp-changes.patch (text/x-patch, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 28023 <at> debbugs.gnu.org, michael.albinus <at> gmx.de
Subject: Re: bug#28023: fix make-temp-file race on local host
Date: Sat, 12 Aug 2017 19:09:41 +0300

Paul,

Looking back at your original patch, I think the function you are
adding should be named make-temp-file-internal, as we do with other
functions whose low-level parts are implemented in C.
fileio--SOMETHING sounds like something we never had, so I think it's
better to a void a new prefix.

Thanks.

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

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 28023 <at> debbugs.gnu.org, michael.albinus <at> gmx.de
Subject: Re: bug#28023: fix make-temp-file race on local host
Date: Sat, 12 Aug 2017 09:25:37 -0700

Eli Zaretskii wrote:
> I think the function you are
> adding should be named make-temp-file-internal, as we do with other
> functions whose low-level parts are implemented in C.

I was following the lead of names like lread--substitute-command-keys, 
print--preprocess, and thread--blocker, all low-level C functions whose first 
part identifies which C module they're in. Although I see that the "-internal" 
suffix is more popular for this sort of thing, isn't that a revenant of the old 
days, before we instituted the convention of using PREFIX--NAME for private 
names? Or is the "-internal" suffix a separate naming convention, used by both 
Lisp and C code, that has a different semantics from PREFIX--NAME? If so, it 
would be nice to have advice somewhere as to when to use the -internal suffix vs 
when to use PREFIX--NAME.

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 28023 <at> debbugs.gnu.org, michael.albinus <at> gmx.de
Subject: Re: bug#28023: fix make-temp-file race on local host
Date: Sat, 12 Aug 2017 19:52:46 +0300

> Cc: michael.albinus <at> gmx.de, 28023 <at> debbugs.gnu.org
> From: Paul Eggert <eggert <at> cs.ucla.edu>
> Date: Sat, 12 Aug 2017 09:25:37 -0700
> 
> I was following the lead of names like lread--substitute-command-keys, 
> print--preprocess, and thread--blocker, all low-level C functions whose first 
> part identifies which C module they're in. Although I see that the "-internal" 
> suffix is more popular for this sort of thing, isn't that a revenant of the old 
> days, before we instituted the convention of using PREFIX--NAME for private 
> names? Or is the "-internal" suffix a separate naming convention, used by both 
> Lisp and C code, that has a different semantics from PREFIX--NAME? If so, it 
> would be nice to have advice somewhere as to when to use the -internal suffix vs 
> when to use PREFIX--NAME.

I think the prefix that comes from the file where the function is
defined is more of a Lisp convention, and the -internal convention is
more for C implementations.

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

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 28023 <at> debbugs.gnu.org
Subject: Re: bug#28023: fix make-temp-file race on local host
Date: Sat, 12 Aug 2017 19:51:12 +0200

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

Hi Paul,

> Thanks. Unfortunately this caused "make check" to fail on Fedora 26. I
> installed the attached patch, which causes the test to pass. Can you
> please look over it to see whether it makes sense, and look for
> similar problems elsewhere? (I'm not that familiar with Tramp.)

The patch is OK. Sorry for the trouble, I've searched for
"-write-region" in the whole lisp directory for such cases, but I must
have missed jka-compr.el.

> Thanks.

Best regards, Michael.

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

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 28023 <at> debbugs.gnu.org, michael.albinus <at> gmx.de
Subject: Re: bug#28023: fix make-temp-file race on local host
Date: Sat, 12 Aug 2017 10:57:26 -0700

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

Eli Zaretskii wrote:
> I think the prefix that comes from the file where the function is
> defined is more of a Lisp convention, and the -internal convention is
> more for C implementations.

I see lots of exceptions to these conventions, presumably either because 
developers don't know the intent or because the code was written before the 
conventions were established. Still, it's better to document the intent so I 
installed the attached patch to do that. I'll adjust the make-temp-file patch 
accordingly.

[0001-Document-internal-use-naming-conventions.patch (text/x-patch, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 28023 <at> debbugs.gnu.org, michael.albinus <at> gmx.de
Subject: Re: bug#28023: fix make-temp-file race on local host
Date: Sat, 12 Aug 2017 21:07:20 +0300

> Cc: michael.albinus <at> gmx.de, 28023 <at> debbugs.gnu.org
> From: Paul Eggert <eggert <at> cs.ucla.edu>
> Date: Sat, 12 Aug 2017 10:57:26 -0700
> 
> I see lots of exceptions to these conventions, presumably either because 
> developers don't know the intent or because the code was written before the 
> conventions were established. Still, it's better to document the intent so I 
> installed the attached patch to do that.

Thanks, I agree this should be documented.

> I'll adjust the make-temp-file patch accordingly.

Thanks.

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

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: 28023-done <at> debbugs.gnu.org
Subject: Re: bug#28023: fix make-temp-file race on local host
Date: Sun, 13 Aug 2017 00:21:03 -0700

Michael Albinus wrote:
> Paul Eggert<eggert <at> cs.ucla.edu>  writes:
> 
>> Tramp needs to support a new method make-temp-file that creates a file
>> (or directory) atomically, as make-temp-file does locally now.
> That means, make-temp-file shall be converted into a magic file name
> operation. I'll do my best, but I don't know whether we could get an
> implementation for all Tramp methods w/o a race condition.
> 
>> Tramp also needs to support the excl flag of write-region (currently
>> it ignores that flag).
> This sounds trivial. And yes, it will help.

Thanks for doing that. Because of that, I'm no longer seeing a race in 
make-temp-file, even for Tramp files. That is, although my proposed patch is 
still a performance win on local files, I don't see how it is a correctness win 
any more. It's still worth installing for the performance reasons, though, so I 
did that and I am marking this bug as done.

>> I was planning to write this up as a bug report after the patch goes
>> in.
> Pls do.

To some extent this is moot now, if I understand things correctly. That is, the 
only reason to write this up now would be for performance reasons, not a race 
condition.

There is still a race involving destination directories, for both Tramp and 
non-Tramp versions. I plan to take a look at that next.

Previous Next

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