[PATCH] wrapper: simplify xmkstemp()

[PATCH] wrapper: simplify xmkstemp()

[René Scharfe]

Call xmkstemp_mode() instead of duplicating its error handling code.
This switches the implementation from the system's mkstemp(3) to our own
git_mkstemp_mode(), which works just as well.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 wrapper.c | 19 +------------------
 1 file changed, 1 insertion(+), 18 deletions(-)

diff --git a/wrapper.c b/wrapper.c
index 3d507d4204..d5976b3e7e 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -421,24 +421,7 @@ FILE *fopen_or_warn(const char *path, const char *mode)
 
 int xmkstemp(char *filename_template)
 {
-	int fd;
-	char origtemplate[PATH_MAX];
-	strlcpy(origtemplate, filename_template, sizeof(origtemplate));
-
-	fd = mkstemp(filename_template);
-	if (fd < 0) {
-		int saved_errno = errno;
-		const char *nonrelative_template;
-
-		if (strlen(filename_template) != strlen(origtemplate))
-			filename_template = origtemplate;
-
-		nonrelative_template = absolute_path(filename_template);
-		errno = saved_errno;
-		die_errno("Unable to create temporary file '%s'",
-			nonrelative_template);
-	}
-	return fd;
+	return xmkstemp_mode(filename_template, 0600);
 }
 
 /* Adapted from libiberty's mkstemp.c. */
-- 
2.51.2

Re: [PATCH] wrapper: simplify xmkstemp()

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Call xmkstemp_mode() instead of duplicating its error handling code.
> This switches the implementation from the system's mkstemp(3) to our own
> git_mkstemp_mode(), which works just as well.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  wrapper.c | 19 +------------------
>  1 file changed, 1 insertion(+), 18 deletions(-)
>
> diff --git a/wrapper.c b/wrapper.c
> index 3d507d4204..d5976b3e7e 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -421,24 +421,7 @@ FILE *fopen_or_warn(const char *path, const char *mode)
>  
>  int xmkstemp(char *filename_template)
>  {
> -	int fd;
> -	char origtemplate[PATH_MAX];
> -	strlcpy(origtemplate, filename_template, sizeof(origtemplate));
> -
> -	fd = mkstemp(filename_template);
> -	if (fd < 0) {
> -		int saved_errno = errno;
> -		const char *nonrelative_template;
> -
> -		if (strlen(filename_template) != strlen(origtemplate))
> -			filename_template = origtemplate;
> -
> -		nonrelative_template = absolute_path(filename_template);
> -		errno = saved_errno;
> -		die_errno("Unable to create temporary file '%s'",
> -			nonrelative_template);
> -	}
> -	return fd;
> +	return xmkstemp_mode(filename_template, 0600);
>  }

A patch that loses lines is nice.  My curiosity wonders what the
strlen() comparison in the original was about, but let's not waste
our brain cycles to code that we no longer use ;-).  xmkstemp_mode()
checks if our git_mkstemp_mode() cleared the template[0] as a sign
to restore the origtemplate, and uses the template that was munged
by git_mkstemp_mode() and used to attempt opening it, which seems
very sensible.

Will queue.  Thanks.

Re: [PATCH] wrapper: simplify xmkstemp()

[Jeff King]

On Mon, Nov 17, 2025 at 01:52:53PM -0800, Junio C Hamano wrote:

> >  int xmkstemp(char *filename_template)
> >  {
> > -	int fd;
> > -	char origtemplate[PATH_MAX];
> > -	strlcpy(origtemplate, filename_template, sizeof(origtemplate));
> > -
> > -	fd = mkstemp(filename_template);
> > -	if (fd < 0) {
> > -		int saved_errno = errno;
> > -		const char *nonrelative_template;
> > -
> > -		if (strlen(filename_template) != strlen(origtemplate))
> > -			filename_template = origtemplate;
> > -
> > -		nonrelative_template = absolute_path(filename_template);
> > -		errno = saved_errno;
> > -		die_errno("Unable to create temporary file '%s'",
> > -			nonrelative_template);
> > -	}
> > -	return fd;
> > +	return xmkstemp_mode(filename_template, 0600);
> >  }
> 
> A patch that loses lines is nice.  My curiosity wonders what the
> strlen() comparison in the original was about, but let's not waste
> our brain cycles to code that we no longer use ;-).  xmkstemp_mode()
> checks if our git_mkstemp_mode() cleared the template[0] as a sign
> to restore the origtemplate, and uses the template that was munged
> by git_mkstemp_mode() and used to attempt opening it, which seems
> very sensible.

I agree that we do not need to worry about code that is going away,
but...it made me wonder if the reason for the strlen() applied equally
to git_mkstemp_mode(). I.e., if it has had a small bug for a long time,
and we are now about to expose it to a wider audience.

