René Scharfemktemp(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
René Scharfe[PATCH 1/4] wrapper: add git_mkdtemp()
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(-)
wrapper.c, wrapper.h
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
Chris TorekRe: [PATCH 1/4] wrapper: add git_mkdtemp()
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
Junio C HamanoRe: [PATCH 1/4] wrapper: add git_mkdtemp()
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;
René Scharfe[PATCH 2/4] compat: use git_mkdtemp()
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
Jeff KingRe: [PATCH 2/4] compat: use git_mkdtemp()
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:
Makefile, compat/mkdtemp.c, compat/posix.h, meson.build, configure.ac, contrib/buildsystems/CMakeLists.txt
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
René ScharfeRe: [PATCH 2/4] compat: use git_mkdtemp()
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é
Jeff KingRe: [PATCH 2/4] compat: use git_mkdtemp()
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
Junio C HamanoRe: [PATCH 2/4] compat: use git_mkdtemp()
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.
René Scharfe[PATCH 3/4] compat: remove mingw_mktemp()
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(-)
compat/mingw-posix.h, compat/mingw.c
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
René Scharfe[PATCH 4/4] banned.h: ban mktemp(3)
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
Jeff KingRe: [PATCH 4/4] banned.h: ban mktemp(3)
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
René Scharfe[PATCH v2 0/5] ban mktemp(3)
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
René Scharfe[PATCH v2 1/5] wrapper: add git_mkdtemp()
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(-)
wrapper.c, wrapper.h
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
René Scharfe[PATCH v2 2/5] compat: use git_mkdtemp()
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
René Scharfe[PATCH v2 3/5] compat: remove mingw_mktemp()
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(-)
compat/mingw-posix.h, compat/mingw.c
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
René Scharfe[PATCH v2 4/5] banned.h: ban mktemp(3)
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
René Scharfe[PATCH v2 5/5] compat: remove gitmkdtemp()
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
Makefile, compat/mkdtemp.c, compat/posix.h, contrib/buildsystems/CMakeLists.txt, meson.build
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
Jeff KingRe: [PATCH v2 0/5] ban mktemp(3)
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