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

On Tue, 15 Jan 2008, Brandon Casey wrote:

Linus Torvalds wrote:

quoted

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.

The bug really is pretty stupid:

 - prepare_index() does a 

	fd = hold_lock_file_for_update(&false_lock, ...
	...
	if (write_cache(fd, active_cache, active_nr) || close(fd))
		die("unable to write temporary index file");

and the magic here is that *it*closes*the*fd*.

But that's not how "hold_lock_file_for_update()" works. It still has that 
fd squirrelled away in it's "false_lock.fd", and later on, when we do 

	rollback_lock_file(&false_lock);

(in the COMMIT_PARTIAL case of either "commit_index_files()" or 
"rollback_index_files()"), that rollback_lock_file() will do:

	void rollback_lock_file(struct lock_file *lk)
	{
	        if (lk->filename[0]) {
	                close(lk->fd);
	                unlink(lk->filename);
	        }
	        lk->filename[0] = 0;
	}

and now it's trying to close that fd *again* and would normally get a 
EBADF there. But in the meantime, somebody already re-used it for 
something else, and what rollback_lock_file() ends up doing is to just 
close some random file descriptor.

In other words, I'm pretty sure that the bug goes away with this really 
ugly hack. The real problem is that that "false_lockfile" thing simply 
mis-uses the whole lockfile interface. So this is not a pretty fix, but it 
at least should hide the effects of the mis-use of the interface.

In other words: the file descriptor that is returned by the lock_file 
interface functions *MUST*NOT* be closed. But if you violate that rule, 
you'd better make sure that you also fix the effects.

		Linus

---
 builtin-commit.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 265ba6b..7a52224 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c

@@ -308,6 +308,9 @@ static char *prepare_index(int argc, const char **argv, const char *prefix)
 
 	if (write_cache(fd, active_cache, active_nr) || close(fd))
 		die("unable to write temporary index file");
+
+	/* We closed the false lock-file fd, make sure we don't do anything else to it */
+	false_lock.fd = -1;
 	return false_lock.filename;
 }