Re: [PATCH 2/3] prune_object_dir(): verify that path fits in the temporary buffer

[Jeff King <peff@peff.net>]

On Tue, Dec 17, 2013 at 10:51:30AM -0800, Junio C Hamano wrote:

> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
> > Dimension the buffer based on PATH_MAX rather than a magic number, and
> > verify that the path fits in it before continuing.
> >
> > Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> > ---
> >
> > I don't think that this problem is remotely exploitable, because the
> > size of the string doesn't depend on inputs that can be influenced by
> > a client (at least not within Git).
> 
> This is shrinking the buffer on some platforms where PATH_MAX is
> lower than 4k---granted, we will die() with the new check instead of
> crashing uncontrolled, but it still feels somewhat wrong.

On such a system, though, does the resulting prune_dir call actually do
anything? We will feed the result to opendir(), which I would think
would choke on the long name.

Still, it is probably better to do everything we can and let the OS
choke (especially because we would want to continue operating on other
paths in this case).

Converting it to use strbuf looks like it will actually let us drop a
bunch of copying, too, as we just end up in mkpath at the very lowest
level. I.e., something like below.

As an aside, I have noticed us using this "push/pop" approach to treating a
strbuf as a stack of paths elsewhere, too. I.e:

  size_t baselen = base->len;
  strbuf_addf(base, "/%s", some_thing);
  some_call(base);
  base->len = baselen;

I wondered if there was any kind of helper we could add to make it look
nicer. But I don't think so; the hairy part is that you must remember to
reset base->len after the call, and there is no easy way around that in
C. If you had object destructors that ran as the stack unwound, or
dynamic scoping, it would be easy to manipulate the object. Wrapping
won't work because strbuf isn't just a length wrapping an immutable
buffer; it actually has to move the NUL in the buffer.

Anyway, not important, but perhaps somebody is more clever than I am.

diff --git a/builtin/prune.c b/builtin/prune.c
index 6366917..4ca8ec1 100644
--- a/builtin/prune.c
+++ b/builtin/prune.c
@@ -17,9 +17,8 @@ static int verbose;
 static unsigned long expire;
 static int show_progress = -1;
 
-static int prune_tmp_object(const char *path, const char *filename)
+static int prune_tmp_object(const char *fullpath)
 {
-	const char *fullpath = mkpath("%s/%s", path, filename);
 	struct stat st;
 	if (lstat(fullpath, &st))
 		return error("Could not stat '%s'", fullpath);
@@ -32,9 +31,8 @@ static int prune_tmp_object(const char *path, const char *filename)
 	return 0;
 }
 
-static int prune_object(char *path, const char *filename, const unsigned char *sha1)
+static int prune_object(const char *fullpath, const unsigned char *sha1)
 {
-	const char *fullpath = mkpath("%s/%s", path, filename);
 	struct stat st;
 	if (lstat(fullpath, &st))
 		return error("Could not stat '%s'", fullpath);
@@ -50,9 +48,10 @@ static int prune_object(char *path, const char *filename, const unsigned char *s
 	return 0;
 }
 
-static int prune_dir(int i, char *path)
+static int prune_dir(int i, struct strbuf *path)
 {
-	DIR *dir = opendir(path);
+	size_t baselen = path->len;
+	DIR *dir = opendir(path->buf);
 	struct dirent *de;
 
 	if (!dir)
@@ -77,28 +76,39 @@ static int prune_dir(int i, char *path)
 			if (lookup_object(sha1))
 				continue;
 
-			prune_object(path, de->d_name, sha1);
+			strbuf_addf(path, "/%s", de->d_name);
+			prune_object(path->buf, sha1);
+			path->len = baselen;
 			continue;
 		}
 		if (!prefixcmp(de->d_name, "tmp_obj_")) {
-			prune_tmp_object(path, de->d_name);
+			strbuf_addf(path, "/%s", de->d_name);
+			prune_tmp_object(path->buf);
+			path->len = baselen;
 			continue;
 		}
-		fprintf(stderr, "bad sha1 file: %s/%s\n", path, de->d_name);
+		fprintf(stderr, "bad sha1 file: %s/%s\n", path->buf, de->d_name);
 	}
 	closedir(dir);
 	if (!show_only)
-		rmdir(path);
+		rmdir(path->buf);
 	return 0;
 }
 
 static void prune_object_dir(const char *path)
 {
+	struct strbuf buf = STRBUF_INIT;
+	size_t baselen;
 	int i;
+
+	strbuf_addstr(&buf, path);
+	strbuf_addch(&buf, '/');
+	baselen = buf.len;
+
 	for (i = 0; i < 256; i++) {
-		static char dir[4096];
-		sprintf(dir, "%s/%02x", path, i);
-		prune_dir(i, dir);
+		strbuf_addf(&buf, "%02x", i);
+		prune_dir(i, &buf);
+		buf.len = baselen;
 	}
 }
 
@@ -120,7 +130,7 @@ static void remove_temporary_files(const char *path)
 	}
 	while ((de = readdir(dir)) != NULL)
 		if (!prefixcmp(de->d_name, "tmp_"))
-			prune_tmp_object(path, de->d_name);
+			prune_tmp_object(mkpath("%s/%s", path, de->d_name));
 	closedir(dir);
 }