Looks like it comes from f7be59b477 (xmkstemp(): avoid showing truncated
template more carefully, 2012-12-18), where some implementations of
mkstemp() would truncate "foo/bar.XXXXX" as "foo\0". But since we are
now always using our own function, we know it truncates at the very
start. So we do not need to worry about that hack.


I also wondered if we ever use mkstemp() at all after this patch. If
not, we might want to declare it off-limits. Not because it is evil, but
because our own implementation is more predictable (and we can drop the
compat wrappers for mingw). It looks like there is one more call in
entry.c's open_output_fd(), but arguably that should be calling
xmkstemp() or git_mkstemp_mode(). But that's out of scope for this patch
(I just thought I might nerd-snipe René into looking at it).

-Peff

Re: [PATCH] wrapper: simplify xmkstemp()

[René Scharfe]

On 11/18/25 10:46 AM, Jeff King wrote:
> 
> I also wondered if we ever use mkstemp() at all after this patch. If
> not, we might want to declare it off-limits. Not because it is evil, but
> because our own implementation is more predictable (and we can drop the
> compat wrappers for mingw). It looks like there is one more call in
> entry.c's open_output_fd(), but arguably that should be calling
> xmkstemp() or git_mkstemp_mode(). But that's out of scope for this patch
> (I just thought I might nerd-snipe René into looking at it).
Thought about it before, but couldn't bring myself to ban mkstemp(3).
Its only faults are lack of features (mode setting and suffix support)
and not being available on Windows, but apart from that it does its
job as advertised.  Which means ... it doesn't cut it for us.  Hmm.

--- >8 ---
Subject: [PATCH] stop using mkstemp(3)

mkstemp(3) works fine if you don't need custom permissions, a specific
filename suffix or to run it on Windows.  For those cases we have a
custom implementation around git_mkstemps_mode().  Use it for the base
case as well, for consistency across platforms.

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 compat/mingw-posix.h | 1 -
 compat/mingw.c       | 5 -----
 git-compat-util.h    | 2 ++
 3 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/compat/mingw-posix.h b/compat/mingw-posix.h
index 631a208684..57915119c6 100644
--- a/compat/mingw-posix.h
+++ b/compat/mingw-posix.h
@@ -185,7 +185,6 @@ char *mingw_locate_in_PATH(const char *cmd);
 
 int pipe(int filedes[2]);
 unsigned int sleep (unsigned int seconds);
-int mkstemp(char *template);
 int gettimeofday(struct timeval *tv, void *tz);
 #ifndef __MINGW64_VERSION_MAJOR
 struct tm *gmtime_r(const time_t *timep, struct tm *result);
diff --git a/compat/mingw.c b/compat/mingw.c
index 736a07a028..dc3da7c6d5 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1174,11 +1174,6 @@ char *mingw_mktemp(char *template)
 	return template;
 }
 
-int mkstemp(char *template)
-{
-	return git_mkstemp_mode(template, 0600);
-}
-
 int gettimeofday(struct timeval *tv, void *tz UNUSED)
 {
 	FILETIME ft;
diff --git a/git-compat-util.h b/git-compat-util.h
index 398e0fac4f..0e6bd266cc 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -446,6 +446,8 @@ static inline int git_has_dir_sep(const char *path)
 
 #include "wrapper.h"
 
+#define mkstemp(template) git_mkstemp_mode((template), 0600)
+
 /* General helper functions */
 NORETURN void usage(const char *err);
 NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));
-- 
2.52.0

Re: [PATCH] wrapper: simplify xmkstemp()

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> On 11/18/25 10:46 AM, Jeff King wrote:
>> 
>> I also wondered if we ever use mkstemp() at all after this patch. If
>> not, we might want to declare it off-limits. Not because it is evil, but
>> because our own implementation is more predictable (and we can drop the
>> compat wrappers for mingw). It looks like there is one more call in
>> entry.c's open_output_fd(), but arguably that should be calling
>> xmkstemp() or git_mkstemp_mode(). But that's out of scope for this patch
>> (I just thought I might nerd-snipe René into looking at it).
> Thought about it before, but couldn't bring myself to ban mkstemp(3).
> Its only faults are lack of features (mode setting and suffix support)
> and not being available on Windows, but apart from that it does its
> job as advertised.  Which means ... it doesn't cut it for us.  Hmm.

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?"

 - the platform native one could be more performant?
 - the platform native one could be more secure?
 - using the platform native one, we can lose out custom code?

None of the ones I can come up with offhand sound very legitimate.

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.

