Re: [PATCH] wrapper: simplify xmkstemp()

From: René Scharfe <l.s.r@web.de>
Date: Nov 22, 2025, 13:24 UTC
Message-ID: <fd7023d3-c9a8-4828-b1a2-ac5a1459cbda@web.de>
In-Reply-To: <xmqqqztvc51s.fsf@gitster.g>

On 11/19/25 12:08 AM, Junio C Hamano wrote:

Show 9 quoted lines

> René Scharfe <l.s.r@web.de> writes:
> 
> When somebody asks:
> 
>     On this and that platforms, mkstemp() is natively available.
>     Why are we using git_mkstemp_mode() instead?
> 
> after seeing this patch, I am tempted to say "Why not?"  Are there
> legitimate answers to my "What not?"

My reluctance to let go of mkstemp(3) is seeping through. I like that function. Let's see if I can get over it.

>  - the platform native one could be more performant?

Good question. A platform could use non-portable tricks like using a particularly cheap source of randomness or a system call that creates a file with the next available name matching a prefix. As there any that do, though? We can find out by measuring, patch below.

On macOS 26.1 I get:

$ hyperfine -w3 -C 'rm /tmp/tmp_*' -L fn mkstemp,git_mkstemp_mode "t/helper/test-tool mktemp -n 1000 -f {fn} /tmp/tmp_XXXXXX"
Benchmark 1: t/helper/test-tool mktemp -n 1000 -f mkstemp /tmp/tmp_XXXXXX
  Time (mean ± σ):      56.7 ms ±   0.4 ms    [User: 3.7 ms, System: 52.5 ms]
  Range (min … max):    56.0 ms …  57.3 ms    23 runs

Benchmark 2: t/helper/test-tool mktemp -n 1000 -f git_mkstemp_mode /tmp/tmp_XXXXXX
  Time (mean ± σ):      54.2 ms ±   0.4 ms    [User: 3.1 ms, System: 50.7 ms]
  Range (min … max):    53.7 ms …  54.9 ms    24 runs

Summary
  t/helper/test-tool mktemp -n 1000 -f git_mkstemp_mode /tmp/tmp_XXXXXX ran
    1.05 ± 0.01 times faster than t/helper/test-tool mktemp -n 1000 -f mkstemp /tmp/tmp_XXXXXX

Weird.  How can mkstemp(3) burn measurably more system cycles?

>  - the platform native one could be more secure?

It could if it works deterministically, or uses a better source of randomness, or our code contains an exploitable flaw.

>  - using the platform native one, we can lose out custom code?

Not much unless we're willing to give up performance on those platforms for the cases where we want to set the file mode. There is mkstemps(3) for allowing a suffix and mkostemps(3) for also allowing to set open(2) flags, but I couldn't find an equivalent of git_mkstemp_mode().

A replacement could look like this:

int git_mkstemp_mode(char *pattern, int mode)
{
	int fd = mkstemp(pattern);
	if (fd >= 0 && mode != 0600)
		fchmod(fd, mode);
	return fd;
}

That's basically what happens if we give the test helper a non-default mode value. Unsurprisingly the extra fchmod(2) call has a significant cost:

$ hyperfine -w3 -C 'rm /tmp/tmp_*' -L fn mkstemp,git_mkstemp_mode "t/helper/test-tool mktemp -n 1000 -m 0700 -f {fn} /tmp/tmp_XXXXXX"
Benchmark 1: t/helper/test-tool mktemp -n 1000 -m 0700 -f mkstemp /tmp/tmp_XXXXXX
  Time (mean ± σ):      67.2 ms ±   0.4 ms    [User: 3.9 ms, System: 62.9 ms]
  Range (min … max):    66.6 ms …  68.1 ms    22 runs

Benchmark 2: t/helper/test-tool mktemp -n 1000 -m 0700 -f git_mkstemp_mode /tmp/tmp_XXXXXX
  Time (mean ± σ):      54.3 ms ±   0.7 ms    [User: 3.1 ms, System: 50.6 ms]
  Range (min … max):    53.5 ms …  57.1 ms    24 runs

