Re: [PATCH 1/3] prune-packed: fix a possible buffer overflow
On Thu, Dec 19, 2013 at 05:33:55PM +0100, Michael Haggerty wrote: 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: 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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] prune-packed: fix a possible buffer overflow
On 12/19/2013 01:04 AM, Jeff King wrote: 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). [...] 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 mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] prune-packed: fix a possible buffer overflow
On 12/17/2013 07:43 PM, Junio C Hamano wrote: Duy Nguyen pclo...@gmail.com 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? [...] 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 mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] prune-packed: fix a possible buffer overflow
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] prune-packed: fix a possible buffer overflow
On Wed, Dec 18, 2013 at 1:43 AM, Junio C Hamano gits...@pobox.com wrote:
Duy Nguyen pclo...@gmail.com 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?
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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] prune-packed: fix a possible buffer overflow
On Tue, Dec 17, 2013 at 8:43 PM, Michael Haggerty mhag...@alum.mit.edu wrote:
The pathname character array might hold:
strlen(pathname) -- the pathname argument
'/' -- a slash, if not already present in pathname
%02x/-- the first two characters of the SHA-1 plus slash
38 characters-- the last 38 characters of the SHA-1
NUL -- terminator
-
strlen(pathname) + 43
(Actually, the NUL character is not written explicitly to pathname;
rather, the code relies on pathname being initialized to zeros and the
zero following the pathname still being there after the other
characters are written to the array.)
But the old pathname variable was PATH_MAX characters long, whereas
the check was (len PATH_MAX - 42). So there might have been one
byte too many stored in pathname. This would have resulted in it's
not being NUL-terminated.
So, increase the size of the pathname variable by one byte to avoid
this possibility.
Why don't we take this opportunity to replace that array with a
strbuf? The conversion looks simple with this function.
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
builtin/prune-packed.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/builtin/prune-packed.c b/builtin/prune-packed.c
index fa6ce42..81bc786 100644
--- a/builtin/prune-packed.c
+++ b/builtin/prune-packed.c
@@ -37,7 +37,7 @@ static void prune_dir(int i, DIR *dir, char *pathname, int
len, int opts)
void prune_packed_objects(int opts)
{
int i;
- static char pathname[PATH_MAX];
+ static char pathname[PATH_MAX + 1];
const char *dir = get_object_directory();
int len = strlen(dir);
@@ -45,7 +45,7 @@ void prune_packed_objects(int opts)
progress = start_progress_delay(Removing duplicate objects,
256, 95, 2);
- if (len PATH_MAX - 42)
+ if (len + 42 PATH_MAX)
die(impossible object directory);
memcpy(pathname, dir, len);
if (len pathname[len-1] != '/')
--
--
Duy
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] prune-packed: fix a possible buffer overflow
Duy Nguyen pclo...@gmail.com 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);
}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
