Re: [PATCH v3 2/2] wrapper: use a CSPRNG to generate random file names

["René Scharfe" <l.s.r@web.de>]

Am 18.01.22 um 18:25 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>>> This series really feels like it's adding too much complexity and
>>> potential auditing headaches (distributors worrying about us shipping a
>>> CSPRNG, having to audit it) to a low-level codepath that most of the
>>> time won't need this at all.
>>
>> Good point.  Please let me think out loud for a moment.
>
> Yeah, I agree you and Ævar that the topic may be over-engineering
> the solution for problem that we shouldn't be the ones who solve.
>
> I agree with your analysis that the "diff" tempfiles do need suffix,
> we SHOULD create them in $TMPDIR (not in the working tree or
> $GIT_DIR) to support operation in a read-only repository, but we can
> create a unique temporary directory and place a file (even under its
> original name) in it as a workaround.

I forgot one crucial aspect, though: umask.  The "m" variants of the
tempfile functions set a mode, with umask applied.  git_mkstemps_mode()
does that by passing the mode to open(2), which applies the umask
internally.  Neither mkstemp(3) nor chmod(2) do that for us, so a
replacement needs to call umask(2) to get it and again to restore it,
which requires two system calls and is racy if multiple threads do this.

Diff doesn't need a custom mode, so we can still use mkdtemp() there and
thus make the suffix feature of git_mkstemps_mode() unnecessary.  But
a replacement for git_mkstemp_mode() with two umask(2) calls looks less
attractive to me than fortifying git_mkstemps_mode() with a good source
of randomness.

René