PATCH: [for consideration] TMPSUFFIX

PATCH: [for consideration] TMPSUFFIX

[Bart Schaefer]

The signal queuing in utils.c was just something I noticed while looking
for the right place to put the TMPSUFFIX code.

The addfilelist() is done whether or not we successfully opened the file,
because (a) it was that way before and (b) the name is returned either
way, so the caller might try to create the file itself.  However, it
seems a bit odd that the failure of open() is mostly ignored?


diff --git a/Doc/Zsh/params.yo b/Doc/Zsh/params.yo
index 03625ce..c7d84b9 100644
--- a/Doc/Zsh/params.yo
+++ b/Doc/Zsh/params.yo
@@ -1547,6 +1547,15 @@ A pathname prefix which the shell will use for all temporary files.
 Note that this should include an initial part for the file name as
 well as any directory names.  The default is `tt(/tmp/zsh)'.
 )
+vindex(TMPSUFFIX)
+item(tt(TMPSUFFIX))(
+A filename suffix which the shell will use for temporary files created
+by process substitutions (e.g., `tt(=LPAR()var(list)RPAR())').
+Note that the value should include a leading dot `tt(.)' if intended
+to be interpreted as a file extension.  The default is not to append
+any suffix, thus this parameter should be assigned only when needed
+and then unset again.
+)
 vindex(watch)
 vindex(WATCH)
 item(tt(watch) <S> <Z> (tt(WATCH) <S>))(
diff --git a/Src/exec.c b/Src/exec.c
index 4e89340..6b5bfd6 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -4332,18 +4332,27 @@ getoutputfile(char *cmd, char **eptr)
 	    untokenize(s);
     }
 
-    addfilelist(nam, 0);
-
     if (!s)
 	child_block();
-    fd = open(nam, O_WRONLY | O_CREAT | O_EXCL | O_NOCTTY, 0600);
+
+    if ((fd = open(nam, O_WRONLY | O_CREAT | O_EXCL | O_NOCTTY, 0600)) >= 0) {
+	char *suffix = getsparam("TMPSUFFIX");
+	if (suffix && *suffix && !strstr(suffix, "/")) {
+	    suffix = dyncat(nam, unmeta(suffix));
+	    if (link(nam, suffix) == 0)
+		nam = ztrdup(suffix);
+	}
+    }
+    addfilelist(nam, 0);
 
     if (s) {
 	/* optimised here-string */
 	int len;
-	unmetafy(s, &len);
-	write_loop(fd, s, len);
-	close(fd);
+	if (fd >= 0) {
+	    unmetafy(s, &len);
+	    write_loop(fd, s, len);
+	    close(fd);
+	}
 	return nam;
     }
 
diff --git a/Src/utils.c b/Src/utils.c
index b434821..db43529 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -2164,6 +2164,7 @@ gettempfile(const char *prefix, int use_heap, char **tempname)
 #if HAVE_MKSTEMP
     char *suffix = prefix ? ".XXXXXX" : "XXXXXX";
 
+    queue_signals();
     if (!prefix && !(prefix = getsparam("TMPPREFIX")))
 	prefix = DEFAULT_TMPPREFIX;
     if (use_heap)
@@ -2180,6 +2181,7 @@ gettempfile(const char *prefix, int use_heap, char **tempname)
 #else
     int failures = 0;
 
+    queue_signals();
     do {
 	if (!(fn = gettempname(prefix, use_heap))) {
 	    fd = -1;
@@ -2193,6 +2195,8 @@ gettempfile(const char *prefix, int use_heap, char **tempname)
     } while (errno == EEXIST && ++failures < 16);
 #endif
     *tempname = fn;
+
+    unqueue_signals();
     return fd;
 }
 

Re: PATCH: [for consideration] TMPSUFFIX

[Daniel Shahaf]

Bart Schaefer wrote on Sun, Sep 25, 2016 at 15:51:12 -0700:
> The addfilelist() is done whether or not we successfully opened the file,
> because (a) it was that way before and (b) the name is returned either
> way, so the caller might try to create the file itself.  However, it
> seems a bit odd that the failure of open() is mostly ignored?

This looks like a race condition: if another process creates the
filename that mktemp() returned before the open(O_EXCL) call [which will
therefore fail], zsh will remove that filename even though zsh didn't
create it.  [This could trigger through a deliberate attack or through
a race condition between two zsh instances calling =(:) concurrently.]

I don't get "the caller will create the file".  The =(:) syntax promises
to expand to an existing filename with a defined content.  How is it
helpful to return a filename, which may or may not exist and certainly
doesn't contain the output of 'cmd'?  It seems to me erroring out would
be better.  (The caller can call getoutputfile() again if he wishes.)

The patch creates two hard links to the file but only removes one of
them.

Cheers,

Daniel

Re: PATCH: [for consideration] TMPSUFFIX

[Bart Schaefer]

On Sep 26,  7:25am, Daniel Shahaf wrote:
} Subject: Re: PATCH: [for consideration] TMPSUFFIX
}
} This looks like a race condition: if another process creates the
} filename that mktemp() returned before the open(O_EXCL) call [which will
} therefore fail], zsh will remove that filename even though zsh didn't
} create it.

If another process creates the filename that mktemp() returned, we have
a much larger problem -- the whole point is that it should be impossible
for another process to create that name first.

} [This could trigger through a deliberate attack or through
} a race condition between two zsh instances calling =(:) concurrently.]

It definitely CANNOT occur because of two zsh instances using =().  I
won't assert the attack is 100% impossible, but there's nothing more
we can do about that than we already have.

So mostly we're concerned with $TMPDIR being unwritable, or full file
system, etc. -- but of course failure for any other reason including
attacker-duplicated file name would get swept up if we change the way
failure is handled.

} I don't get "the caller will create the file".  [...]
} (The caller can call getoutputfile() again if he wishes.)

The SHELL SCRIPT caller, not the C code caller.  That is, as currently
coded, =(...) will substitute a file name whether or not it actually got
created.  In a case like

    () { ... do something with $1 ... } =(contents)

the name in $1 could be used in output redirections, etc.  As written,
exec.c seems intended to allow this.

} How is it helpful to return a filename, which may or may not exist and
} certainly doesn't contain the output of 'cmd'? It seems to me erroring
} out would be better.

It'd have to be an error on the same order as "bad substitution" so the
whole command context fails.  Hence bringing it up for discussion.

} The patch creates two hard links to the file but only removes one of
} them.

Oops, design iteration error -- I first used rename() instead of link()
until I realized that would allow e.g. TMPSUFFIX="/../other/filesystem" 
which would invoke an implicit copy and invalidate the file descriptor.
Didn't put back the original addfilelist().  Thanks for noticing, will
fix in next iteration after we resolve the error-handling question.

Re: PATCH: [for consideration] TMPSUFFIX

[Daniel Shahaf]

Bart Schaefer wrote on Mon, Sep 26, 2016 at 09:19:22 -0700:
> On Sep 26,  7:25am, Daniel Shahaf wrote:
> } Subject: Re: PATCH: [for consideration] TMPSUFFIX
> }
> } This looks like a race condition: if another process creates the
> } filename that mktemp() returned before the open(O_EXCL) call [which will
> } therefore fail], zsh will remove that filename even though zsh didn't
> } create it.
> 
> If another process creates the filename that mktemp() returned, we have
> a much larger problem -- the whole point is that it should be impossible
> for another process to create that name first.
> 

That is not impossible; it is only improbable.  That's why O_EXCL is
passed to the open() call that receives the name returned by mktemp().

It would be impossible if the code used mkstemp().

> } [This could trigger through a deliberate attack or through
> } a race condition between two zsh instances calling =(:) concurrently.]
> 
> It definitely CANNOT occur because of two zsh instances using =().  I

In the time window between mktemp() returning and open() called,
a second zsh process could call mktemp() and be returned the same value
that was returned in the first process, couldn't it?

I think it's more likely that a name of the form ${TMPPREFIX}XXXXXX
would be created by another zsh process that has the same $TMPPREFIX
setting than by an attacker.

> won't assert the attack is 100% impossible, but there's nothing more
> we can do about that than we already have.
> 

Yes, there is: we should stop calling addfilelist(nam) if open(nam)
returns negative.

(Sidebar: that mktemp() call is the only warning in my build; it'd be
nice to get rid of it while we're here.)

> So mostly we're concerned with $TMPDIR being unwritable, or full file
> system, etc. -- but of course failure for any other reason including
> attacker-duplicated file name would get swept up if we change the way
> failure is handled.
> 

Agreed, all failure reasons can be handled the same way.

> } I don't get "the caller will create the file".  [...]
> } (The caller can call getoutputfile() again if he wishes.)
> 
> The SHELL SCRIPT caller, not the C code caller.  That is, as currently
> coded, =(...) will substitute a file name whether or not it actually got
> created.  In a case like
> 
>     () { ... do something with $1 ... } =(contents)
> 
> the name in $1 could be used in output redirections, etc.  As written,
> exec.c seems intended to allow this.
> 

I checked 'blame' to find the original motivation for that, but the
lines in question predate the import to git.

> } How is it helpful to return a filename, which may or may not exist and
> } certainly doesn't contain the output of 'cmd'? It seems to me erroring
> } out would be better.
> 
> It'd have to be an error on the same order as "bad substitution" so the
> whole command context fails.  Hence bringing it up for discussion.

I don't know what's best here.

I'm quite sure that if open() fails, we mustn't pass the filename we
tried to code that expects to receive a newly-created filename, since
that exposes us to a symlink attack.

Using ERRFLAG_ERROR risks aborting too much code (39154 being a recent
example).

A middle way would be to force the simple command that =(:) was part to
return 127.  When =(:) is used a an argument to an anonymous function,
that would prevent the function's body from being executed with an
invalid filename argument, which is good... but nobody checks the exit
code of such anonymous functions, so code _after_ the anonymous function
might break because it assumes the function was executed.

Maybe there's some simple solution that I'm overlooking.  (And with any
luck, it will appear in my -workers@ mailbox in the near future ;-).)

Cheers,

Daniel

Re: PATCH: [for consideration] TMPSUFFIX

[Bart Schaefer]

On Sep 27,  7:00am, Daniel Shahaf wrote:
}
} > won't assert the attack is 100% impossible, but there's nothing more
} > we can do about that than we already have.
} 
} Yes, there is: we should stop calling addfilelist(nam) if open(nam)
} returns negative.

That's not the "that" I was talking about.

} (Sidebar: that mktemp() call is the only warning in my build; it'd be
} nice to get rid of it while we're here.)

zsh-workers had this conversation years ago and landed where we are now
(except I don't think this bit about =() returning the name even if it
wasn't opened was noticed/considered).

} > It'd have to be an error on the same order as "bad substitution" so the
} > whole command context fails.  Hence bringing it up for discussion.
} 
} I don't know what's best here.
} 
} Using ERRFLAG_ERROR risks aborting too much code (39154 being a recent
} example).

I'm not sure that's really a valid example for this case -- the trouble
there was not that too much code was aborted, rather that because of a
surrounding use of "eval" not *enough* code was aborted; a scripting
error.

} A middle way would be to force the simple command that =(:) was part to
} return 127.

If we were to parallel other redirection errors, the command should
just return 1.  Otherwise I'd say we should report the actual error
number instead of always 127.  However, I don't see any obvious way
to do either of those things from inside stringsubst().

The code in stringsubst() expects getoutputfile() to return NULL and
then substitutes nothing as the replacement, which could change the
number of arguments in the command -- so that isn't ideal either.

We could just call zwarn() [instead of zerr()] and return "/dev/null"
to supply an empty file as if the command inside the =() had failed,
but I'm more inclined to just call zerr() and allow things to abort.

This obviously isn't a very common occurrence or we'd have seen it come
up before; this is hardly an obscure or little-used feature.

Re: PATCH: [for consideration] TMPSUFFIX

[Daniel Shahaf]

Bart Schaefer wrote on Tue, Sep 27, 2016 at 12:20:50 -0700:
> On Sep 27,  7:00am, Daniel Shahaf wrote:
> } Using ERRFLAG_ERROR risks aborting too much code (39154 being a recent
> } example).
> 
> I'm not sure that's really a valid example for this case -- the trouble
> there was not that too much code was aborted, rather that because of a
> surrounding use of "eval" not *enough* code was aborted; a scripting
> error.

What can we do to cause enough code to be aborted in that case?  Perhaps
something like the following:

[[[
diff --git a/Completion/Base/Core/_dispatch b/Completion/Base/Core/_dispatch
index 3f6fe5b..8c5220f 100644
--- a/Completion/Base/Core/_dispatch
+++ b/Completion/Base/Core/_dispatch
@@ -60,7 +60,13 @@ done
 
 if [[ -n "$comp" && "$name" != "${argv[-1]}" ]]; then
   _compskip=patterns
-  eval "$comp" && ret=0
+  {
+    eval "$comp" && ret=0
+  } always {
+    if (( TRY_BLOCK_ERROR )); then
+      ... throw the exception ... 
+    fi
+  }
   [[ "$_compskip" = (all|*patterns*) ]] && return ret
 fi
 
]]]

where the ellipsis throws the exception that occurred in the 'eval'.

(Sidebar: if errflag ever had the ERRFLAG_HARD bit set but the
ERRFLAG_ERROR bit unset, then $TRY_BLOCK_ERROR would be zero; this can't
currently occur because ERRFLAG_HARD always occurs alongside ERRFLAG_ERROR..)

> } A middle way would be to force the simple command that =(:) was part to
> } return 127.
> 
> If we were to parallel other redirection errors, the command should
> just return 1.  Otherwise I'd say we should report the actual error
> number instead of always 127. 

Agreed.  (My bad for being unclear: I said "127" but I meant "non-zero".)

> However, I don't see any obvious way to do either of those things from
> inside stringsubst().

_dispatch (was Re: PATCH: [for consideration] TMPSUFFIX)

[Bart Schaefer]

On Sep 28, 10:24am, Daniel Shahaf wrote:
} Subject: Re: PATCH: [for consideration] TMPSUFFIX
}
} Bart Schaefer wrote on Tue, Sep 27, 2016 at 12:20:50 -0700:
} > there was not that too much code was aborted, rather that because of a
} > surrounding use of "eval" not *enough* code was aborted; a scripting
} > error.
} 
} What can we do to cause enough code to be aborted in that case?  Perhaps
} something like the following:
} 
} +  {
} +    eval "$comp" && ret=0
} +  } always {
} +    if (( TRY_BLOCK_ERROR )); then
} +      ... throw the exception ... 
} +    fi
} +  }

Sadly, that won't do it either -- TRY_BLOCK_ERROR is not set:

[[[
torch% comp=': ${:}'
torch% { eval "$comp" && print OK } always { print $TRY_BLOCK_ERROR }
zsh: unrecognized modifier
0
torch% print $?
1
torch% comp=': *(#q)(#q)'                                           
torch% { eval "$comp" && echo OK } always { print $TRY_BLOCK_ERROR }
zsh: unknown file attribute: #
0
]]]

The eval captures everything and turns it into nonzero exit status.

The eval is there (and in two other places in _dispatch) because one
can pass a command line as the first argument of compadd:

    compdef 'compadd *' f

There might be another way to permit that without eval'ing, but it
would be convoluted.

Re: _dispatch (was Re: PATCH: [for consideration] TMPSUFFIX)

[Daniel Shahaf]

Bart Schaefer wrote on Wed, Sep 28, 2016 at 11:49:17 -0700:
> } +  {
> } +    eval "$comp" && ret=0
> } +  } always {
> } +    if (( TRY_BLOCK_ERROR )); then
> } +      ... throw the exception ... 
> } +    fi
> } +  }
> 
> Sadly, that won't do it either -- TRY_BLOCK_ERROR is not set:
> 
> The eval captures everything and turns it into nonzero exit status.
> 
⋮
>     compdef 'compadd *' f
> 
> There might be another way to permit that without eval'ing, but it
> would be convoluted.

It might be easier to just give a flag to eval that means "propagate
errflag of the evaluated expression to the calling scope"?  So the eval
itself would be aborted/unrolled, too.

Re: _dispatch (was Re: PATCH: [for consideration] TMPSUFFIX)

[Bart Schaefer]

On Sep 29,  6:39am, Daniel Shahaf wrote:
}
} It might be easier to just give a flag to eval that means "propagate
} errflag of the evaluated expression to the calling scope"?  So the eval
} itself would be aborted/unrolled, too.

I don't think this is a significant enough problem to warrant changing
the definition of "eval" (if by "give a flag" you mean "add a -e option"
for some variant of -e).

However, if it were possible for the "eval" builtin to know when it
was inside an "always" construct and propagate a new ERRFLAG_EVAL
out to where TRY_BLOCK_ERROR could reflect it, that might work.
The important bit would be that ERRFLAG_EVAL never converts directly
back into ERRFLAG_ERROR, so if the script ignores TRY_BLOCK_ERROR
then all errors disappear at the end of the always-block.

Or something like that.

Re: _dispatch (was Re: PATCH: [for consideration] TMPSUFFIX)

[Daniel Shahaf]

Bart Schaefer wrote on Thu, Sep 29, 2016 at 00:30:47 -0700:
> On Sep 29,  6:39am, Daniel Shahaf wrote:
> }
> } It might be easier to just give a flag to eval that means "propagate
> } errflag of the evaluated expression to the calling scope"?  So the eval
> } itself would be aborted/unrolled, too.
> 
> I don't think this is a significant enough problem to warrant changing
> the definition of "eval" (if by "give a flag" you mean "add a -e option"
> for some variant of -e).
> 

Yes, that's what I meant.  In terms if your idea, I think my idea was to
have ERRFLAG_EVAL automatically convert back to ERRFLAG_ERROR iff -e was
passed to eval — i.e., not by default.

> However, if it were possible for the "eval" builtin to know when it
> was inside an "always" construct and propagate a new ERRFLAG_EVAL
> out to where TRY_BLOCK_ERROR could reflect it, that might work.
> The important bit would be that ERRFLAG_EVAL never converts directly
> back into ERRFLAG_ERROR, so if the script ignores TRY_BLOCK_ERROR
> then all errors disappear at the end of the always-block.
> 
> Or something like that.

This makes sense, but wouldn't it also require some way for the always
block to (manually) set ERRFLAG_ERROR again upon an ERRFLAG_EVAL, in
order to "abort enough code" (which was the original issue)?

The interface could be a new builtin, or a setfn on TRY_BLOCK_ERROR; the
important thing is that then the always block finishes ERRFLAG_ERROR
would be set.

Cheers,

Daniel

Re: _dispatch (was Re: PATCH: [for consideration] TMPSUFFIX)

[Bart Schaefer]

On Sep 30,  7:03am, Daniel Shahaf wrote:
} Subject: Re: _dispatch (was Re: PATCH: [for consideration] TMPSUFFIX)
}
} Bart Schaefer wrote on Thu, Sep 29, 2016 at 00:30:47 -0700:
} > The important bit would be that ERRFLAG_EVAL never converts directly
} > back into ERRFLAG_ERROR, so if the script ignores TRY_BLOCK_ERROR
} > then all errors disappear at the end of the always-block.
} 
} This makes sense, but wouldn't it also require some way for the always
} block to (manually) set ERRFLAG_ERROR again upon an ERRFLAG_EVAL, in
} order to "abort enough code" (which was the original issue)?
} 
} The interface could be [...] a setfn on TRY_BLOCK_ERROR

That's what I was thinking of, yes.  There's already a setfn that
reacts to TRY_BLOCK_ERROR=0 so it wouldn't be hard to have one that
notices when TRY_BLOCK_ERROR changes from 2 to 1 or some such.

I'm not sure what kind of gyrations would be required for eval to be
aware that it's inside a try-block, though.

Re: PATCH: [for consideration] TMPSUFFIX

[Sebastian Gniazdowski]

On 26 September 2016 at 00:51, Bart Schaefer <schaefer@brasslantern.com> wrote:
> The signal queuing in utils.c was just something I noticed while looking
> for the right place to put the TMPSUFFIX code.
>
> The addfilelist() is done whether or not we successfully opened the file,
> because (a) it was that way before and (b) the name is returned either
> way, so the caller might try to create the file itself.  However, it
> seems a bit odd that the failure of open() is mostly ignored?

So I will be pasting the following to command line:

() { local TMPSUFFIX=.md; vim =( git -C /homse/user/github/proj.git
cat-file blob bb896f1d9350a95b0494508e671b8452933069a7 ) } # README.md

Seems fair enough to me.

Best regards,
Sebastian Gniazdowski