Re: [PATCH v3 2/2] wrapper: use a CSPRNG to generate random file names

[René Scharfe <l.s.r@xxxxxx>]

Am 18.01.22 um 18:25 schrieb Junio C Hamano:
> I agree with your analysis that the "diff" tempfiles do need suffix,
> we SHOULD create them in $TMPDIR (not in the working tree or
> $GIT_DIR) to support operation in a read-only repository, but we can
> create a unique temporary directory and place a file (even under its
> original name) in it as a workaround.

Here's a proof-of-concept patch for handling just that diff use-case
using mkdtemp.  Indeed it's nice to see the actual filename with
difftool.


 diff.c                   |  4 ++--
 t/t4020-diff-external.sh |  2 +-
 tempfile.c               | 20 ++++++++++++++++++++
 tempfile.h               |  1 +
 wrapper.c                | 12 ++++++++++++
 5 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/diff.c b/diff.c
index c862771a58..37db4743b0 100644
--- a/diff.c
+++ b/diff.c
@@ -4098,8 +4098,8 @@ static void prep_temp_blob(struct index_state *istate,

 	init_checkout_metadata(&meta, NULL, NULL, oid);

-	/* Generate "XXXXXX_basename.ext" */
-	strbuf_addstr(&tempfile, "XXXXXX_");
+	/* Generate "git-blob-XXXXXX/basename.ext" */
+	strbuf_addstr(&tempfile, "git-blob-XXXXXX/");
 	strbuf_addstr(&tempfile, base);

 	temp->tempfile = mks_tempfile_ts(tempfile.buf, strlen(base) + 1);
diff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh
index 54bb8ef27e..e7f93f36f5 100755
--- a/t/t4020-diff-external.sh
+++ b/t/t4020-diff-external.sh
@@ -217,7 +217,7 @@ test_expect_success 'GIT_EXTERNAL_DIFF generates pretty paths' '
 	touch file.ext &&
 	git add file.ext &&
 	echo with extension > file.ext &&
-	GIT_EXTERNAL_DIFF=echo git diff file.ext | grep ......_file\.ext &&
+	GIT_EXTERNAL_DIFF=echo git diff file.ext | grep git-blob-....../file\.ext &&
 	git update-index --force-remove file.ext &&
 	rm file.ext
 '
diff --git a/tempfile.c b/tempfile.c
index 94aa18f3f7..80cc9fb512 100644
--- a/tempfile.c
+++ b/tempfile.c
@@ -56,6 +56,21 @@

 static VOLATILE_LIST_HEAD(tempfile_list);

+static void remove_template_directory(struct tempfile *tempfile,
+				      int in_signal_handler)
+{
+	char *end = tempfile->filename.buf + tempfile->filename.len;
+	char *suffix = end - tempfile->suffixlen;
+	if (*suffix != '/')
+		return;
+	*suffix = '\0';
+	if (in_signal_handler)
+		rmdir(tempfile->filename.buf);
+	else
+		rmdir_or_warn(tempfile->filename.buf);
+	*suffix = '/';
+}
+
 static void remove_tempfiles(int in_signal_handler)
 {
 	pid_t me = getpid();
@@ -74,6 +89,7 @@ static void remove_tempfiles(int in_signal_handler)
 			unlink(p->filename.buf);
 		else
 			unlink_or_warn(p->filename.buf);
+		remove_template_directory(p, in_signal_handler);

 		p->active = 0;
 	}
@@ -100,6 +116,7 @@ static struct tempfile *new_tempfile(void)
 	tempfile->owner = 0;
 	INIT_LIST_HEAD(&tempfile->list);
 	strbuf_init(&tempfile->filename, 0);
+	tempfile->suffixlen = 0;
 	return tempfile;
 }

@@ -170,6 +187,7 @@ struct tempfile *mks_tempfile_sm(const char *filename_template, int suffixlen, i
 	struct tempfile *tempfile = new_tempfile();

 	strbuf_add_absolute_path(&tempfile->filename, filename_template);
+	tempfile->suffixlen = suffixlen;
 	tempfile->fd = git_mkstemps_mode(tempfile->filename.buf, suffixlen, mode);
 	if (tempfile->fd < 0) {
 		deactivate_tempfile(tempfile);
@@ -189,6 +207,7 @@ struct tempfile *mks_tempfile_tsm(const char *filename_template, int suffixlen,
 		tmpdir = "/tmp";

 	strbuf_addf(&tempfile->filename, "%s/%s", tmpdir, filename_template);
+	tempfile->suffixlen = suffixlen;
 	tempfile->fd = git_mkstemps_mode(tempfile->filename.buf, suffixlen, mode);
 	if (tempfile->fd < 0) {
 		deactivate_tempfile(tempfile);
@@ -316,6 +335,7 @@ void delete_tempfile(struct tempfile **tempfile_p)

 	close_tempfile_gently(tempfile);
 	unlink_or_warn(tempfile->filename.buf);
+	remove_template_directory(tempfile, 0);
 	deactivate_tempfile(tempfile);
 	*tempfile_p = NULL;
 }
diff --git a/tempfile.h b/tempfile.h
index 4de3bc77d2..b9a60f2431 100644
--- a/tempfile.h
+++ b/tempfile.h
@@ -82,6 +82,7 @@ struct tempfile {
 	FILE *volatile fp;
 	volatile pid_t owner;
 	struct strbuf filename;
+	size_t suffixlen;
 };

 /*
diff --git a/wrapper.c b/wrapper.c
index 36e12119d7..358db282f9 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -481,6 +481,18 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
 		return -1;
 	}

+	if (pattern[len - suffix_len] == '/') {
+		if (mode != 0600) {
+			errno = EINVAL;
+			return -1;
+		}
+		pattern[len - suffix_len] = '\0';
+		if (!mkdtemp(pattern))
+			return -1;
+		pattern[len - suffix_len] = '/';
+		return open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);
+	}
+
 	/*
 	 * Replace pattern's XXXXXX characters with randomness.
 	 * Try TMP_MAX different filenames.