{"thread":{"id":"64495","subject":"[PATCH] wrapper: simplify xmkstemp()","startedAt":"2025-11-17T19:42:57Z","lastAt":"2025-11-22T13:29:57Z","messageCount":9,"participants":["René Scharfe","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"530832","messageId":"058c5722-30f5-4bc5-90f5-24e4c6f3ff8f@web.de","threadId":"64495","inReplyTo":null,"subject":"[PATCH] wrapper: simplify xmkstemp()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-11-17T19:42:55Z","receivedAt":"2025-11-17T19:42:57Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Call xmkstemp_mode() instead of duplicating its error handling code.\nThis switches the implementation from the system's mkstemp(3) to our own\ngit_mkstemp_mode(), which works just as well.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n wrapper.c | 19 +------------------\n 1 file changed, 1 insertion(+), 18 deletions(-)\n\ndiff --git a/wrapper.c b/wrapper.c\nindex 3d507d4204..d5976b3e7e 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -421,24 +421,7 @@ FILE *fopen_or_warn(const char *path, const char *mode)\n \n int xmkstemp(char *filename_template)\n {\n-\tint fd;\n-\tchar origtemplate[PATH_MAX];\n-\tstrlcpy(origtemplate, filename_template, sizeof(origtemplate));\n-\n-\tfd = mkstemp(filename_template);\n-\tif (fd < 0) {\n-\t\tint saved_errno = errno;\n-\t\tconst char *nonrelative_template;\n-\n-\t\tif (strlen(filename_template) != strlen(origtemplate))\n-\t\t\tfilename_template = origtemplate;\n-\n-\t\tnonrelative_template = absolute_path(filename_template);\n-\t\terrno = saved_errno;\n-\t\tdie_errno(\"Unable to create temporary file '%s'\",\n-\t\t\tnonrelative_template);\n-\t}\n-\treturn fd;\n+\treturn xmkstemp_mode(filename_template, 0600);\n }\n \n /* Adapted from libiberty's mkstemp.c. */\n-- \n2.51.2\n"},{"id":"530839","messageId":"xmqqbjl0iax6.fsf@gitster.g","threadId":"64495","inReplyTo":"058c5722-30f5-4bc5-90f5-24e4c6f3ff8f@web.de","subject":"Re: [PATCH] wrapper: simplify xmkstemp()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-17T21:52:53Z","receivedAt":"2025-11-17T21:52:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Call xmkstemp_mode() instead of duplicating its error handling code.\n> This switches the implementation from the system's mkstemp(3) to our own\n> git_mkstemp_mode(), which works just as well.\n>\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n>  wrapper.c | 19 +------------------\n>  1 file changed, 1 insertion(+), 18 deletions(-)\n>\n> diff --git a/wrapper.c b/wrapper.c\n> index 3d507d4204..d5976b3e7e 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -421,24 +421,7 @@ FILE *fopen_or_warn(const char *path, const char *mode)\n>  \n>  int xmkstemp(char *filename_template)\n>  {\n> -\tint fd;\n> -\tchar origtemplate[PATH_MAX];\n> -\tstrlcpy(origtemplate, filename_template, sizeof(origtemplate));\n> -\n> -\tfd = mkstemp(filename_template);\n> -\tif (fd < 0) {\n> -\t\tint saved_errno = errno;\n> -\t\tconst char *nonrelative_template;\n> -\n> -\t\tif (strlen(filename_template) != strlen(origtemplate))\n> -\t\t\tfilename_template = origtemplate;\n> -\n> -\t\tnonrelative_template = absolute_path(filename_template);\n> -\t\terrno = saved_errno;\n> -\t\tdie_errno(\"Unable to create temporary file '%s'\",\n> -\t\t\tnonrelative_template);\n> -\t}\n> -\treturn fd;\n> +\treturn xmkstemp_mode(filename_template, 0600);\n>  }\n\nA patch that loses lines is nice.  My curiosity wonders what the\nstrlen() comparison in the original was about, but let's not waste\nour brain cycles to code that we no longer use ;-).  xmkstemp_mode()\nchecks if our git_mkstemp_mode() cleared the template[0] as a sign\nto restore the origtemplate, and uses the template that was munged\nby git_mkstemp_mode() and used to attempt opening it, which seems\nvery sensible.\n\nWill queue.  Thanks.\n"},{"id":"530893","messageId":"20251118094621.GB530545@coredump.intra.peff.net","threadId":"64495","inReplyTo":"xmqqbjl0iax6.fsf@gitster.g","subject":"Re: [PATCH] wrapper: simplify xmkstemp()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-18T09:46:21Z","receivedAt":"2025-11-18T09:46:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 17, 2025 at 01:52:53PM -0800, Junio C Hamano wrote:\n\n> >  int xmkstemp(char *filename_template)\n> >  {\n> > -\tint fd;\n> > -\tchar origtemplate[PATH_MAX];\n> > -\tstrlcpy(origtemplate, filename_template, sizeof(origtemplate));\n> > -\n> > -\tfd = mkstemp(filename_template);\n> > -\tif (fd < 0) {\n> > -\t\tint saved_errno = errno;\n> > -\t\tconst char *nonrelative_template;\n> > -\n> > -\t\tif (strlen(filename_template) != strlen(origtemplate))\n> > -\t\t\tfilename_template = origtemplate;\n> > -\n> > -\t\tnonrelative_template = absolute_path(filename_template);\n> > -\t\terrno = saved_errno;\n> > -\t\tdie_errno(\"Unable to create temporary file '%s'\",\n> > -\t\t\tnonrelative_template);\n> > -\t}\n> > -\treturn fd;\n> > +\treturn xmkstemp_mode(filename_template, 0600);\n> >  }\n> \n> A patch that loses lines is nice.  My curiosity wonders what the\n> strlen() comparison in the original was about, but let's not waste\n> our brain cycles to code that we no longer use ;-).  xmkstemp_mode()\n> checks if our git_mkstemp_mode() cleared the template[0] as a sign\n> to restore the origtemplate, and uses the template that was munged\n> by git_mkstemp_mode() and used to attempt opening it, which seems\n> very sensible.\n\nI agree that we do not need to worry about code that is going away,\nbut...it made me wonder if the reason for the strlen() applied equally\nto git_mkstemp_mode(). I.e., if it has had a small bug for a long time,\nand we are now about to expose it to a wider audience.\n\nLooks like it comes from f7be59b477 (xmkstemp(): avoid showing truncated\ntemplate more carefully, 2012-12-18), where some implementations of\nmkstemp() would truncate \"foo/bar.XXXXX\" as \"foo\\0\". But since we are\nnow always using our own function, we know it truncates at the very\nstart. So we do not need to worry about that hack.\n\n\nI also wondered if we ever use mkstemp() at all after this patch. If\nnot, we might want to declare it off-limits. Not because it is evil, but\nbecause our own implementation is more predictable (and we can drop the\ncompat wrappers for mingw). It looks like there is one more call in\nentry.c's open_output_fd(), but arguably that should be calling\nxmkstemp() or git_mkstemp_mode(). But that's out of scope for this patch\n(I just thought I might nerd-snipe René into looking at it).\n\n-Peff\n"},{"id":"530939","messageId":"3b1cb53a-6427-4626-a768-1961e25514f8@web.de","threadId":"64495","inReplyTo":"20251118094621.GB530545@coredump.intra.peff.net","subject":"Re: [PATCH] wrapper: simplify xmkstemp()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-11-18T22:29:38Z","receivedAt":"2025-11-18T22:35:01Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"On 11/18/25 10:46 AM, Jeff King wrote:\n> \n> I also wondered if we ever use mkstemp() at all after this patch. If\n> not, we might want to declare it off-limits. Not because it is evil, but\n> because our own implementation is more predictable (and we can drop the\n> compat wrappers for mingw). It looks like there is one more call in\n> entry.c's open_output_fd(), but arguably that should be calling\n> xmkstemp() or git_mkstemp_mode(). But that's out of scope for this patch\n> (I just thought I might nerd-snipe René into looking at it).\nThought about it before, but couldn't bring myself to ban mkstemp(3).\nIts only faults are lack of features (mode setting and suffix support)\nand not being available on Windows, but apart from that it does its\njob as advertised.  Which means ... it doesn't cut it for us.  Hmm.\n\n--- >8 ---\nSubject: [PATCH] stop using mkstemp(3)\n\nmkstemp(3) works fine if you don't need custom permissions, a specific\nfilename suffix or to run it on Windows.  For those cases we have a\ncustom implementation around git_mkstemps_mode().  Use it for the base\ncase as well, for consistency across platforms.\n\nSuggested-by: Jeff King <peff@peff.net>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n compat/mingw-posix.h | 1 -\n compat/mingw.c       | 5 -----\n git-compat-util.h    | 2 ++\n 3 files changed, 2 insertions(+), 6 deletions(-)\n\ndiff --git a/compat/mingw-posix.h b/compat/mingw-posix.h\nindex 631a208684..57915119c6 100644\n--- a/compat/mingw-posix.h\n+++ b/compat/mingw-posix.h\n@@ -185,7 +185,6 @@ char *mingw_locate_in_PATH(const char *cmd);\n \n int pipe(int filedes[2]);\n unsigned int sleep (unsigned int seconds);\n-int mkstemp(char *template);\n int gettimeofday(struct timeval *tv, void *tz);\n #ifndef __MINGW64_VERSION_MAJOR\n struct tm *gmtime_r(const time_t *timep, struct tm *result);\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 736a07a028..dc3da7c6d5 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -1174,11 +1174,6 @@ char *mingw_mktemp(char *template)\n \treturn template;\n }\n \n-int mkstemp(char *template)\n-{\n-\treturn git_mkstemp_mode(template, 0600);\n-}\n-\n int gettimeofday(struct timeval *tv, void *tz UNUSED)\n {\n \tFILETIME ft;\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 398e0fac4f..0e6bd266cc 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -446,6 +446,8 @@ static inline int git_has_dir_sep(const char *path)\n \n #include \"wrapper.h\"\n \n+#define mkstemp(template) git_mkstemp_mode((template), 0600)\n+\n /* General helper functions */\n NORETURN void usage(const char *err);\n NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));\n-- \n2.52.0\n\n"},{"id":"530940","messageId":"xmqqqztvc51s.fsf@gitster.g","threadId":"64495","inReplyTo":"3b1cb53a-6427-4626-a768-1961e25514f8@web.de","subject":"Re: [PATCH] wrapper: simplify xmkstemp()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-18T23:08:31Z","receivedAt":"2025-11-18T23:08:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> On 11/18/25 10:46 AM, Jeff King wrote:\n>> \n>> I also wondered if we ever use mkstemp() at all after this patch. If\n>> not, we might want to declare it off-limits. Not because it is evil, but\n>> because our own implementation is more predictable (and we can drop the\n>> compat wrappers for mingw). It looks like there is one more call in\n>> entry.c's open_output_fd(), but arguably that should be calling\n>> xmkstemp() or git_mkstemp_mode(). But that's out of scope for this patch\n>> (I just thought I might nerd-snipe René into looking at it).\n> Thought about it before, but couldn't bring myself to ban mkstemp(3).\n> Its only faults are lack of features (mode setting and suffix support)\n> and not being available on Windows, but apart from that it does its\n> job as advertised.  Which means ... it doesn't cut it for us.  Hmm.\n\nWhen somebody asks:\n\n    On this and that platforms, mkstemp() is natively available.\n    Why are we using git_mkstemp_mode() instead?\n\nafter seeing this patch, I am tempted to say \"Why not?\"  Are there\nlegitimate answers to my \"What not?\"\n\n - the platform native one could be more performant?\n - the platform native one could be more secure?\n - using the platform native one, we can lose out custom code?\n\nNone of the ones I can come up with offhand sound very legitimate.\n\nOne upside might be that doing so would make the behaviour more\npredictable, in that even on a platform with native mkstemp(), we\nwould use the same implementation as what we use on Windows.  But\nI do not know how much upside it is in practice, either.\n\n> --- >8 ---\n> Subject: [PATCH] stop using mkstemp(3)\n>\n> mkstemp(3) works fine if you don't need custom permissions, a specific\n> filename suffix or to run it on Windows.  For those cases we have a\n> custom implementation around git_mkstemps_mode().  Use it for the base\n> case as well, for consistency across platforms.\n>\n> Suggested-by: Jeff King <peff@peff.net>\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n>  compat/mingw-posix.h | 1 -\n>  compat/mingw.c       | 5 -----\n>  git-compat-util.h    | 2 ++\n>  3 files changed, 2 insertions(+), 6 deletions(-)\n>\n> diff --git a/compat/mingw-posix.h b/compat/mingw-posix.h\n> index 631a208684..57915119c6 100644\n> --- a/compat/mingw-posix.h\n> +++ b/compat/mingw-posix.h\n> @@ -185,7 +185,6 @@ char *mingw_locate_in_PATH(const char *cmd);\n>  \n>  int pipe(int filedes[2]);\n>  unsigned int sleep (unsigned int seconds);\n> -int mkstemp(char *template);\n>  int gettimeofday(struct timeval *tv, void *tz);\n>  #ifndef __MINGW64_VERSION_MAJOR\n>  struct tm *gmtime_r(const time_t *timep, struct tm *result);\n> diff --git a/compat/mingw.c b/compat/mingw.c\n> index 736a07a028..dc3da7c6d5 100644\n> --- a/compat/mingw.c\n> +++ b/compat/mingw.c\n> @@ -1174,11 +1174,6 @@ char *mingw_mktemp(char *template)\n>  \treturn template;\n>  }\n>  \n> -int mkstemp(char *template)\n> -{\n> -\treturn git_mkstemp_mode(template, 0600);\n> -}\n> -\n>  int gettimeofday(struct timeval *tv, void *tz UNUSED)\n>  {\n>  \tFILETIME ft;\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index 398e0fac4f..0e6bd266cc 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -446,6 +446,8 @@ static inline int git_has_dir_sep(const char *path)\n>  \n>  #include \"wrapper.h\"\n>  \n> +#define mkstemp(template) git_mkstemp_mode((template), 0600)\n> +\n>  /* General helper functions */\n>  NORETURN void usage(const char *err);\n>  NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2)));\n"},{"id":"531053","messageId":"20251120082328.GD1283645@coredump.intra.peff.net","threadId":"64495","inReplyTo":"xmqqqztvc51s.fsf@gitster.g","subject":"Re: [PATCH] wrapper: simplify xmkstemp()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-20T08:23:28Z","receivedAt":"2025-11-20T08:23:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 18, 2025 at 03:08:31PM -0800, Junio C Hamano wrote:\n\n> When somebody asks:\n> \n>     On this and that platforms, mkstemp() is natively available.\n>     Why are we using git_mkstemp_mode() instead?\n> \n> after seeing this patch, I am tempted to say \"Why not?\"  Are there\n> legitimate answers to my \"What not?\"\n> \n>  - the platform native one could be more performant?\n>  - the platform native one could be more secure?\n>  - using the platform native one, we can lose out custom code?\n> \n> None of the ones I can come up with offhand sound very legitimate.\n> \n> One upside might be that doing so would make the behaviour more\n> predictable, in that even on a platform with native mkstemp(), we\n> would use the same implementation as what we use on Windows.  But\n> I do not know how much upside it is in practice, either.\n\nI think predictability cuts both ways. The system mkstemp() will behave\nmore like it does on the rest of that platform, but maybe less like Git\non other platforms. Using Git's implementation will be consistent across\nplatforms, but maybe inconsistent with the rest of the current platform.\n\nI think the \"consistent with the rest of the current platform\" ship may\nhave already sailed, though. We already use our custom git_mkstemp_mode() on\nevery platform for most tempfiles. And now even those few xmkstemp()\ncalls will do so (after René's first patch).\n\nMy suggestion was mostly: if we are going to use custom code at all,\nthen let's at least do so always, and not ever use the system mkstemp().\n\n> > diff --git a/git-compat-util.h b/git-compat-util.h\n> > index 398e0fac4f..0e6bd266cc 100644\n> > --- a/git-compat-util.h\n> > +++ b/git-compat-util.h\n> > @@ -446,6 +446,8 @@ static inline int git_has_dir_sep(const char *path)\n> >  \n> >  #include \"wrapper.h\"\n> >  \n> > +#define mkstemp(template) git_mkstemp_mode((template), 0600)\n\nSo this patch implements what I was thinking, though I probably would\nhave made it more explicit: add mkstemp() to the banned list (not\nbecause it's evil but because it's unportable) and force callers to use\ngit_mkstemp_mode() explicitly.\n\n-Peff\n"},{"id":"531056","messageId":"xmqqbjkwahu1.fsf@gitster.g","threadId":"64495","inReplyTo":"20251120082328.GD1283645@coredump.intra.peff.net","subject":"Re: [PATCH] wrapper: simplify xmkstemp()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-20T14:39:50Z","receivedAt":"2025-11-20T14:39:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> > +#define mkstemp(template) git_mkstemp_mode((template), 0600)\n>\n> So this patch implements what I was thinking, though I probably would\n> have made it more explicit: add mkstemp() to the banned list (not\n> because it's evil but because it's unportable) and force callers to use\n> git_mkstemp_mode() explicitly.\n\nBecause only a very small number (one?)  of callers call mkstemp()\nin the current code, the above is probably a good thing to do.\n\n"},{"id":"531161","messageId":"fd7023d3-c9a8-4828-b1a2-ac5a1459cbda@web.de","threadId":"64495","inReplyTo":"xmqqqztvc51s.fsf@gitster.g","subject":"Re: [PATCH] wrapper: simplify xmkstemp()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-11-22T13:24:58Z","receivedAt":"2025-11-22T13:25:12Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"On 11/19/25 12:08 AM, Junio C Hamano wrote:\n> René Scharfe <l.s.r@web.de> writes:\n> \n> When somebody asks:\n> \n>     On this and that platforms, mkstemp() is natively available.\n>     Why are we using git_mkstemp_mode() instead?\n> \n> after seeing this patch, I am tempted to say \"Why not?\"  Are there\n> legitimate answers to my \"What not?\"\n\nMy reluctance to let go of mkstemp(3) is seeping through.  I like that\nfunction.  Let's see if I can get over it.\n\n>  - the platform native one could be more performant?\n\nGood question.  A platform could use non-portable tricks like using a\nparticularly cheap source of randomness or a system call that creates a\nfile with the next available name matching a prefix.  As there any that\ndo, though?  We can find out by measuring, patch below.\n\nOn macOS 26.1 I get:\n\n$ hyperfine -w3 -C 'rm /tmp/tmp_*' -L fn mkstemp,git_mkstemp_mode \"t/helper/test-tool mktemp -n 1000 -f {fn} /tmp/tmp_XXXXXX\"\nBenchmark 1: t/helper/test-tool mktemp -n 1000 -f mkstemp /tmp/tmp_XXXXXX\n  Time (mean ± σ):      56.7 ms ±   0.4 ms    [User: 3.7 ms, System: 52.5 ms]\n  Range (min … max):    56.0 ms …  57.3 ms    23 runs\n\nBenchmark 2: t/helper/test-tool mktemp -n 1000 -f git_mkstemp_mode /tmp/tmp_XXXXXX\n  Time (mean ± σ):      54.2 ms ±   0.4 ms    [User: 3.1 ms, System: 50.7 ms]\n  Range (min … max):    53.7 ms …  54.9 ms    24 runs\n\nSummary\n  t/helper/test-tool mktemp -n 1000 -f git_mkstemp_mode /tmp/tmp_XXXXXX ran\n    1.05 ± 0.01 times faster than t/helper/test-tool mktemp -n 1000 -f mkstemp /tmp/tmp_XXXXXX\n\nWeird.  How can mkstemp(3) burn measurably more system cycles?\n\n>  - the platform native one could be more secure?\n\nIt could if it works deterministically, or uses a better source of\nrandomness, or our code contains an exploitable flaw.\n\n>  - using the platform native one, we can lose out custom code?\n\nNot much unless we're willing to give up performance on those platforms\nfor the cases where we want to set the file mode.  There is mkstemps(3)\nfor allowing a suffix and mkostemps(3) for also allowing to set open(2)\nflags, but I couldn't find an equivalent of git_mkstemp_mode().\n\nA replacement could look like this:\n\nint git_mkstemp_mode(char *pattern, int mode)\n{\n\tint fd = mkstemp(pattern);\n\tif (fd >= 0 && mode != 0600)\n\t\tfchmod(fd, mode);\n\treturn fd;\n}\n\nThat's basically what happens if we give the test helper a non-default\nmode value.  Unsurprisingly the extra fchmod(2) call has a significant\ncost:\n\n$ 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\"\nBenchmark 1: t/helper/test-tool mktemp -n 1000 -m 0700 -f mkstemp /tmp/tmp_XXXXXX\n  Time (mean ± σ):      67.2 ms ±   0.4 ms    [User: 3.9 ms, System: 62.9 ms]\n  Range (min … max):    66.6 ms …  68.1 ms    22 runs\n\nBenchmark 2: t/helper/test-tool mktemp -n 1000 -m 0700 -f git_mkstemp_mode /tmp/tmp_XXXXXX\n  Time (mean ± σ):      54.3 ms ±   0.7 ms    [User: 3.1 ms, System: 50.6 ms]\n  Range (min … max):    53.5 ms …  57.1 ms    24 runs\n\nSummary\n  t/helper/test-tool mktemp -n 1000 -m 0700 -f git_mkstemp_mode /tmp/tmp_XXXXXX ran\n    1.24 ± 0.02 times faster than t/helper/test-tool mktemp -n 1000 -m 0700 -f mkstemp /tmp/tmp_XXXXXX\n\nIf we keep the mode-setting variant anyway, the rest is just a bunch of\ntrivial wrappers that cannot possibly add a performance problem and are\neasy to check for security issues.\n\n> One upside might be that doing so would make the behaviour more\n> predictable, in that even on a platform with native mkstemp(), we\n> would use the same implementation as what we use on Windows.  But\n> I do not know how much upside it is in practice, either.\n\nUsing a single implementation everywhere is easier to code, test and\nmaintain.  A flaw in it would have a bigger blast radius, though.\n\nCan we depend on git_mkstemps_mode() and co.?  We already do.  Can\nwe depend on them in cases where we don't need a suffix and mode\n0600 suffixes?  I don't see why we can't.\n\nRené\n\n\n--- >8 ---\nSubject: [PATCH] test-mktemp: allow testing mkstemp(3) and git_mkstemp_mode()\n\nAllow testing two more functions for creating temporary files by using\nparseopt to provide options for selecting them.\n\nAllow specifying custom file permissions to exercise git_mkstemp_mode()\nfully.\n\nAlso add an option for calling the selected function a number of times,\nwhich allows for easier performance tests.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n t/helper/test-mktemp.c | 73 ++++++++++++++++++++++++++++++++++++++++--\n 1 file changed, 70 insertions(+), 3 deletions(-)\n\ndiff --git a/t/helper/test-mktemp.c b/t/helper/test-mktemp.c\nindex 2290688940..9b8bc95683 100644\n--- a/t/helper/test-mktemp.c\n+++ b/t/helper/test-mktemp.c\n@@ -3,13 +3,80 @@\n  */\n #include \"test-tool.h\"\n #include \"git-compat-util.h\"\n+#include \"parse-options.h\"\n+#include \"strbuf.h\"\n+\n+static char const * const test_mktemp_usage[] = {\n+\tN_(\"test-tool mktemp [options] <template>\"),\n+\tNULL\n+};\n+\n+static int test_xmkstemp(char *template, int mode)\n+{\n+\tint fd = xmkstemp(template);\n+\tif (mode != 0600)\n+\t\tfchmod(fd, mode);\n+\treturn fd;\n+}\n+\n+static int test_mkstemp(char *template, int mode)\n+{\n+\tint fd = mkstemp(template);\n+\tif (fd < -1)\n+\t\tdie_errno(_(\"unable to create temporary file '%s'\"), template);\n+\tif (mode != 0600)\n+\t\tfchmod(fd, mode);\n+\treturn fd;\n+}\n+\n+static int test_git_mkstemp_mode(char *template, int mode)\n+{\n+\tint fd = git_mkstemp_mode(template, mode);\n+\tif (fd < -1)\n+\t\tdie_errno(_(\"unable to create temporary file '%s'\"), template);\n+\treturn fd;\n+}\n \n int cmd__mktemp(int argc, const char **argv)\n {\n-\tif (argc != 2)\n-\t\tusage(\"Expected 1 parameter defining the temporary file template\");\n+\tstruct strbuf template = STRBUF_INIT;\n+\tconst char *function_name = \"xmkstemp\";\n+\tint mode = 0600;\n+\tint count = 1;\n+\tstruct option options[] = {\n+\t\tOPT_STRING_F('f', \"function\", &function_name, \"name\",\n+\t\t\t     N_(\"select the function to call\"),\n+\t\t\t     PARSE_OPT_NONEG),\n+\t\tOPT_INTEGER('m', \"mode\", &mode,\n+\t\t\t    N_(\"specify file permission bits\")),\n+\t\tOPT_INTEGER('n', \"count\", &count,\n+\t\t\t    N_(\"specify the number of files\")),\n+\t\tOPT_END()\n+\t};\n+\tint (*fn)(char *, int);\n+\n+\targc = parse_options(argc, argv, NULL, options,\n+\t\t\t     test_mktemp_usage, 0);\n+\n+\tif (argc != 1)\n+\t\tusage_with_options(test_mktemp_usage, options);\n+\n+\tif (!strcmp(function_name, \"xmkstemp\"))\n+\t\tfn = test_xmkstemp;\n+\telse if (!strcmp(function_name, \"mkstemp\"))\n+\t\tfn = test_mkstemp;\n+\telse if (!strcmp(function_name, \"git_mkstemp_mode\"))\n+\t\tfn = test_git_mkstemp_mode;\n+\telse\n+\t\tdie(_(\"unsupported function: %s\"), function_name);\n+\n+\tfor (int i = 0; i < count; i++) {\n+\t\tstrbuf_reset(&template);\n+\t\tstrbuf_addstr(&template, argv[0]);\n+\t\tclose(fn(template.buf, mode));\n+\t}\n \n-\txmkstemp(xstrdup(argv[1]));\n+\tstrbuf_release(&template);\n \n \treturn 0;\n }\n-- \n2.52.0\n\n"},{"id":"531162","messageId":"18a0a729-d77c-4f4d-9581-b102bd66816c@web.de","threadId":"64495","inReplyTo":"xmqqbjkwahu1.fsf@gitster.g","subject":"Re: [PATCH] wrapper: simplify xmkstemp()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2025-11-22T13:29:49Z","receivedAt":"2025-11-22T13:29:57Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"On 11/20/25 3:39 PM, Junio C Hamano wrote:\n> Jeff King <peff@peff.net> writes:\n> \n>>>> +#define mkstemp(template) git_mkstemp_mode((template), 0600)\n>>\n>> So this patch implements what I was thinking, though I probably would\n>> have made it more explicit: add mkstemp() to the banned list (not\n>> because it's evil but because it's unportable) and force callers to use\n>> git_mkstemp_mode() explicitly.\n> \n> Because only a very small number (one?)  of callers call mkstemp()\n> in the current code, the above is probably a good thing to do.\n\nTrue, banning mkstemp(3) instead of overriding it would simplify the\ncode by removing one layer of indirection, and the hassle of no longer\nbeing able to use that standard function would be low.\n\nRené\n\n"}]}