From: "René Scharfe" <l.s.r@web•de>
To: Junio C Hamano <gitster@pobox•com>
Cc: Jeff King <peff@peff•net>, Git List <git@vger•kernel.org>
Subject: Re: [PATCH] wrapper: simplify xmkstemp()
Date: Sat, 22 Nov 2025 14:24:58 +0100 [thread overview]
Message-ID: <fd7023d3-c9a8-4828-b1a2-ac5a1459cbda@web.de> (raw)
In-Reply-To: <xmqqqztvc51s.fsf@gitster.g>
On 11/19/25 12:08 AM, Junio C Hamano wrote:
> 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
prev parent reply other threads:[~2025-11-22 13:25 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-11-17 19:42 [PATCH] wrapper: simplify xmkstemp() René Scharfe
2025-11-17 21:52 ` Junio C Hamano
2025-11-18 9:46 ` Jeff King
2025-11-18 22:29 ` René Scharfe
2025-11-18 23:08 ` Junio C Hamano
2025-11-20 8:23 ` Jeff King
2025-11-20 14:39 ` Junio C Hamano
2025-11-22 13:29 ` René Scharfe
2025-11-22 13:24 ` René Scharfe [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=fd7023d3-c9a8-4828-b1a2-ac5a1459cbda@web.de \
--to=l.s.r@web$(echo .)de \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=peff@peff$(echo .)net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox