[PATCH 0/4] ban mktemp(3)

[PATCH 0/4] ban mktemp(3)

[René Scharfe]

mktemp(3) is insecure and POSIX.1-2008 no longer specifies it.  Stop
using it.

  wrapper: add git_mkdtemp()
  compat: use git_mkdtemp()
  compat: remove mingw_mktemp()
  banned.h: ban mktemp(3)

 banned.h             |  3 +++
 compat/mingw-posix.h |  3 ---
 compat/mingw.c       | 12 ------------
 compat/mkdtemp.c     |  4 +---
 wrapper.c            | 17 +++++++++++++++--
 wrapper.h            |  2 ++
 6 files changed, 21 insertions(+), 20 deletions(-)

-- 
2.52.0

[PATCH 1/4] wrapper: add git_mkdtemp()

[René Scharfe]

Extend git_mkstemps_mode() to optionally call mkdir(2) instead of
open(2), then use that ability to create a mkdtemp(3) replacement,
git_mkdtemp().  We'll start using it in the next commit.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 wrapper.c | 17 +++++++++++++++--
 wrapper.h |  2 ++
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/wrapper.c b/wrapper.c
index d5976b3e7e..6ddf4eb66b 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -429,7 +429,7 @@ int xmkstemp(char *filename_template)
 #undef TMP_MAX
 #define TMP_MAX 16384
 
-int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
+static int git_mkdstemps_mode(char *pattern, int suffix_len, int mode, bool dir)
 {
 	static const char letters[] =
 		"abcdefghijklmnopqrstuvwxyz"
@@ -471,7 +471,10 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
 			v /= num_letters;
 		}
 
-		fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);
+		if (dir)
+			fd = mkdir(pattern, mode);
+		else
+			fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);
 		if (fd >= 0)
 			return fd;
 		/*
@@ -486,6 +489,16 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
 	return -1;
 }
 
+char *git_mkdtemp(char *pattern)
+{
+	return git_mkdstemps_mode(pattern, 0, 0700, true) ? NULL : pattern;
+}
+
+int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
+{
+	return git_mkdstemps_mode(pattern, suffix_len, mode, false);
+}
+
 int git_mkstemp_mode(char *pattern, int mode)
 {
 	/* mkstemp is just mkstemps with no suffix */
diff --git a/wrapper.h b/wrapper.h
index 44a8597ac3..15ac3bab6e 100644
--- a/wrapper.h
+++ b/wrapper.h
@@ -37,6 +37,8 @@ int xsnprintf(char *dst, size_t max, const char *fmt, ...);
 
 int xgethostname(char *buf, size_t len);
 
+char *git_mkdtemp(char *pattern);
+
 /* set default permissions by passing mode arguments to open(2) */
 int git_mkstemps_mode(char *pattern, int suffix_len, int mode);
 int git_mkstemp_mode(char *pattern, int mode);
-- 
2.52.0

Re: [PATCH 1/4] wrapper: add git_mkdtemp()

[Chris Torek]

fairly trivial, but:

On Wed, Dec 3, 2025 at 2:52 AM René Scharfe <l.s.r@web.de> wrote:
> Extend git_mkstemps_mode() to optionally call mkdir(2) instead of
> open(2), then use that ability to create a mkdtemp(3) replacement,
> git_mkdtemp().  We'll start using it in the next commit.
[snip]
> -               fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);
> +               if (dir)
> +                       fd = mkdir(pattern, mode);
> +               else
> +                       fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);

mkdir() returns a success (0) / fail (-1) indication, rather than
a file descriptor, so this is kind of misleading.  I think a
comment mentioning it would suffice (but also be a good idea, lest
someone later think it needs a close() call).

Chris

Re: [PATCH 1/4] wrapper: add git_mkdtemp()

[Junio C Hamano]

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

> -int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
> +static int git_mkdstemps_mode(char *pattern, int suffix_len, int mode, bool dir)

This is a file-scope static, so as long as it is understood by those
who futz with things in this file well, there is no need to go extra
mile to avoid confusion, but the meaning of the returned value from
this function is vastly different depending on the value of "dir".
It used to be that you can subject it to write(2), but obviously
that is not relevant when you called mkdir(2) here.

>  {
>  	static const char letters[] =
>  		"abcdefghijklmnopqrstuvwxyz"
> @@ -471,7 +471,10 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
>  			v /= num_letters;
>  		}
>  
> -		fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);
> +		if (dir)
> +			fd = mkdir(pattern, mode);

