Re: [PATCH 1/3] prune-packed: fix a possible buffer overflow

Duy Nguyen [off-list ref] writes:

Why don't we take this opportunity to replace that array with a
strbuf? The conversion looks simple with this function.

Indeed.  Something like this, perhaps?

 builtin/prune-packed.c | 38 +++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/builtin/prune-packed.c b/builtin/prune-packed.c
index fa6ce42..fcf5fb6 100644
--- a/builtin/prune-packed.c
+++ b/builtin/prune-packed.c

@@ -10,58 +10,62 @@ static const char * const prune_packed_usage[] = {
 
 static struct progress *progress;
 
-static void prune_dir(int i, DIR *dir, char *pathname, int len, int opts)
+static void prune_dir(int i, DIR *dir, struct strbuf *pathname, int opts)
 {
 	struct dirent *de;
 	char hex[40];
+	int top_len = pathname->len;
 
 	sprintf(hex, "%02x", i);
 	while ((de = readdir(dir)) != NULL) {
 		unsigned char sha1[20];
 		if (strlen(de->d_name) != 38)
 			continue;
-		memcpy(hex+2, de->d_name, 38);
+		memcpy(hex + 2, de->d_name, 38);
 		if (get_sha1_hex(hex, sha1))
 			continue;
 		if (!has_sha1_pack(sha1))
 			continue;
-		memcpy(pathname + len, de->d_name, 38);
+
+		strbuf_add(pathname, de->d_name, 38);
 		if (opts & PRUNE_PACKED_DRY_RUN)
-			printf("rm -f %s\n", pathname);
+			printf("rm -f %s\n", pathname->buf);
 		else
-			unlink_or_warn(pathname);
+			unlink_or_warn(pathname->buf);
 		display_progress(progress, i + 1);
+		strbuf_setlen(pathname, top_len);
 	}
 }
 
 void prune_packed_objects(int opts)
 {
 	int i;
-	static char pathname[PATH_MAX];
 	const char *dir = get_object_directory();
-	int len = strlen(dir);
+	struct strbuf pathname = STRBUF_INIT;
+	int top_len;
 
+	strbuf_addstr(&pathname, dir);
 	if (opts & PRUNE_PACKED_VERBOSE)
 		progress = start_progress_delay("Removing duplicate objects",
 			256, 95, 2);
 
-	if (len > PATH_MAX - 42)
-		die("impossible object directory");
-	memcpy(pathname, dir, len);
-	if (len && pathname[len-1] != '/')
-		pathname[len++] = '/';
+	if (pathname.len && pathname.buf[pathname.len - 1] != '/')
+		strbuf_addch(&pathname, '/');
+
+	top_len = pathname.len;
 	for (i = 0; i < 256; i++) {
 		DIR *d;
 
 		display_progress(progress, i + 1);
-		sprintf(pathname + len, "%02x/", i);
-		d = opendir(pathname);
+		strbuf_setlen(&pathname, top_len);
+		strbuf_addf(&pathname, "%02x/", i);
+		d = opendir(pathname.buf);
 		if (!d)
 			continue;
-		prune_dir(i, d, pathname, len + 3, opts);
+		prune_dir(i, d, &pathname, opts);
 		closedir(d);
-		pathname[len + 2] = '\0';
-		rmdir(pathname);
+		strbuf_setlen(&pathname, top_len + 2);
+		rmdir(pathname.buf);
 	}
 	stop_progress(&progress);
 }

On 12/17/2013 07:43 PM, Junio C Hamano wrote:

Duy Nguyen [off-list ref] writes:

quoted

Why don't we take this opportunity to replace that array with a
strbuf? The conversion looks simple with this function.

Indeed.  Something like this, perhaps?
[...]

Frankly, with my initial patches I was just trying to paper over the bug
with the smallest possible change.  It's nice that people are attempting
bigger improvements.

I went in a slightly different direction: I am spiking out an API for
iterating over loose object files.  It would be useful in a couple of
places.

[While doing so, I got sidetracked by the question: what happens if a
prune process deletes the "objects/XX" directory just the same moment
that another process is trying to write an object into that directory?
I think the relevant function is sha1_file.c:create_tmpfile().  It looks
like there is a nonzero but very small race window that could result in
a spurious "unable to create temporary file" error, but even then I
don't think there would be any corruption or anything.]

But don't let me stop you; the cleanups you are working on are
definitely nice and are complementary to my ideas.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

On Wed, Dec 18, 2013 at 11:44:58PM +0100, Michael Haggerty wrote:

[While doing so, I got sidetracked by the question: what happens if a
prune process deletes the "objects/XX" directory just the same moment
that another process is trying to write an object into that directory?
I think the relevant function is sha1_file.c:create_tmpfile().  It looks
like there is a nonzero but very small race window that could result in
a spurious "unable to create temporary file" error, but even then I
don't think there would be any corruption or anything.]

There's a race there, but I think it's hard to trigger.

Our strategy with object creation is to call open, recognize ENOENT,
mkdir, and then try again. If the rmdir happens before our call to open,
then we're fine. If open happens first, then the rmdir will fail.

But we don't loop on ENOENT. So if the rmdir happens in the middle,
after the mkdir but before we call open again, we'd fail, because we
don't treat ENOENT specially in the second call to open. That is
unlikely to happen, though, as prune would not be removing a directory
it did not just enter and clean up an object from (in which case we
would not have gotten the first ENOENT in the creator). I think you'd So
you'd have to have something creating and then pruning the directory in
the time between our open and mkdir. It would probably be more likely to
see it if you had two prunes running (the first one kills the directory,
creator notices and calls mkdir, then the second prune kills the
directory, too).

So it seems unlikely and the worst case is a temporary failure, not a
corruption. It's probably not worth caring too much about, but we could
solve it pretty easily by looping on ENOENT on creation.

On a similar note, I imagine that a simultaneous "branch foo/bar" and
"branch -d foo/baz" could race over the creation/deletion of
"refs/heads/foo", but I didn't look into it.

-Peff

On Wed, Dec 18, 2013 at 1:43 AM, Junio C Hamano [off-list ref] wrote:

Duy Nguyen [off-list ref] writes:

quoted

Why don't we take this opportunity to replace that array with a
strbuf? The conversion looks simple with this function.

Indeed.  Something like this, perhaps?

Yes, looking good.

 void prune_packed_objects(int opts)
 {
        int i;
-       static char pathname[PATH_MAX];
        const char *dir = get_object_directory();
-       int len = strlen(dir);
+       struct strbuf pathname = STRBUF_INIT;
+       int top_len;

+       strbuf_addstr(&pathname, dir);
        if (opts & PRUNE_PACKED_VERBOSE)
                progress = start_progress_delay("Removing duplicate objects",
                        256, 95, 2);

-       if (len > PATH_MAX - 42)
-               die("impossible object directory");
-       memcpy(pathname, dir, len);
-       if (len && pathname[len-1] != '/')
-               pathname[len++] = '/';
+       if (pathname.len && pathname.buf[pathname.len - 1] != '/')
+               strbuf_addch(&pathname, '/');

I see this pattern (add a trailing slash) in a few places too. Maybe
we could make a wrapper for it.
-- 
Duy

On 12/19/2013 01:04 AM, Jeff King wrote:

On Wed, Dec 18, 2013 at 11:44:58PM +0100, Michael Haggerty wrote:

quoted

[While doing so, I got sidetracked by the question: what happens if a
prune process deletes the "objects/XX" directory just the same moment
that another process is trying to write an object into that directory?
I think the relevant function is sha1_file.c:create_tmpfile().  It looks
like there is a nonzero but very small race window that could result in
a spurious "unable to create temporary file" error, but even then I
don't think there would be any corruption or anything.]

There's a race there, but I think it's hard to trigger.

Our strategy with object creation is to call open, recognize ENOENT,
mkdir, and then try again. If the rmdir happens before our call to open,
then we're fine. If open happens first, then the rmdir will fail.

But we don't loop on ENOENT. So if the rmdir happens in the middle,
after the mkdir but before we call open again, we'd fail, because we
don't treat ENOENT specially in the second call to open. That is
unlikely to happen, though, as prune would not be removing a directory
it did not just enter and clean up an object from (in which case we
would not have gotten the first ENOENT in the creator). [...]

The way I read it, prune tries to delete the directory whether or not
there were any files in it.  So the race could be triggered by a single
writer that wants to write an object to a not-yet-existent shard
directory and a single prune process that encounters the directory
between when it is created and when the object file is added.

But that doesn't mean I disagree with your conclusion:

So it seems unlikely and the worst case is a temporary failure, not a
corruption. It's probably not worth caring too much about, but we could
solve it pretty easily by looping on ENOENT on creation.

Regarding references:

On a similar note, I imagine that a simultaneous "branch foo/bar" and
"branch -d foo/baz" could race over the creation/deletion of
"refs/heads/foo", but I didn't look into it.

Deleting a loose reference doesn't cause the directory containing it to
be deleted.  The directory is only deleted by pack-refs (and then only
when a reference in the directory was just packed) or when there is an
attempt to create a new reference that conflicts with the directory.  So
the question is whether the creation of a loose ref file is robust
against the disappearance of a directory that it just created.

And the answer is "no".  It looks like there are a bunch of places where
similar races occur involving references.  And probably many others
elsewhere in the code.  (Any caller of safe_create_leading_directories()
is a candidate problem point, and in fact that function itself has an
internal race.)  I've started fixing some of these but it might take a
while.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

On Thu, Dec 19, 2013 at 05:33:55PM +0100, Michael Haggerty wrote:

quoted

But we don't loop on ENOENT. So if the rmdir happens in the middle,
after the mkdir but before we call open again, we'd fail, because we
don't treat ENOENT specially in the second call to open. That is
unlikely to happen, though, as prune would not be removing a directory
it did not just enter and clean up an object from (in which case we
would not have gotten the first ENOENT in the creator). [...]

The way I read it, prune tries to delete the directory whether or not
there were any files in it.  So the race could be triggered by a single
writer that wants to write an object to a not-yet-existent shard
directory and a single prune process that encounters the directory
between when it is created and when the object file is added.

Yes, that's true. It does make the race slightly more difficult than a
straight deletion because the prune has to catch it in the moment where
it exists but does not yet have an object. But it's still possible.

But that doesn't mean I disagree with your conclusion:

I think we're in violent agreement at this point. :)

Regarding references:

quoted

On a similar note, I imagine that a simultaneous "branch foo/bar" and
"branch -d foo/baz" could race over the creation/deletion of
"refs/heads/foo", but I didn't look into it.

Deleting a loose reference doesn't cause the directory containing it to
be deleted.  The directory is only deleted by pack-refs (and then only
when a reference in the directory was just packed) or when there is an
attempt to create a new reference that conflicts with the directory.  So
the question is whether the creation of a loose ref file is robust
against the disappearance of a directory that it just created.

Ah, right, I forgot we leave the directories sitting around after
deletion. So we may run into a collision with another creator, but by
definition we would have a D/F conflict with such a creator anyway, so
we cannot both succeed.

But we can hit the problem with pack-refs, as you note:

And the answer is "no".  It looks like there are a bunch of places where
similar races occur involving references.  And probably many others
elsewhere in the code.  (Any caller of safe_create_leading_directories()
is a candidate problem point, and in fact that function itself has an
internal race.)  I've started fixing some of these but it might take a
while.

Yeah, I think you'd have to teach safe_create_leading_directories to
atomically try-to-create-and-check-errno rather than stat+mkdir. And
then teach it to backtrack when an expected leading path goes missing
after we created it (so mkdir("foo"), then mkdir("foo/bar"), then step
back to mkdir("foo") if we got ENOENT).

I don't think the races are a big deal, though. As with the prune case,
we will ultimately fail to create the lockfile and get a temporary
failure rather than a corruption. So unless we actually have reports of
it happening (and I have seen none), it's probably not worth spending
much time on.

-Peff