threads / patch / 9227

patchFix git_mkstemp to return an error when path is too long.

Subject: [PATCH] Fix git_mkstemp to return an error when path is too long.

## tl;dr

4 messages between Jul 26, 2007 and Jul 26, 2007. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Carlos Rica· Jul 26, 2007, 01:32 UTC · lore

Now the function returns -2 to the caller if the given buffer is too short to save the entire path for the temporary file.

Signed-off-by: Carlos Rica <jasampler@gmail.com>
---
 diff.c |    2 ++
 path.c |    9 ++++++---
 2 files changed, 8 insertions(+), 3 deletions(-)

Show changes to 2 files +8 −3

diff.c, path.c

diff --git a/diff.c b/diff.c
index cd6b0c4..8735467 100644
--- a/diff.c
+++ b/diff.c
@@ -1694,6 +1694,8 @@ static void prep_temp_blob(struct diff_tempfile *temp,
 	int fd;

 	fd = git_mkstemp(temp->tmp_path, PATH_MAX, ".diff_XXXXXX");
+	if (fd == -2)
+		die("path too long for temp-file");
 	if (fd < 0)
 		die("unable to create temp-file");
 	if (write_in_full(fd, blob, size) != size)
diff --git a/path.c b/path.c
index c4ce962..f33d15d 100644
--- a/path.c
+++ b/path.c
@@ -68,7 +68,8 @@ char *git_path(const char *fmt, ...)
 }


-/* git_mkstemp() - create tmp file honoring TMPDIR variable */
+/* git_mkstemp() - create tmp file honoring TMPDIR variable.
+ * return -2 if path is too long to have it concatenated. */
 int git_mkstemp(char *path, size_t len, const char *template)
 {
 	char *env, *pch = path;
@@ -79,12 +80,14 @@ int git_mkstemp(char *path, size_t len, const char *template)
 		pch += 5;
 	} else {
 		size_t n = snprintf(pch, len, "%s/", env);
-
+		if (n >= len)
+			return -2;
 		len -= n;
 		pch += n;
 	}

-	strlcpy(pch, template, len);
+	if (strlcpy(pch, template, len) >= len)
+		return -2;

 	return mkstemp(path);
 }

-- 
1.5.0

Johannes Schindelin· Jul 26, 2007, 03:36 UTC · re: Carlos Rica · lore

Re: [PATCH] Fix git_mkstemp to return an error when path is too long.

Hi,

On Thu, 26 Jul 2007, Carlos Rica wrote:

Show 7 quoted lines

> @@ -79,12 +80,14 @@ int git_mkstemp(char *path, size_t len, const char *template)
>  		pch += 5;
>  	} else {
>  		size_t n = snprintf(pch, len, "%s/", env);
> -
> +		if (n >= len)
> +			return -2;

That is certainly a bug fixed (even if few people have an insanely long TMPDIR...)

Show 7 quoted lines

>  		len -= n;
>  		pch += n;
>  	}
> 
> -	strlcpy(pch, template, len);
> +	if (strlcpy(pch, template, len) >= len)
> +		return -2;

Maybe just "return error("filename too long: %.*s", 60, pch);"? So that all callers to git_mkstemp() get the message for free?

Ciao, Dscho

Junio C Hamano· Jul 26, 2007, 04:26 UTC · re: Carlos Rica · lore

Re: [PATCH] Fix git_mkstemp to return an error when path is too long.

Carlos Rica <jasampler@gmail.com> writes:

> Now the function returns -2 to the caller if the given buffer
> is too short to save the entire path for the temporary file.
>
> Signed-off-by: Carlos Rica <jasampler@gmail.com>

Trying not to overstep the static buffer is of course a good thing, and I think it is probably Ok to error out on a TMPDIR environment variable that is insanely long, instead of using an extra malloc/free, as underlying mkstemp() would error out if it is given an insanely long template buffer anyway.

However, I think it is not so useful to return -2, even if that is done so that you can differentiate the case where the TMPDIR and/or the template were too long and the case mkstemp() errored out.

Stop and think for a minute: what does the underlying mkstemp() do, if the given template is too long?

That's right. You would get ENAMETOOLONG. So why don't we do this instead?

---
 diff.c |    2 +-
 path.c |   25 ++++++++++---------------
 2 files changed, 11 insertions(+), 16 deletions(-)

Show changes to 2 files +11 −16

diff.c, path.c

diff --git a/diff.c b/diff.c
index cd6b0c4..a5fc56b 100644
--- a/diff.c
+++ b/diff.c
@@ -1695,7 +1695,7 @@ static void prep_temp_blob(struct diff_tempfile *temp,
 
 	fd = git_mkstemp(temp->tmp_path, PATH_MAX, ".diff_XXXXXX");
 	if (fd < 0)
-		die("unable to create temp-file");
+		die("unable to create temp-file: %s", strerror(errno));
 	if (write_in_full(fd, blob, size) != size)
 		die("unable to write temp-file");
 	close(fd);
diff --git a/path.c b/path.c
index c4ce962..dc7ded9 100644
--- a/path.c
+++ b/path.c
@@ -71,25 +71,20 @@ char *git_path(const char *fmt, ...)
 /* git_mkstemp() - create tmp file honoring TMPDIR variable */
 int git_mkstemp(char *path, size_t len, const char *template)
 {
-	char *env, *pch = path;
-
-	if ((env = getenv("TMPDIR")) == NULL) {
-		strcpy(pch, "/tmp/");
-		len -= 5;
-		pch += 5;
-	} else {
-		size_t n = snprintf(pch, len, "%s/", env);
-
-		len -= n;
-		pch += n;
+	const char *tmp;
+	size_t n;
+
+	tmp = getenv("TMPDIR");
+	if (!tmp)
+		tmp = "/tmp";
+	n = snprintf(path, len, "%s/%s", tmp, template);
+	if (len <= n) {
+		errno = ENAMETOOLONG;
+		return -1;
 	}
-
-	strlcpy(pch, template, len);
-
 	return mkstemp(path);
 }
 
-
 int validate_headref(const char *path)
 {
 	struct stat st;

Carlos Rica· Jul 26, 2007, 19:42 UTC · re: Junio C Hamano · lore

Re: [PATCH] Fix git_mkstemp to return an error when path is too long.

I like that new solution. ENAMETOOLONG is fantastic!

Usually, the function is called using the long PATH_MAX constant, however, the function doesn't force you to supply an insanely long buffer like that, and therefore is nice to say when the limit is exceeded.