OK.  The caller calls this helper with 0700 (S_IRWXU), so that's
probably OK.  If we can make this into two helper functions with
distinct function signatures that share the majority of logic, it
would have been much nicer, but short of introducing a callback
function I do not think of a good way, so I'll let it pass.

> +		else
> +			fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);
>  		if (fd >= 0)
>  			return fd;

[PATCH 2/4] compat: use git_mkdtemp()

[René Scharfe]

A file might appear at the path returned by mktemp(3) before we call
mkdir(2).  Use the more robust git_mkdtemp() instead, which retries a
number of times and doesn't need to call lstat(2).

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 compat/mkdtemp.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/compat/mkdtemp.c b/compat/mkdtemp.c
index 1136119592..fcdd4e01e1 100644
--- a/compat/mkdtemp.c
+++ b/compat/mkdtemp.c
@@ -2,7 +2,5 @@
 
 char *gitmkdtemp(char *template)
 {
-	if (!*mktemp(template) || mkdir(template, 0700))
-		return NULL;
-	return template;
+	return git_mkdtemp(template);
 }
-- 
2.52.0

Re: [PATCH 2/4] compat: use git_mkdtemp()

[Jeff King]

On Wed, Dec 03, 2025 at 11:52:30AM +0100, René Scharfe wrote:

> A file might appear at the path returned by mktemp(3) before we call
> mkdir(2).  Use the more robust git_mkdtemp() instead, which retries a
> number of times and doesn't need to call lstat(2).

This seems like a good idea. At least one of the mkdtemp() callers was
using $TMPDIR, so this was a potential security-sensitive race.

> diff --git a/compat/mkdtemp.c b/compat/mkdtemp.c
> index 1136119592..fcdd4e01e1 100644
> --- a/compat/mkdtemp.c
> +++ b/compat/mkdtemp.c
> @@ -2,7 +2,5 @@
>  
>  char *gitmkdtemp(char *template)
>  {
> -	if (!*mktemp(template) || mkdir(template, 0700))
> -		return NULL;
> -	return template;
> +	return git_mkdtemp(template);
>  }

OK, so now we have gitmkdtemp() and git_mkdtemp(), which are also now
the exact same thing. That seems overly complicated. ;)

This one is a conditionally-compiled wrapper for NO_MKDTEMP. But since
we always have git_mkdtemp() available (as of your first patch), can't
we just point at it directly with the macro?

Like this:

diff --git a/Makefile b/Makefile
index 237b56fc9d..8226aed443 100644
--- a/Makefile
+++ b/Makefile
@@ -1919,7 +1919,6 @@ ifdef NO_SETENV
 endif
 ifdef NO_MKDTEMP
 	COMPAT_CFLAGS += -DNO_MKDTEMP
-	COMPAT_OBJS += compat/mkdtemp.o
 endif
 ifdef MKDIR_WO_TRAILING_SLASH
 	COMPAT_CFLAGS += -DMKDIR_WO_TRAILING_SLASH
diff --git a/compat/mkdtemp.c b/compat/mkdtemp.c
deleted file mode 100644
index fcdd4e01e1..0000000000
--- a/compat/mkdtemp.c
+++ /dev/null
@@ -1,6 +0,0 @@
-#include "../git-compat-util.h"
-
-char *gitmkdtemp(char *template)
-{
-	return git_mkdtemp(template);
-}
diff --git a/compat/posix.h b/compat/posix.h
index 067a00f33b..245386fa4a 100644
--- a/compat/posix.h
+++ b/compat/posix.h
@@ -329,8 +329,7 @@ int gitsetenv(const char *, const char *, int);
 #endif
 
 #ifdef NO_MKDTEMP
-#define mkdtemp gitmkdtemp
-char *gitmkdtemp(char *);
+#define mkdtemp git_mkdtemp
 #endif
 
 #ifdef NO_UNSETENV
