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

From: René Scharfe <l.s.r@web.de>
Date: Jan 22, 2022, 10:41 UTC
Message-ID: <f31dd12e-6d27-a070-77e4-ad53fb319735@web.de>
In-Reply-To: <xmqq7daxt0pk.fsf@gitster.g>

Am 18.01.22 um 18:25 schrieb Junio C Hamano:

Show 17 quoted lines

> 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é