bug#28023: fix make-temp-file race on local host

classic Classic list List threaded Threaded
23 messages Options
12
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#28023: fix make-temp-file race on local host

Paul Eggert
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 (22K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#28023: fix make-temp-file race on local host

Michael Albinus
Paul Eggert <[hidden email]> 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.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#28023: fix make-temp-file race on local host

Paul Eggert
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.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#28023: fix make-temp-file race on local host

Michael Albinus
Paul Eggert <[hidden email]> 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.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#28023: fix make-temp-file race on local host

Eli Zaretskii
In reply to this post by Paul Eggert
> From: Paul Eggert <[hidden email]>
> 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?



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#28023: fix make-temp-file race on local host

Paul Eggert
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 (14K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#28023: fix make-temp-file race on local host

Eli Zaretskii
> Cc: [hidden email]
> From: Paul Eggert <[hidden email]>
> 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.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#28023: fix make-temp-file race on local host

Paul Eggert
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 (20K) Download Attachment
0002-Remove-ms-w32-mkostemp.patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#28023: fix make-temp-file race on local host

Eli Zaretskii
> Cc: [hidden email]
> From: Paul Eggert <[hidden email]>
> 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.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#28023: fix make-temp-file race on local host

Paul Eggert
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.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#28023: fix make-temp-file race on local host

Eli Zaretskii
> Cc: [hidden email]
> From: Paul Eggert <[hidden email]>
> 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?



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#28023: fix make-temp-file race on local host

Paul Eggert
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.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#28023: fix make-temp-file race on local host

Eli Zaretskii
> Cc: [hidden email]
> From: Paul Eggert <[hidden email]>
> 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.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#28023: fix make-temp-file race on local host

Eli Zaretskii
> Date: Fri, 11 Aug 2017 11:03:04 +0300
> From: Eli Zaretskii <[hidden email]>
> Cc: [hidden email]
>
> > Cc: [hidden email]
> > From: Paul Eggert <[hidden email]>
> > 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.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#28023: fix make-temp-file race on local host

Michael Albinus
In reply to this post by Paul Eggert
Paul Eggert <[hidden email]> 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.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#28023: fix make-temp-file race on local host

Paul Eggert
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 (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#28023: fix make-temp-file race on local host

Eli Zaretskii
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.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#28023: fix make-temp-file race on local host

Paul Eggert
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.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#28023: fix make-temp-file race on local host

Eli Zaretskii
> Cc: [hidden email], [hidden email]
> From: Paul Eggert <[hidden email]>
> 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.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

bug#28023: fix make-temp-file race on local host

Michael Albinus
In reply to this post by Paul Eggert
Paul Eggert <[hidden email]> 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.



12
Loading...