diff --git a/meson.build b/meson.build
index f1b3615659..090b1911ca 100644
--- a/meson.build
+++ b/meson.build
@@ -1401,7 +1401,6 @@ checkfuncs = {
   'strlcpy' : ['strlcpy.c'],
   'strtoull' : [],
   'setenv' : ['setenv.c'],
-  'mkdtemp' : ['mkdtemp.c'],
   'initgroups' : [],
   'strtoumax' : ['strtoumax.c', 'strtoimax.c'],
   'pread' : ['pread.c'],


We could even take it a step further and just always use git_mkdtemp(),
like we were discussing elsewhere for mkstemp(). And then the makefile
knobs can go away, too, like:

diff --git a/Makefile b/Makefile
index 8226aed443..8ef5497c10 100644
--- a/Makefile
+++ b/Makefile
@@ -68,8 +68,6 @@ include shared.mak
 #
 # Define NO_UNSETENV if you don't have unsetenv in the C library.
 #
-# Define NO_MKDTEMP if you don't have mkdtemp in the C library.
-#
 # Define MKDIR_WO_TRAILING_SLASH if your mkdir() can't deal with trailing slash.
 #
 # Define NO_GECOS_IN_PWENT if you don't have pw_gecos in struct passwd
diff --git a/compat/posix.h b/compat/posix.h
index 245386fa4a..c49d67e653 100644
--- a/compat/posix.h
+++ b/compat/posix.h
@@ -328,9 +328,7 @@ ssize_t git_pread(int fd, void *buf, size_t count, off_t offset);
 int gitsetenv(const char *, const char *, int);
 #endif
 
-#ifdef NO_MKDTEMP
 #define mkdtemp git_mkdtemp
-#endif
 
 #ifdef NO_UNSETENV
 #define unsetenv gitunsetenv
diff --git a/configure.ac b/configure.ac
index cfb50112bf..8e61186f18 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1140,12 +1140,6 @@ GIT_CHECK_FUNC(unsetenv,
 [NO_UNSETENV=YesPlease])
 GIT_CONF_SUBST([NO_UNSETENV])
 #
-# Define NO_MKDTEMP if you don't have mkdtemp in the C library.
-GIT_CHECK_FUNC(mkdtemp,
-[NO_MKDTEMP=],
-[NO_MKDTEMP=YesPlease])
-GIT_CONF_SUBST([NO_MKDTEMP])
-#
 # Define NO_INITGROUPS if you don't have initgroups in the C library.
 GIT_CHECK_FUNC(initgroups,
 [NO_INITGROUPS=],
diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 479163ab5c..d28de227f5 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -374,7 +374,7 @@ endif()
 #function checks
 set(function_checks
 	strcasestr memmem strlcpy strtoimax strtoumax strtoull
-	setenv mkdtemp poll pread memmem)
+	setenv poll pread memmem)
 
 #unsetenv,hstrerror are incompatible with windows build
 if(NOT WIN32)
@@ -411,10 +411,6 @@ if(NOT HAVE_SETENV)
 	list(APPEND compat_SOURCES compat/setenv.c)
 endif()
 
-if(NOT HAVE_MKDTEMP)
-	list(APPEND compat_SOURCES compat/mkdtemp.c)
-endif()
-
 if(NOT HAVE_PREAD)
 	list(APPEND compat_SOURCES compat/pread.c)
 endif()

-Peff

Re: [PATCH 2/4] compat: use git_mkdtemp()

[René Scharfe]

On 12/3/25 5:11 PM, Jeff King wrote:
> On Wed, Dec 03, 2025 at 11:52:30AM +0100, René Scharfe wrote:
> 
>> A file might appear at the path returned by mktemp(3) before we call
>> mkdir(2).  Use the more robust git_mkdtemp() instead, which retries a
>> number of times and doesn't need to call lstat(2).
> 
> This seems like a good idea. At least one of the mkdtemp() callers was
> using $TMPDIR, so this was a potential security-sensitive race.
> 
>> diff --git a/compat/mkdtemp.c b/compat/mkdtemp.c
>> index 1136119592..fcdd4e01e1 100644
>> --- a/compat/mkdtemp.c
>> +++ b/compat/mkdtemp.c
>> @@ -2,7 +2,5 @@
>>  
>>  char *gitmkdtemp(char *template)
>>  {
>> -	if (!*mktemp(template) || mkdir(template, 0700))
>> -		return NULL;
>> -	return template;
>> +	return git_mkdtemp(template);
>>  }
> 
> OK, so now we have gitmkdtemp() and git_mkdtemp(), which are also now
> the exact same thing. That seems overly complicated. ;)
> 
> This one is a conditionally-compiled wrapper for NO_MKDTEMP. But since
> we always have git_mkdtemp() available (as of your first patch), can't
> we just point at it directly with the macro?

