[PATCH] make sure internal temp files are user readable and writeable

[PATCH] make sure internal temp files are user readable and writeable

[Martijn Dekker]

[-- Attachment #1: Type: text/plain, Size: 507 bytes --]

Here-documents fail if the umask does not allow user reading.

$ zsh -c $'umask u-r; cat <<EOF\nEOF'
zsh:1: can't create temp file for here document: permission denied

Silly though 'umask u-r' may be, the fact that here-documents use
temporary files on zsh is an internal implementation detail that should
make no difference to the script. It is trivial to save and temporarily
fix the umask during the creation of the temporary file.

This may fix a few other things that use gettempfile() as well.

- M.

[-- Attachment #2: heredoc-umask.patch --]
[-- Type: text/plain, Size: 925 bytes --]

diff --git a/Src/utils.c b/Src/utils.c
index 6517e15..a9c6231 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -2177,6 +2177,7 @@ gettempfile(const char *prefix, int use_heap, char **tempname)
 {
     char *fn;
     int fd;
+    mode_t old_umask = umask(0177);
 #if HAVE_MKSTEMP
     char *suffix = prefix ? ".XXXXXX" : "XXXXXX";
 
@@ -2212,6 +2213,7 @@ gettempfile(const char *prefix, int use_heap, char **tempname)
 #endif
     *tempname = fn;
 
+    umask(old_umask);
     unqueue_signals();
     return fd;
 }
diff --git a/Test/A04redirect.ztst b/Test/A04redirect.ztst
index ef7ddb2..b5b65cf 100644
--- a/Test/A04redirect.ztst
+++ b/Test/A04redirect.ztst
@@ -667,3 +667,12 @@
 0:Redirect in the middle of assignments
 >b
 >d
+
+  umask 0777
+  cat <<'  HERE'
+  look ma, no permissions
+  HERE
+  cat <<<"it's a miracle"
+0:Here-{string,document}s succeed with restrictive umask
+>  look ma, no permissions
+>it's a miracle

Re: [PATCH] make sure internal temp files are user readable and writeable

[Stephane Chazelas]

2018-03-26 18:45:23 +0200, Martijn Dekker:
> Here-documents fail if the umask does not allow user reading.
> 
> $ zsh -c $'umask u-r; cat <<EOF\nEOF'
> zsh:1: can't create temp file for here document: permission denied
[...]
> +    mode_t old_umask = umask(0177);
[...]

Thanks. See also 42446
(https://www.zsh.org/mla/workers/2018/msg00252.html)
with other suggested options.

There I mentioned the potential need to block signals between
the time the umask is changed temporarily and when it's restored
(to avoid traps running with the wrong umask (0177 instead of
the user's requested one)).

-- 
Stephane

Re: [PATCH] make sure internal temp files are user readable and writeable

[Martijn Dekker]

Op 26-03-18 om 19:00 schreef Stephane Chazelas:
> Thanks. See also 42446
> (https://www.zsh.org/mla/workers/2018/msg00252.html)
> with other suggested options.

Ah yes, sorry, forgot you'd already brought it up here.

> There I mentioned the potential need to block signals between
> the time the umask is changed temporarily and when it's restored
> (to avoid traps running with the wrong umask (0177 instead of
> the user's requested one)).

Good point.

Please consider my patch withdrawn.

- M.

Re: [PATCH] make sure internal temp files are user readable and writeable

[Martijn Dekker]

[-- Attachment #1: Type: text/plain, Size: 1013 bytes --]

Op 26-03-18 om 19:37 schreef Martijn Dekker:
> Op 26-03-18 om 19:00 schreef Stephane Chazelas:
>> Thanks. See also 42446
>> (https://www.zsh.org/mla/workers/2018/msg00252.html)
>> with other suggested options.
> 
> Ah yes, sorry, forgot you'd already brought it up here.

Re-reading that, my thought is: while option 2 might be nice to have,
option 4 is the simple, obvious and immediate fix, so that's the one I'm
capable of providing -- especially with a release imminent.

If you'd like to have a go at implementing option 2, so much the better.

>> There I mentioned the potential need to block signals between
>> the time the umask is changed temporarily and when it's restored
>> (to avoid traps running with the wrong umask (0177 instead of
>> the user's requested one)).
> 
> Good point.
> 
> Please consider my patch withdrawn.

Today was apparently not my best day. Since that function was already
blocking (a.k.a. queueing) signals to do its thing, of course my patch
was trivial to fix. Take 2.

- M.

[-- Attachment #2: heredoc-umask.patch --]
[-- Type: text/plain, Size: 1304 bytes --]

diff --git a/Src/utils.c b/Src/utils.c
index 6517e15..4660142 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -2177,10 +2177,12 @@ gettempfile(const char *prefix, int use_heap, char **tempname)
 {
     char *fn;
     int fd;
+    mode_t old_umask;
 #if HAVE_MKSTEMP
     char *suffix = prefix ? ".XXXXXX" : "XXXXXX";
 
     queue_signals();
+    old_umask = umask(0177);
     if (!prefix && !(prefix = getsparam("TMPPREFIX")))
 	prefix = DEFAULT_TMPPREFIX;
     if (use_heap)
@@ -2198,6 +2200,7 @@ gettempfile(const char *prefix, int use_heap, char **tempname)
     int failures = 0;
 
     queue_signals();
+    old_umask = umask(0177);
     do {
 	if (!(fn = gettempname(prefix, use_heap))) {
 	    fd = -1;
@@ -2212,6 +2215,7 @@ gettempfile(const char *prefix, int use_heap, char **tempname)
 #endif
     *tempname = fn;
 
+    umask(old_umask);
     unqueue_signals();
     return fd;
 }
diff --git a/Test/A04redirect.ztst b/Test/A04redirect.ztst
index ef7ddb2..b5b65cf 100644
--- a/Test/A04redirect.ztst
+++ b/Test/A04redirect.ztst
@@ -667,3 +667,12 @@
 0:Redirect in the middle of assignments
 >b
 >d
+
+  umask 0777
+  cat <<'  HERE'
+  look ma, no permissions
+  HERE
+  cat <<<"it's a miracle"
+0:Here-{string,document}s succeed with restrictive umask
+>  look ma, no permissions
+>it's a miracle

Re: [PATCH] make sure internal temp files are user readable and writeable

[Peter Stephenson]

On Mon, 26 Mar 2018 22:32:50 +0200
Martijn Dekker <martijn@inlv.org> wrote:
> Today was apparently not my best day. Since that function was already
> blocking (a.k.a. queueing) signals to do its thing, of course my patch
> was trivial to fix. Take 2.

Couldn't see any obvious problem with that, so I've committed it.
Thanks

pws