From: René Scharfe Date: Fri, 05 Dec 2025 12:11:40 GMT Subject: Re: [PATCH 2/4] compat: use git_mkdtemp() Message-ID: In-Reply-To: <20251203161154.GA44940@coredump.intra.peff.net> 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é