A worthwhile cleanup if we stop at this point, but complicated by
targeting three build systems, the CMake build being broken on macOS and
me only knowing how to fake NO_MKDEMP for make, leaving half the build
space untestable for me.

> Like this:
> 
> diff --git a/Makefile b/Makefile
> index 237b56fc9d..8226aed443 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1919,7 +1919,6 @@ ifdef NO_SETENV
>  endif
>  ifdef NO_MKDTEMP
>  	COMPAT_CFLAGS += -DNO_MKDTEMP
> -	COMPAT_OBJS += compat/mkdtemp.o
>  endif
>  ifdef MKDIR_WO_TRAILING_SLASH
>  	COMPAT_CFLAGS += -DMKDIR_WO_TRAILING_SLASH
> diff --git a/compat/mkdtemp.c b/compat/mkdtemp.c
> deleted file mode 100644
> index fcdd4e01e1..0000000000
> --- a/compat/mkdtemp.c
> +++ /dev/null
> @@ -1,6 +0,0 @@
> -#include "../git-compat-util.h"
> -
> -char *gitmkdtemp(char *template)
> -{
> -	return git_mkdtemp(template);
> -}
> diff --git a/compat/posix.h b/compat/posix.h
> index 067a00f33b..245386fa4a 100644
> --- a/compat/posix.h
> +++ b/compat/posix.h
> @@ -329,8 +329,7 @@ int gitsetenv(const char *, const char *, int);
>  #endif
>  
>  #ifdef NO_MKDTEMP
> -#define mkdtemp gitmkdtemp
> -char *gitmkdtemp(char *);
> +#define mkdtemp git_mkdtemp
>  #endif
>  
>  #ifdef NO_UNSETENV
> diff --git a/meson.build b/meson.build
> index f1b3615659..090b1911ca 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1401,7 +1401,6 @@ checkfuncs = {
>    'strlcpy' : ['strlcpy.c'],
>    'strtoull' : [],
>    'setenv' : ['setenv.c'],
> -  'mkdtemp' : ['mkdtemp.c'],
>    'initgroups' : [],
>    'strtoumax' : ['strtoumax.c', 'strtoimax.c'],
>    'pread' : ['pread.c'],

We need to keep that dictionary entry to still define NO_MKDTEMP, and
just empty the array of filenames.

contrib/buildsystems/CMakeLists.txt needs to be updated as well, like
you do below (keep in function_checks, remove from compat_SOURCES).

At the very least this cleanup should be done in a separated patch, as
it's harder than it looks.

> We could even take it a step further and just always use git_mkdtemp(),
> like we were discussing elsewhere for mkstemp(). And then the makefile
> knobs can go away, too, like:
> 
> diff --git a/Makefile b/Makefile
> index 8226aed443..8ef5497c10 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -68,8 +68,6 @@ include shared.mak
>  #
>  # Define NO_UNSETENV if you don't have unsetenv in the C library.
>  #
> -# Define NO_MKDTEMP if you don't have mkdtemp in the C library.
> -#
>  # Define MKDIR_WO_TRAILING_SLASH if your mkdir() can't deal with trailing slash.
>  #
>  # Define NO_GECOS_IN_PWENT if you don't have pw_gecos in struct passwd
> diff --git a/compat/posix.h b/compat/posix.h
> index 245386fa4a..c49d67e653 100644
> --- a/compat/posix.h
> +++ b/compat/posix.h
> @@ -328,9 +328,7 @@ ssize_t git_pread(int fd, void *buf, size_t count, off_t offset);
>  int gitsetenv(const char *, const char *, int);
>  #endif
>  
> -#ifdef NO_MKDTEMP
>  #define mkdtemp git_mkdtemp
> -#endif
>  
>  #ifdef NO_UNSETENV
>  #define unsetenv gitunsetenv
> diff --git a/configure.ac b/configure.ac
> index cfb50112bf..8e61186f18 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -1140,12 +1140,6 @@ GIT_CHECK_FUNC(unsetenv,
>  [NO_UNSETENV=YesPlease])
>  GIT_CONF_SUBST([NO_UNSETENV])
>  #
> -# Define NO_MKDTEMP if you don't have mkdtemp in the C library.
> -GIT_CHECK_FUNC(mkdtemp,
> -[NO_MKDTEMP=],
> -[NO_MKDTEMP=YesPlease])
> -GIT_CONF_SUBST([NO_MKDTEMP])
> -#
>  # Define NO_INITGROUPS if you don't have initgroups in the C library.
>  GIT_CHECK_FUNC(initgroups,
>  [NO_INITGROUPS=],
> diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> index 479163ab5c..d28de227f5 100644
> --- a/contrib/buildsystems/CMakeLists.txt
> +++ b/contrib/buildsystems/CMakeLists.txt
> @@ -374,7 +374,7 @@ endif()
>  #function checks
>  set(function_checks
>  	strcasestr memmem strlcpy strtoimax strtoumax strtoull
> -	setenv mkdtemp poll pread memmem)
> +	setenv poll pread memmem)
>  
>  #unsetenv,hstrerror are incompatible with windows build
>  if(NOT WIN32)
> @@ -411,10 +411,6 @@ if(NOT HAVE_SETENV)
>  	list(APPEND compat_SOURCES compat/setenv.c)
>  endif()
>  
> -if(NOT HAVE_MKDTEMP)
> -	list(APPEND compat_SOURCES compat/mkdtemp.c)
> -endif()
> -
>  if(NOT HAVE_PREAD)
>  	list(APPEND compat_SOURCES compat/pread.c)
>  endif()

Right.  Dropping this dependency and then deep cleaning the compat code
is attractive and mostly sidesteps the build system complications.
That's for a later series.

Ultimately you'd prefer banning mkdtemp(3) instead of automatically
redirecting to git_mkdtemp(), though, no?

René

Re: [PATCH 2/4] compat: use git_mkdtemp()

[Jeff King]

On Fri, Dec 05, 2025 at 01:11:40PM +0100, René Scharfe wrote:

> > This one is a conditionally-compiled wrapper for NO_MKDTEMP. But since
> > we always have git_mkdtemp() available (as of your first patch), can't
> > we just point at it directly with the macro?
> 
> A worthwhile cleanup if we stop at this point, but complicated by
> targeting three build systems, the CMake build being broken on macOS and
> me only knowing how to fake NO_MKDEMP for make, leaving half the build
> space untestable for me.
> [...]
> At the very least this cleanup should be done in a separated patch, as
> it's harder than it looks.

OK, I am convinced that it is not entirely trivial and can go in a
separate patch. :) Mostly I was surprised that you would not have
followed through on an obvious cleanup opportunity.  It just turned out
harder than I expected.

