Re: PATCH: [for consideration] TMPSUFFIX

[Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx>]

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.