Re: git-commit fatal: Out of memory? mmap failed: Bad file descriptor

From: Kristian Høgsberg <krh@redhat.com>
Date: Jan 15, 2008, 20:00 UTC
Message-ID: <1200427202.5821.7.camel@gaara.boston.redhat.com>
In-Reply-To: <478D0CDA.5050709@nrlssc.navy.mil>

On Tue, 2008-01-15 at 13:43 -0600, Brandon Casey wrote:

Show 29 quoted lines

> Linus Torvalds wrote:
> > 
> > On Tue, 15 Jan 2008, Brandon Casey wrote:
> >> Linus Torvalds wrote:
> >>> It would obviously be interesting to see the base repository and the 
> >>> commit you are trying to do - is that possibly publicly available?
> >> I wish it was.
> > 
> > It's ok, I found the bug in your full strace.
> 
> Good catch, but that wasn't it. Still getting the same error.
> 
> > and now it's trying to close that fd *again*
> 
> In that same vein, just above your changes in prepare_index() is:
> 
> 	if (!pathspec || !*pathspec) {
> 		fd = hold_locked_index(&index_lock, 1);
> 		refresh_cache(REFRESH_QUIET);
> 		if (write_cache(fd, active_cache, active_nr) ||
> 		    close(fd) || commit_locked_index(&index_lock))
> 			die("unable to write new_index file");
> 		commit_style = COMMIT_AS_IS;
> 		return get_index_file();
> 	}
> 
> If I followed hold_locked_index() correctly, then fd and index_lock.fd
> are equal, and commit_locked_index() does a close(lk->fd) making the
> close(fd) above, redundant (or vice-versa).

To my defense, the lockfile API is used a little inconsitently in git. Many places in git does a close(fd) and the call commit_locked_index(), which will close the fd again. Normally that will just cause an EBADFD which we ignore, but the problem here is that there's a longer time between close(fd) and the commit/rollback of the lock file. I guess the correct way to use the API is to never close the fd manually, but I copied and pasted the lockfile use in builtin-commit.c from somewhere else and along with it the double close.

There's four close(fd) calls in prepare_index() and they're all incorrect. The open fd's are cleaned up in rollback_index_files() and shouldn't be closed manually. The patch below gets rid of the extra close() calls and should fix the problem.

cheers, Kristian

diff --git a/builtin-commit.c b/builtin-commit.c
index 73f1e35..4494c9c 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -212,7 +212,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix)
 		int fd = hold_locked_index(&index_lock, 1);
 		add_files_to_cache(0, also ? prefix : NULL, pathspec);
 		refresh_cache(REFRESH_QUIET);
-		if (write_cache(fd, active_cache, active_nr) || close(fd))
+		if (write_cache(fd, active_cache, active_nr))
 			die("unable to write new_index file");
 		commit_style = COMMIT_NORMAL;
 		return index_lock.filename;
@@ -231,7 +231,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix)
 		fd = hold_locked_index(&index_lock, 1);
 		refresh_cache(REFRESH_QUIET);
 		if (write_cache(fd, active_cache, active_nr) ||
-		    close(fd) || commit_locked_index(&index_lock))
+		    commit_locked_index(&index_lock))
 			die("unable to write new_index file");
 		commit_style = COMMIT_AS_IS;
 		return get_index_file();
@@ -273,7 +273,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix)
 	fd = hold_locked_index(&index_lock, 1);
 	add_remove_files(&partial);
 	refresh_cache(REFRESH_QUIET);
-	if (write_cache(fd, active_cache, active_nr) || close(fd))
+	if (write_cache(fd, active_cache, active_nr))
 		die("unable to write new_index file");
 
 	fd = hold_lock_file_for_update(&false_lock,
@@ -289,7 +289,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix)
 	add_remove_files(&partial);
 	refresh_cache(REFRESH_QUIET);
 
-	if (write_cache(fd, active_cache, active_nr) || close(fd))
+	if (write_cache(fd, active_cache, active_nr))
 		die("unable to write temporary index file");
 	return false_lock.filename;
 }