> Right.  Dropping this dependency and then deep cleaning the compat code
> is attractive and mostly sidesteps the build system complications.
> That's for a later series.

Yup. Sounds reasonable.

> Ultimately you'd prefer banning mkdtemp(3) instead of automatically
> redirecting to git_mkdtemp(), though, no?

I'm OK either way. Whatever we end up doing for mkstemp(), I think we
should match here.

I do like being explicit that we are using our own wrapper and not the
system function. But I wonder if it might make complications in
third-party code like clar, which calls mkdtemp(). If we don't have a
compat macro we'll have to patch the sources that we import into
t/unit-tests/clar.

So maybe that is an argument that we should leave the "#define mkdtemp"
in place.

-Peff

Re: [PATCH 2/4] compat: use git_mkdtemp()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> This one is a conditionally-compiled wrapper for NO_MKDTEMP. But since
> we always have git_mkdtemp() available (as of your first patch), can't
> we just point at it directly with the macro?

Yup, that is much nicer.

[PATCH 3/4] compat: remove mingw_mktemp()

[René Scharfe]

Remove the mktemp(3) compatibility function now that its last caller was
removed by the previous commit.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 compat/mingw-posix.h |  3 ---
 compat/mingw.c       | 12 ------------
 2 files changed, 15 deletions(-)

diff --git a/compat/mingw-posix.h b/compat/mingw-posix.h
index 631a208684..0939feff27 100644
--- a/compat/mingw-posix.h
+++ b/compat/mingw-posix.h
@@ -241,9 +241,6 @@ int mingw_chdir(const char *dirname);
 int mingw_chmod(const char *filename, int mode);
 #define chmod mingw_chmod
 
-char *mingw_mktemp(char *template);
-#define mktemp mingw_mktemp
-
 char *mingw_getcwd(char *pointer, int len);
 #define getcwd mingw_getcwd
 