Summary
  t/helper/test-tool mktemp -n 1000 -m 0700 -f git_mkstemp_mode /tmp/tmp_XXXXXX ran
    1.24 ± 0.02 times faster than t/helper/test-tool mktemp -n 1000 -m 0700 -f mkstemp /tmp/tmp_XXXXXX

If we keep the mode-setting variant anyway, the rest is just a bunch of trivial wrappers that cannot possibly add a performance problem and are easy to check for security issues.

> One upside might be that doing so would make the behaviour more
> predictable, in that even on a platform with native mkstemp(), we
> would use the same implementation as what we use on Windows.  But
> I do not know how much upside it is in practice, either.

Using a single implementation everywhere is easier to code, test and maintain. A flaw in it would have a bigger blast radius, though.

Can we depend on git_mkstemps_mode() and co.? We already do. Can we depend on them in cases where we don't need a suffix and mode 0600 suffixes? I don't see why we can't.

René

--- >8 ---
Subject: [PATCH] test-mktemp: allow testing mkstemp(3) and git_mkstemp_mode()

Allow testing two more functions for creating temporary files by using parseopt to provide options for selecting them.

Allow specifying custom file permissions to exercise git_mkstemp_mode() fully.

Also add an option for calling the selected function a number of times, which allows for easier performance tests.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 t/helper/test-mktemp.c | 73 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 70 insertions(+), 3 deletions(-)

diff --git a/t/helper/test-mktemp.c b/t/helper/test-mktemp.c
index 2290688940..9b8bc95683 100644
--- a/t/helper/test-mktemp.c
+++ b/t/helper/test-mktemp.c
@@ -3,13 +3,80 @@
  */
 #include "test-tool.h"
 #include "git-compat-util.h"
+#include "parse-options.h"
+#include "strbuf.h"
+
+static char const * const test_mktemp_usage[] = {
+	N_("test-tool mktemp [options] <template>"),
+	NULL
+};
+
+static int test_xmkstemp(char *template, int mode)
+{
+	int fd = xmkstemp(template);
+	if (mode != 0600)
+		fchmod(fd, mode);
+	return fd;
+}
+
+static int test_mkstemp(char *template, int mode)
+{
+	int fd = mkstemp(template);
+	if (fd < -1)
+		die_errno(_("unable to create temporary file '%s'"), template);
+	if (mode != 0600)
+		fchmod(fd, mode);
+	return fd;
+}
+
+static int test_git_mkstemp_mode(char *template, int mode)
+{
+	int fd = git_mkstemp_mode(template, mode);
+	if (fd < -1)
+		die_errno(_("unable to create temporary file '%s'"), template);
+	return fd;
+}
 
 int cmd__mktemp(int argc, const char **argv)
 {
-	if (argc != 2)
-		usage("Expected 1 parameter defining the temporary file template");
+	struct strbuf template = STRBUF_INIT;
+	const char *function_name = "xmkstemp";
+	int mode = 0600;
+	int count = 1;
+	struct option options[] = {
+		OPT_STRING_F('f', "function", &function_name, "name",
+			     N_("select the function to call"),
+			     PARSE_OPT_NONEG),
+		OPT_INTEGER('m', "mode", &mode,
+			    N_("specify file permission bits")),
+		OPT_INTEGER('n', "count", &count,
+			    N_("specify the number of files")),
+		OPT_END()
+	};
+	int (*fn)(char *, int);
+
+	argc = parse_options(argc, argv, NULL, options,
+			     test_mktemp_usage, 0);
+
+	if (argc != 1)
+		usage_with_options(test_mktemp_usage, options);
+
+	if (!strcmp(function_name, "xmkstemp"))
+		fn = test_xmkstemp;
+	else if (!strcmp(function_name, "mkstemp"))
+		fn = test_mkstemp;
+	else if (!strcmp(function_name, "git_mkstemp_mode"))
+		fn = test_git_mkstemp_mode;
+	else
+		die(_("unsupported function: %s"), function_name);
+
+	for (int i = 0; i < count; i++) {
+		strbuf_reset(&template);
+		strbuf_addstr(&template, argv[0]);
+		close(fn(template.buf, mode));
+	}
 
-	xmkstemp(xstrdup(argv[1]));
+	strbuf_release(&template);
 
 	return 0;
 }

-- 
2.52.0