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

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Jan 15, 2008, 18:50 UTC
Message-ID: <alpine.LFD.1.00.0801151036110.2806@woody.linux-foundation.org>
In-Reply-To: <478CFAFF.6010006@nrlssc.navy.mil>

On Tue, 15 Jan 2008, Brandon Casey wrote:

Show 6 quoted lines

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

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;
 }