diff --git a/compat/mingw.c b/compat/mingw.c
index 90ba5cea9d..939f938fe2 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1164,18 +1164,6 @@ unsigned int sleep (unsigned int seconds)
 	return 0;
 }
 
-char *mingw_mktemp(char *template)
-{
-	wchar_t wtemplate[MAX_PATH];
-	if (xutftowcs_path(wtemplate, template) < 0)
-		return NULL;
-	if (!_wmktemp(wtemplate))
-		return NULL;
-	if (xwcstoutf(template, wtemplate, strlen(template) + 1) < 0)
-		return NULL;
-	return template;
-}
-
 int mkstemp(char *template)
 {
 	return git_mkstemp_mode(template, 0600);
-- 
2.52.0

[PATCH 4/4] banned.h: ban mktemp(3)

[René Scharfe]

Older versions of mktemp(3) generate easily guessable file names.  The
function checks if the generated name is used, which is unreliable, as
a file with that name might then be created by some other process before
we can do it ourselves.  The function was dropped from POSIX due to its
security problems.  Forbid its use.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 banned.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/banned.h b/banned.h
index 44e76bd90a..2b934c8c43 100644
--- a/banned.h
+++ b/banned.h
@@ -41,4 +41,7 @@
 #undef asctime_r
 #define asctime_r(t, buf) BANNED(asctime_r)
 
+#undef mktemp
+#define mktemp(x) BANNED(mktemp)
+
 #endif /* BANNED_H */
-- 
2.52.0

Re: [PATCH 4/4] banned.h: ban mktemp(3)

[Jeff King]

On Wed, Dec 03, 2025 at 11:53:06AM +0100, René Scharfe wrote:

> Older versions of mktemp(3) generate easily guessable file names.  The
> function checks if the generated name is used, which is unreliable, as
> a file with that name might then be created by some other process before
> we can do it ourselves.  The function was dropped from POSIX due to its
> security problems.  Forbid its use.

Great. I am happy to see this.

-Peff

[PATCH v2 0/5] ban mktemp(3)

[René Scharfe]

mktemp(3) is insecure and POSIX.1-2008 no longer specifies it.  Stop
using it.

Changes since v1:
- add comment regarding return values of git_mkdstemps_mode()
- add patch to drop trivialized gitmkdtemp()

  wrapper: add git_mkdtemp()
  compat: use git_mkdtemp()
  compat: remove mingw_mktemp()
  banned.h: ban mktemp(3)
  compat: remove gitmkdtemp()

 Makefile                            |  1 -
 banned.h                            |  3 +++
 compat/mingw-posix.h                |  3 ---
 compat/mingw.c                      | 12 ------------
 compat/mkdtemp.c                    |  8 --------
 compat/posix.h                      |  3 +--
 contrib/buildsystems/CMakeLists.txt |  4 ----
 meson.build                         |  2 +-
 wrapper.c                           | 21 +++++++++++++++++++--
 wrapper.h                           |  2 ++
 10 files changed, 26 insertions(+), 33 deletions(-)
 delete mode 100644 compat/mkdtemp.c

Range-diff against v1:
1:  830e6375aa ! 1:  413131caf6 wrapper: add git_mkdtemp()
    @@ wrapper.c: int xmkstemp(char *filename_template)
      #define TMP_MAX 16384
      
     -int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
    ++/*
    ++ * Returns -1 on error, 0 if it created a directory, or an open file
    ++ * descriptor to the created regular file.
    ++ */
     +static int git_mkdstemps_mode(char *pattern, int suffix_len, int mode, bool dir)
      {
      	static const char letters[] =
2:  889903eaa2 = 2:  f8850b2a92 compat: use git_mkdtemp()
3:  255b97254f = 3:  6986b4b6bf compat: remove mingw_mktemp()
4:  8300d2e224 = 4:  f34252f411 banned.h: ban mktemp(3)
-:  ---------- > 5:  d106855a23 compat: remove gitmkdtemp()
-- 
2.52.0

[PATCH v2 1/5] wrapper: add git_mkdtemp()

[René Scharfe]

Extend git_mkstemps_mode() to optionally call mkdir(2) instead of
open(2), then use that ability to create a mkdtemp(3) replacement,
git_mkdtemp().  We'll start using it in the next commit.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 wrapper.c | 21 +++++++++++++++++++--
 wrapper.h |  2 ++
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/wrapper.c b/wrapper.c
index d5976b3e7e..b794fb20e7 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -429,7 +429,11 @@ int xmkstemp(char *filename_template)
 #undef TMP_MAX
 #define TMP_MAX 16384
 
-int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
+/*
+ * Returns -1 on error, 0 if it created a directory, or an open file
+ * descriptor to the created regular file.
+ */
+static int git_mkdstemps_mode(char *pattern, int suffix_len, int mode, bool dir)
 {
 	static const char letters[] =
 		"abcdefghijklmnopqrstuvwxyz"
@@ -471,7 +475,10 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
 			v /= num_letters;
 		}
 
-		fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);
+		if (dir)
+			fd = mkdir(pattern, mode);
+		else
+			fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);
 		if (fd >= 0)
 			return fd;
 		/*
@@ -486,6 +493,16 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
 	return -1;
 }
 
+char *git_mkdtemp(char *pattern)
+{
+	return git_mkdstemps_mode(pattern, 0, 0700, true) ? NULL : pattern;
+}
+
+int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
+{
+	return git_mkdstemps_mode(pattern, suffix_len, mode, false);
+}
+
 int git_mkstemp_mode(char *pattern, int mode)
 {
 	/* mkstemp is just mkstemps with no suffix */