> --- >8 ---
> Subject: [PATCH] stop using mkstemp(3)
>
> mkstemp(3) works fine if you don't need custom permissions, a specific
> filename suffix or to run it on Windows.  For those cases we have a
> custom implementation around git_mkstemps_mode().  Use it for the base
> case as well, for consistency across platforms.
>
> Suggested-by: Jeff King <peff@peff.net>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  compat/mingw-posix.h | 1 -
>  compat/mingw.c       | 5 -----
>  git-compat-util.h    | 2 ++
>  3 files changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/compat/mingw-posix.h b/compat/mingw-posix.h
> index 631a208684..57915119c6 100644
> --- a/compat/mingw-posix.h
> +++ b/compat/mingw-posix.h
> @@ -185,7 +185,6 @@ char *mingw_locate_in_PATH(const char *cmd);
>  
>  int pipe(int filedes[2]);
>  unsigned int sleep (unsigned int seconds);
> -int mkstemp(char *template);
>  int gettimeofday(struct timeval *tv, void *tz);
>  #ifndef __MINGW64_VERSION_MAJOR
>  struct tm *gmtime_r(const time_t *timep, struct tm *result);
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 736a07a028..dc3da7c6d5 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -1174,11 +1174,6 @@ char *mingw_mktemp(char *template)
>  	return template;
>  }
>  
> -int mkstemp(char *template)
> -{
> -	return git_mkstemp_mode(template, 0600);
> -}
> -
>  int gettimeofday(struct timeval *tv, void *tz UNUSED)
>  {
>  	FILETIME ft;
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 398e0fac4f..0e6bd266cc 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -446,6 +446,8 @@ static inline int git_has_dir_sep(const char *path)
>  
>  #include "wrapper.h"
>  
> +#define mkstemp(template) git_mkstemp_mode((template), 0600)
> +
>  /* General helper functions */
>  NORETURN void usage(const char *err);
>  NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));

Re: [PATCH] wrapper: simplify xmkstemp()

[Jeff King]

On Tue, Nov 18, 2025 at 03:08:31PM -0800, Junio C Hamano wrote:

> 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?"
> 
>  - the platform native one could be more performant?
>  - the platform native one could be more secure?
>  - using the platform native one, we can lose out custom code?
> 
> None of the ones I can come up with offhand sound very legitimate.
> 
> 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.

I think predictability cuts both ways. The system mkstemp() will behave
more like it does on the rest of that platform, but maybe less like Git
on other platforms. Using Git's implementation will be consistent across
platforms, but maybe inconsistent with the rest of the current platform.

I think the "consistent with the rest of the current platform" ship may
have already sailed, though. We already use our custom git_mkstemp_mode() on
every platform for most tempfiles. And now even those few xmkstemp()
calls will do so (after René's first patch).

My suggestion was mostly: if we are going to use custom code at all,
then let's at least do so always, and not ever use the system mkstemp().

> > diff --git a/git-compat-util.h b/git-compat-util.h
> > index 398e0fac4f..0e6bd266cc 100644
> > --- a/git-compat-util.h
> > +++ b/git-compat-util.h
> > @@ -446,6 +446,8 @@ static inline int git_has_dir_sep(const char *path)
> >  
> >  #include "wrapper.h"
> >  
> > +#define mkstemp(template) git_mkstemp_mode((template), 0600)

So this patch implements what I was thinking, though I probably would
have made it more explicit: add mkstemp() to the banned list (not
because it's evil but because it's unportable) and force callers to use
git_mkstemp_mode() explicitly.

-Peff

Re: [PATCH] wrapper: simplify xmkstemp()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> > +#define mkstemp(template) git_mkstemp_mode((template), 0600)
>
> So this patch implements what I was thinking, though I probably would
> have made it more explicit: add mkstemp() to the banned list (not
> because it's evil but because it's unportable) and force callers to use
> git_mkstemp_mode() explicitly.

Because only a very small number (one?)  of callers call mkstemp()
in the current code, the above is probably a good thing to do.

Re: [PATCH] wrapper: simplify xmkstemp()

[René Scharfe]

On 11/20/25 3:39 PM, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
>>>> +#define mkstemp(template) git_mkstemp_mode((template), 0600)
>>
>> So this patch implements what I was thinking, though I probably would
>> have made it more explicit: add mkstemp() to the banned list (not
>> because it's evil but because it's unportable) and force callers to use
>> git_mkstemp_mode() explicitly.
> 
> Because only a very small number (one?)  of callers call mkstemp()
> in the current code, the above is probably a good thing to do.

True, banning mkstemp(3) instead of overriding it would simplify the
code by removing one layer of indirection, and the hassle of no longer
being able to use that standard function would be low.

René

Re: [PATCH] wrapper: simplify xmkstemp()

[René Scharfe]

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