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

From: Jeff King <peff@peff.net>
Date: Dec 19, 2013, 00:04 UTC
Message-ID: <20131219000415.GA17420@sigill.intra.peff.net>
In-Reply-To: <52B2256A.3060802@alum.mit.edu>

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

Show 7 quoted lines

> [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