diff --git a/wrapper.h b/wrapper.h
index 44a8597ac3..15ac3bab6e 100644
--- a/wrapper.h
+++ b/wrapper.h
@@ -37,6 +37,8 @@ int xsnprintf(char *dst, size_t max, const char *fmt, ...);
 
 int xgethostname(char *buf, size_t len);
 
+char *git_mkdtemp(char *pattern);
+
 /* set default permissions by passing mode arguments to open(2) */
 int git_mkstemps_mode(char *pattern, int suffix_len, int mode);
 int git_mkstemp_mode(char *pattern, int mode);
-- 
2.52.0

[PATCH v2 2/5] compat: use git_mkdtemp()

[René Scharfe]

A file might appear at the path returned by mktemp(3) before we call
mkdir(2).  Use the more robust git_mkdtemp() instead, which retries a
number of times and doesn't need to call lstat(2).

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 compat/mkdtemp.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/compat/mkdtemp.c b/compat/mkdtemp.c
index 1136119592..fcdd4e01e1 100644
--- a/compat/mkdtemp.c
+++ b/compat/mkdtemp.c
@@ -2,7 +2,5 @@
 
 char *gitmkdtemp(char *template)
 {
-	if (!*mktemp(template) || mkdir(template, 0700))
-		return NULL;
-	return template;
+	return git_mkdtemp(template);
 }
-- 
2.52.0

[PATCH v2 3/5] compat: remove mingw_mktemp()

[René Scharfe]

Remove the mktemp(3) compatibility function now that its last caller was
removed by the previous commit.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 compat/mingw-posix.h |  3 ---
 compat/mingw.c       | 12 ------------
 2 files changed, 15 deletions(-)

diff --git a/compat/mingw-posix.h b/compat/mingw-posix.h
index 631a208684..0939feff27 100644
--- a/compat/mingw-posix.h
+++ b/compat/mingw-posix.h
@@ -241,9 +241,6 @@ int mingw_chdir(const char *dirname);
 int mingw_chmod(const char *filename, int mode);
 #define chmod mingw_chmod
 
-char *mingw_mktemp(char *template);
-#define mktemp mingw_mktemp
-
 char *mingw_getcwd(char *pointer, int len);
 #define getcwd mingw_getcwd
 
diff --git a/compat/mingw.c b/compat/mingw.c
index 90ba5cea9d..939f938fe2 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1164,18 +1164,6 @@ unsigned int sleep (unsigned int seconds)
 	return 0;
 }
 
-char *mingw_mktemp(char *template)
-{
-	wchar_t wtemplate[MAX_PATH];
-	if (xutftowcs_path(wtemplate, template) < 0)
-		return NULL;
-	if (!_wmktemp(wtemplate))
-		return NULL;
-	if (xwcstoutf(template, wtemplate, strlen(template) + 1) < 0)
-		return NULL;
-	return template;
-}
-
 int mkstemp(char *template)
 {
 	return git_mkstemp_mode(template, 0600);
-- 
2.52.0

[PATCH v2 4/5] banned.h: ban mktemp(3)

[René Scharfe]

Older versions of mktemp(3) generate easily guessable file names.  The
function checks if the generated name is used, which is unreliable, as
a file with that name might then be created by some other process before
we can do it ourselves.  The function was dropped from POSIX due to its
security problems.  Forbid its use.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 banned.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/banned.h b/banned.h
index 44e76bd90a..2b934c8c43 100644
--- a/banned.h
+++ b/banned.h
@@ -41,4 +41,7 @@
 #undef asctime_r
 #define asctime_r(t, buf) BANNED(asctime_r)
 
+#undef mktemp
+#define mktemp(x) BANNED(mktemp)
+
 #endif /* BANNED_H */
-- 
2.52.0

[PATCH v2 5/5] compat: remove gitmkdtemp()

[René Scharfe]

gitmkdtemp() has become a trivial wrapper around git_mkdtemp().  Remove
this now unnecessary layer of indirection.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
Only tested with make.

 Makefile                            | 1 -
 compat/mkdtemp.c                    | 6 ------
 compat/posix.h                      | 3 +--
 contrib/buildsystems/CMakeLists.txt | 4 ----
 meson.build                         | 2 +-
 5 files changed, 2 insertions(+), 14 deletions(-)
 delete mode 100644 compat/mkdtemp.c

diff --git a/Makefile b/Makefile
index 237b56fc9d..8226aed443 100644
--- a/Makefile
+++ b/Makefile
@@ -1919,7 +1919,6 @@ ifdef NO_SETENV
 endif
 ifdef NO_MKDTEMP
 	COMPAT_CFLAGS += -DNO_MKDTEMP
-	COMPAT_OBJS += compat/mkdtemp.o
 endif
 ifdef MKDIR_WO_TRAILING_SLASH
 	COMPAT_CFLAGS += -DMKDIR_WO_TRAILING_SLASH
diff --git a/compat/mkdtemp.c b/compat/mkdtemp.c
deleted file mode 100644
index fcdd4e01e1..0000000000
--- a/compat/mkdtemp.c
+++ /dev/null
@@ -1,6 +0,0 @@
-#include "../git-compat-util.h"
-
-char *gitmkdtemp(char *template)
-{
-	return git_mkdtemp(template);
-}
diff --git a/compat/posix.h b/compat/posix.h
index 067a00f33b..245386fa4a 100644
--- a/compat/posix.h
+++ b/compat/posix.h
@@ -329,8 +329,7 @@ int gitsetenv(const char *, const char *, int);
 #endif
 
 #ifdef NO_MKDTEMP
-#define mkdtemp gitmkdtemp
-char *gitmkdtemp(char *);
+#define mkdtemp git_mkdtemp
 #endif
 
 #ifdef NO_UNSETENV
diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 479163ab5c..28877feb9d 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -411,10 +411,6 @@ if(NOT HAVE_SETENV)
 	list(APPEND compat_SOURCES compat/setenv.c)
 endif()
 
-if(NOT HAVE_MKDTEMP)
-	list(APPEND compat_SOURCES compat/mkdtemp.c)
-endif()
-
 if(NOT HAVE_PREAD)
 	list(APPEND compat_SOURCES compat/pread.c)
 endif()
diff --git a/meson.build b/meson.build
index f1b3615659..24c656681c 100644
--- a/meson.build
+++ b/meson.build
@@ -1401,7 +1401,7 @@ checkfuncs = {
   'strlcpy' : ['strlcpy.c'],
   'strtoull' : [],
   'setenv' : ['setenv.c'],
-  'mkdtemp' : ['mkdtemp.c'],
+  'mkdtemp' : [],
   'initgroups' : [],
   'strtoumax' : ['strtoumax.c', 'strtoimax.c'],
   'pread' : ['pread.c'],
-- 
2.52.0

Re: [PATCH v2 0/5] ban mktemp(3)

[Jeff King]

On Sat, Dec 06, 2025 at 02:21:06PM +0100, René Scharfe wrote:

> mktemp(3) is insecure and POSIX.1-2008 no longer specifies it.  Stop
> using it.
> 
> Changes since v1:
> - add comment regarding return values of git_mkdstemps_mode()
> - add patch to drop trivialized gitmkdtemp()

Thanks, this all looks reasonable to me. I don't have a system without
mkdtemp() to test the meson change on, but I can confirm it builds on
Linux for me using meson. ;)

-Peff