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

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

[Brandon Casey]

I got this message from git-commit:

$ git commit -a
<edit message, :wq>
fatal: Out of memory? mmap failed: Bad file descriptor
Create commit <my_prompt_string>

The exit status was 128.
Looks like the commit was successful though.
The partial message 'Create commit ' comes from print_summary()
in builtin-commit.c which is _after_ the actual commit.

$ git --version
git version 1.5.4.rc2.84.gf85fd-dirty

It was compiled with NO_CURL=1. The dirtiness comes from the
patches I submitted for relink earlier today.

The other possible clue is that this repo is on NFS.

-brandon

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

[Charles Bailey]

Brandon Casey wrote:
> I got this message from git-commit:
> 
> $ git commit -a
> <edit message, :wq>
> fatal: Out of memory? mmap failed: Bad file descriptor
> Create commit <my_prompt_string>
> 
> The exit status was 128.
> Looks like the commit was successful though.
> The partial message 'Create commit ' comes from print_summary()
> in builtin-commit.c which is _after_ the actual commit.
> 
> $ git --version
> git version 1.5.4.rc2.84.gf85fd-dirty
> 
> It was compiled with NO_CURL=1. The dirtiness comes from the
> patches I submitted for relink earlier today.
> 
> The other possible clue is that this repo is on NFS.
> 
> -brandon

I have seen this exact type of failure (commit reports possible 
oom, but commit appears to have succeeded) with most recent gits.

I had assumed that it was because I was using a very large 
repository (experimenting with using git for general backup 
purposes) on a machine with not too much memory.

Perhaps there's a real bug in here somewhere after all.

Charles.

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

[Jeff King]

On Fri, Jan 11, 2008 at 10:18:32PM +0000, Charles Bailey wrote:

> I have seen this exact type of failure (commit reports possible oom, but 
> commit appears to have succeeded) with most recent gits.

This is almost certainly caused not by the commit action itself (which
uses very little memory) but by the resulting diffstat to show what
happened. So the commit has already been "committed" to disk by the time
it crashes.

This is at least the case with Brandon's problem (his stack trace shows
the diff happening).

-Peff

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

[Marco Costalba]

On Jan 11, 2008 11:11 PM, Brandon Casey <casey@nrlssc.navy.mil> wrote:
>
> I got this message from git-commit:
>
> $ git commit -a
> <edit message, :wq>
> fatal: Out of memory? mmap failed: Bad file descriptor
> Create commit <my_prompt_string>
>
> The exit status was 128.
> Looks like the commit was successful though.
> The partial message 'Create commit ' comes from print_summary()
> in builtin-commit.c which is _after_ the actual commit.
>
> $ git --version
> git version 1.5.4.rc2.84.gf85fd-dirty
>

I had the same message about one week ago for few times, same
symptoms, I didn't had the time to dig it out and today it seems no
more happening.


> It was compiled with NO_CURL=1. The dirtiness comes from the
> patches I submitted for relink earlier today.
>
> The other possible clue is that this repo is on NFS.
>

I don't use any NFS mount.


Marco

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

[Brandon Casey]

It's reproduceable for me by amending the commit.

Any suggestions?

-brandon



Brandon Casey wrote:
> I got this message from git-commit:
> 
> $ git commit -a
> <edit message, :wq>
> fatal: Out of memory? mmap failed: Bad file descriptor
> Create commit <my_prompt_string>
> 
> The exit status was 128.
> Looks like the commit was successful though.
> The partial message 'Create commit ' comes from print_summary()
> in builtin-commit.c which is _after_ the actual commit.
> 
> $ git --version
> git version 1.5.4.rc2.84.gf85fd-dirty
> 
> It was compiled with NO_CURL=1. The dirtiness comes from the
> patches I submitted for relink earlier today.
> 
> The other possible clue is that this repo is on NFS.
> 
> -brandon
> 
> -
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

[Junio C Hamano]

Brandon Casey <casey@nrlssc.navy.mil> writes:

> It's reproduceable for me by amending the commit.

Reliably reproducible?  Can you build with "-O0 -g" and run
"commit --amend" under gdb?

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

[Brandon Casey]

Junio C Hamano wrote:
> Brandon Casey <casey@nrlssc.navy.mil> writes:
> 
>> It's reproduceable for me by amending the commit.
> 
> Reliably reproducible?  Can you build with "-O0 -g" and run
> "commit --amend" under gdb?
> 

make NO_CURL=1 CFLAGS='-O0 -g'
Done.

I also moved xmmap into commit.c, and turned the inlined definition
in git-compat-util.h into a declaration.


I set a breakpoint on xmmap(). This is the backtrace on the last entry
into xmmap() before it die()'ed.

The fstat message at the end is from a call to fstat that I added to
print out the file size (to compare with mmap length). As you can
see the fstat also fails with the 'Bad file descriptor' message.


#0  xmmap (start=0x0, length=996168, prot=1, flags=2, fd=6, offset=0)
    at commit.c:680
#1  0x080acf30 in use_pack (p=0x8150650, w_cursor=0xffffc0ac, offset=94828, 
    left=0xffffc06c) at sha1_file.c:748
#2  0x080ae169 in unpack_object_header (p=0x8150650, w_curs=0xffffc0ac, 
    curpos=0xffffc0a0, sizep=0xffffc1d0) at sha1_file.c:1333
#3  0x080ae8bb in unpack_entry (p=0x8150650, obj_offset=94828, 
    type=0xffffc1dc, sizep=0xffffc1d0) at sha1_file.c:1595
#4  0x080ae55d in cache_or_unpack_entry (p=0x8150650, base_offset=94828, 
    base_size=0xffffc1d0, type=0xffffc1dc, keep_cache=1) at sha1_file.c:1490
#5  0x080af057 in read_packed_sha1 (
    sha1=0xffffc1b0 "��\034\f~\023\203��E=�$n~��X��@�220", 
    type=0xffffc1dc, size=0xffffc1d0) at sha1_file.c:1815
#6  0x080af2cf in read_sha1_file (
    sha1=0xffffc1b0 "��\034\f~\023\203��E=�$n~��X��@�220", 
    type=0xffffc1dc, size=0xffffc1d0) at sha1_file.c:1881
#7  0x080af3a0 in read_object_with_reference (
    sha1=0x853cf18 "��\034\f~\023\203��E=�$n~��40000 libapsrs", 
    required_type_name=0x80f8488 "tree", size=0xffffc20c, 
    actual_sha1_return=0x0) at sha1_file.c:1910
#8  0x080cd2d0 in diff_tree_sha1 (
    old=0x853cf18 "��\034\f~\023\203��E=�$n~��40000 libapsrs", 
    new=0x853d108 "��\036\034_�006�\f\025\236{�220StM0�40000 libapsrs", 
    base=0x8534098 "aps/src/libapsnav/", opt=0xffffc694) at tree-diff.c:376
#9  0x080cc9cb in compare_tree_entry (t1=0xffffc330, t2=0xffffc310, 
    base=0x815a260 "aps/src/", baselen=8, opt=0xffffc694) at tree-diff.c:61
#10 0x080ccf93 in diff_tree (t1=0xffffc330, t2=0xffffc310, 
    base=0x815a260 "aps/src/", opt=0xffffc694) at tree-diff.c:278
#11 0x080cd371 in diff_tree_sha1 (
    old=0x853cbe0 "N|\021xH/K<R�\025��\2250�00644 stamp-h.in", 
    new=0x853cdb0 "\0027w���}��Ƴ\213\036±|100644 stamp-h.in", 
    base=0x815a260 "aps/src/", opt=0xffffc694) at tree-diff.c:384
---Type <return> to continue, or q <return> to quit---
#12 0x080cc9cb in compare_tree_entry (t1=0xffffc430, t2=0xffffc410, 
    base=0x817d3b8 "aps/", baselen=4, opt=0xffffc694) at tree-diff.c:61
#13 0x080ccf93 in diff_tree (t1=0xffffc430, t2=0xffffc410, 
    base=0x817d3b8 "aps/", opt=0xffffc694) at tree-diff.c:278
#14 0x080cd371 in diff_tree_sha1 (
    old=0x853c580 "\037\215��\217\200�E�b��232�03640000 avhrr", 
    new=0x853c848 "g\230\032a \207V�s~��r\177�23540000 avhrr", 
    base=0x817d3b8 "aps/", opt=0xffffc694) at tree-diff.c:384
#15 0x080cc9cb in compare_tree_entry (t1=0xffffc530, t2=0xffffc510, 
    base=0x80faba8 "", baselen=0, opt=0xffffc694) at tree-diff.c:61
#16 0x080ccf93 in diff_tree (t1=0xffffc530, t2=0xffffc510, base=0x80faba8 "", 
    opt=0xffffc694) at tree-diff.c:278
#17 0x080cd371 in diff_tree_sha1 (
    old=0x813c3dc "\231�2274M�\236�\t?�\225\t��", 
    new=0x813c43c "*!�\200\006�\235�?t��:DR", base=0x80faba8 "", 
    opt=0xffffc694) at tree-diff.c:384
#18 0x080d11d0 in log_tree_diff (opt=0xffffc610, commit=0x813c438, 
    log=0xffffc5c0) at log-tree.c:378
#19 0x080d125a in log_tree_commit (opt=0xffffc610, commit=0x813c438)
    at log-tree.c:402
#20 0x0805d26b in print_summary (prefix=0x0, 
    sha1=0xffffd7e0 "*!�\200\006�\235�?t��:DR") at builtin-commit.c:709
#21 0x0805dae7 in cmd_commit (argc=0, argv=0xffffd9e8, prefix=0x0)
    at builtin-commit.c:898
#22 0x0804b44b in run_command (p=0x80fd468, argc=3, argv=0xffffd9e8)
    at git.c:257
#23 0x0804b5f9 in handle_internal_command (argc=3, argv=0xffffd9e8)
    at git.c:383
#24 0x0804b75c in main (argc=3, argv=0xffffd9e8) at git.c:447
(gdb) c
Continuing.
fstat failed: Bad file descriptor
fatal: Out of memory? mmap failed: Bad file descriptor
Created commit 
Program exited with code 0200.
(gdb)

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

[Junio C Hamano]

Brandon Casey <casey@nrlssc.navy.mil> writes:

> Junio C Hamano wrote:
>> Brandon Casey <casey@nrlssc.navy.mil> writes:
>> 
>>> It's reproduceable for me by amending the commit.
>> 
>> Reliably reproducible?  Can you build with "-O0 -g" and run
>> "commit --amend" under gdb?
>> 
>
> make NO_CURL=1 CFLAGS='-O0 -g'
> Done.
>
> I also moved xmmap into commit.c, and turned the inlined definition
> in git-compat-util.h into a declaration.
>
>
> I set a breakpoint on xmmap(). This is the backtrace on the last entry
> into xmmap() before it die()'ed.
>
> The fstat message at the end is from a call to fstat that I added to
> print out the file size (to compare with mmap length). As you can
> see the fstat also fails with the 'Bad file descriptor' message.
>
> #0  xmmap (start=0x0, length=996168, prot=1, flags=2, fd=6, offset=0)
>     at commit.c:680
> #1  0x080acf30 in use_pack (p=0x8150650, w_cursor=0xffffc0ac, offset=94828, 
>     left=0xffffc06c) at sha1_file.c:748

That's the pack window shuffling code in use_pack().  I presume
your additional fstat is inside xmmap(), so if p->pack_fd is
already closed when this xmmap() call is made, that would
explain the symptom.

	while (packed_git_limit < pack_mapped
		&& unuse_one_window(p, p->pack_fd))
		; /* nothing */
	win->base = xmmap(NULL, win->len,
		PROT_READ, MAP_PRIVATE,
		p->pack_fd, win->offset);

I wonder what's the best way to find out who closes file
descriptor #6 without clearing p->pack_fd that still holds #6?
My reading of unuse_one_window() is that it tried to avoid
closing p->pack_fd, so it may already have been closed when we
get to this codepath.

Shawn, does this ring a bell?

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

[Alex Riesen]

Brandon Casey, Fri, Jan 11, 2008 23:47:17 +0100:
> 
> It's reproduceable for me by amending the commit.
> 
> Any suggestions?

strace -o log -f git commit -C HEAD --amend

and post the "log" here (assuming it failed)

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

[Brandon Casey]

On Sat, 12 Jan 2008, Alex Riesen wrote:

> Brandon Casey, Fri, Jan 11, 2008 23:47:17 +0100:
>>
>> It's reproduceable for me by amending the commit.
>>
>> Any suggestions?
>
> strace -o log -f git commit -C HEAD --amend
>
> and post the "log" here (assuming it failed)

It does not fail when -C HEAD is used.

Specifically I did...

   Modify Makefile.in (random file).
   git commit -a --amend
   <:wq when vi opens, i.e. save without making changes>
   <failure>

   Modify Makefile.in
   git commit -a -C HEAD --amend
   <successful completion>

   Modify Makefile.in
   git commit -a --amend
   <:wq save without making change>
   <failure>

-brandon

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

[Brandon Casey]

Brandon Casey wrote:
> I got this message from git-commit:
> 
> $ git commit -a
> <edit message, :wq>
> fatal: Out of memory? mmap failed: Bad file descriptor
> Create commit <my_prompt_string>

I ran git-bisect and the result is below. Doesn't look like
much help though.

To reiterate, I only have problems with the builtin-commit,
i.e. 1.5.4.*, the 1.5.3.* series works correctly. Of course
if this is a memory corruption issue, then it could just be
that the pattern of memory accesses in 1.5.3 does not tweak
the problem.

The other possibly useful info is that running
'git commit -a -C HEAD --amend' does not cause the error.



1596456309315befb3fd0a985d50a70ed09493e4 is first bad commit
commit 1596456309315befb3fd0a985d50a70ed09493e4
Author: Junio C Hamano <gitster@pobox.com>
Date:   Sun Dec 16 15:03:58 2007 -0800

    builtin-commit: fix summary output.

    Because print_summary() forgot to call diff_setup_done() after futzing with
    diff output options, it failed to activate recursive diff, which resulted in
    an incorrect summary.

    Signed-off-by: Junio C Hamano <gitster@pobox.com>

:100644 100644 518ebe0347e631c72f4e2a83b948259ee20fd213 61770ef456ca7f5f8342796e66f7ebfd3e1e7f73 M      builtin-commit.c


I've spent a number of hours trying to debug this. If there are any other ideas for
debugging, let me know. I'll keep the repo for a while.

-brandon

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

[Linus Torvalds]

On Mon, 14 Jan 2008, Brandon Casey wrote:
> 
> To reiterate, I only have problems with the builtin-commit,
> i.e. 1.5.4.*, the 1.5.3.* series works correctly. Of course
> if this is a memory corruption issue, then it could just be
> that the pattern of memory accesses in 1.5.3 does not tweak
> the problem.

Can you do an strace of the failure case and put it up on some public 
place (it's likely going to be too big to send as email)?

			Linus

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

[Brandon Casey]

Linus Torvalds wrote:
> 
> On Mon, 14 Jan 2008, Brandon Casey wrote:
>> To reiterate, I only have problems with the builtin-commit,
>> i.e. 1.5.4.*, the 1.5.3.* series works correctly. Of course
>> if this is a memory corruption issue, then it could just be
>> that the pattern of memory accesses in 1.5.3 does not tweak
>> the problem.
> 
> Can you do an strace of the failure case and put it up on some public 
> place (it's likely going to be too big to send as email)?

I did the strace. Below is the last screenful of lines.

Do you have a suggestion for a public place to upload? I do not have
one of my own, and I've never used any of the 'free' services. The
strace log is about 8.5MB, compressed to about 500K.

$ git --version
git version 1.5.4.rc3.11.g4e67

Not that it's important, but looks like the file descriptor that
is closed too soon is 3. I got 6 when running under gdb. This is
also using the latest version of git. The results are the same
with either version (including the fd#) so I just used this one.

Junio C Hamano wrote:
> What platform is this on?

$ cat /etc/redhat-release
CentOS release 4.5 (Final)

i686

> Does it reliably reproduce for any commit in the repository, or
> reliably reproduce for one particular commit, or sometimes
> reprooduce for one particular commit?

Reliably for one particular commit.

Additional commits on top of this commit complete successfully.

If this commit is amended without error by amending with '-C HEAD'
or by using a 1.5.3 version, then additional amends or commits
will not produce the error.

-brandon


16170 mmap2(NULL, 417, PROT_READ, MAP_PRIVATE, 3, 0) = 0xb1699000
16170 close(3)                          = 0
16170 munmap(0xb1699000, 417)           = 0
16170 stat64("/home/casey/auto_v3.5/src_temp2/.git/objects/67/981a61208756cf4973
7ec065e7bc0d7ff1a89d", {st_mode=S_IFREG|0444, st_size=417, ...}) = 0
16170 open("/home/casey/auto_v3.5/src_temp2/.git/objects/67/981a61208756cf49737e
c065e7bc0d7ff1a89d", O_RDONLY|O_LARGEFILE) = 3
16170 mmap2(NULL, 417, PROT_READ, MAP_PRIVATE, 3, 0) = 0xb1699000
16170 close(3)                          = 0
16170 munmap(0xb1699000, 417)           = 0
16170 stat64("/home/casey/auto_v3.5/src_temp2/.git/objects/4e/7c1178482f4b3c52e8
afce15db3bc8419530d2", {st_mode=S_IFREG|0444, st_size=417, ...}) = 0
16170 open("/home/casey/auto_v3.5/src_temp2/.git/objects/4e/7c1178482f4b3c52e8af
ce15db3bc8419530d2", O_RDONLY|O_LARGEFILE) = 3
16170 mmap2(NULL, 417, PROT_READ, MAP_PRIVATE, 3, 0) = 0xb1699000
16170 close(3)                          = 0
16170 munmap(0xb1699000, 417)           = 0
16170 stat64("/home/casey/auto_v3.5/src_temp2/.git/objects/02/3777b3c0e5f7697deb
2ce738c6b38b1ec2b17c", {st_mode=S_IFREG|0444, st_size=417, ...}) = 0
16170 open("/home/casey/auto_v3.5/src_temp2/.git/objects/02/3777b3c0e5f7697deb2c
e738c6b38b1ec2b17c", O_RDONLY|O_LARGEFILE) = 3
16170 mmap2(NULL, 417, PROT_READ, MAP_PRIVATE, 3, 0) = 0xb1699000
16170 close(3)                          = 0
16170 munmap(0xb1699000, 417)           = 0
16170 mmap2(NULL, 996168, PROT_READ, MAP_PRIVATE, 3, 0) = -1 EBADF (Bad file des
criptor)
16170 munmap(0xb56bd000, 33554432)      = 0
16170 mmap2(NULL, 996168, PROT_READ, MAP_PRIVATE, 3, 0) = -1 EBADF (Bad file des
criptor)
16170 write(2, "fatal: Out of memory? mmap faile"..., 55) = 55
16170 write(1, "Created commit ", 15)   = 15
16170 exit_group(128)                   = ?

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

[Linus Torvalds]

On Tue, 15 Jan 2008, Brandon Casey wrote:
> 
> Do you have a suggestion for a public place to upload? I do not have
> one of my own, and I've never used any of the 'free' services. The
> strace log is about 8.5MB, compressed to about 500K.

Can you just email the compressed one to me as an attachement, I'll put it 
somewhere..

> Not that it's important, but looks like the file descriptor that
> is closed too soon is 3.

Yes and no. There obviously are several "close(3)"s in even that short 
snippet, but they are for a different kind of close - they are for the 
regular loose object open/mmap/close/munmap sequence which has re-used 
that file descriptor.

So the *incorrect* close(3) happened some time much earlier.

The other alternative is, of course, that the 3 itself is just wrong, and 
something corrupted the packfile data structures.

> > Does it reliably reproduce for any commit in the repository, or
> > reliably reproduce for one particular commit, or sometimes
> > reprooduce for one particular commit?
> 
> Reliably for one particular commit.
> 
> Additional commits on top of this commit complete successfully.

It would obviously be interesting to see the base repository and the 
commit you are trying to do - is that possibly publicly available?

		Linus

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

[Brandon Casey]

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.

-brandon

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

[Linus Torvalds]

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.

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

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

[Brandon Casey]

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

Probably not causing the error at hand, but not good.

-brandon


diff --git a/builtin-commit.c b/builtin-commit.c
index 1e55c2e..3b4f4e2 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -256,7 +256,7 @@ static char *prepare_index(int argc, const char **argv, cons
		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();

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

[Kristian Høgsberg]

On Tue, 2008-01-15 at 13:43 -0600, Brandon Casey wrote:
> 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;
 }

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

[Brandon Casey]

Kristian Høgsberg wrote:

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

It does. Thanks.

-brandon

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

[Brandon Casey]

Kristian Høgsberg wrote:

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

I bet they did that so that the return status of close() could be checked
since commit_lock_file() doesn't currently check it.

-brandon

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

[Linus Torvalds]

On Tue, 15 Jan 2008, Brandon Casey wrote:
> 
> 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();
> 	}

Yeah, I think that may be the one that got you. I obviously couldn't 
follow the exact path through the code, I was just looking at the trace of 
system calls and found that one thing that looked like it was your case, 
but it's entirely possible that it was another path of the index lock file 
that causes it.

Your patch seems "ObviouslyCorrect(tm)".

		Linus

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

[Linus Torvalds]

On Tue, 15 Jan 2008, Linus Torvalds wrote:
> 
> Your patch seems "ObviouslyCorrect(tm)".

And Kristian's more extensive patch that finds a few more cases looks 
better yet. Does that fix it for you?

		Linus

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

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

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

While I think the ones that are immediately followed by
commit_locked_index() can drop the close(fd) safely, I am not
sure about Kristian's changes to the other ones that we
currently close(fd) but do not commit nor rollback immediately.
These indices are now shown to the hook with open fd to it if
you choose not to close them.  Is that okay for Windows guys?  I
somehow had an impression that the other process may have
trouble accessing a file that is still open elsewhere for
writing.

So I think the approach along the lines of your "hack" to close
and tell lockfile API not to double-close is more appropriate.
We would perhaps want "close_lock_file(struct lock_file *)" that
calls close(lk->fd) and does lk->fd = -1 without rename/unlink,
and replace these close() with that.

I am sick today, feeling feverish, and not thinking straight,
so I may be talking total nonsense...

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
>> 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.
>>
>> 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*.
>
> While I think the ones that are immediately followed by
> commit_locked_index() can drop the close(fd) safely, I am not
> sure about Kristian's changes to the other ones that we
> currently close(fd) but do not commit nor rollback immediately.
> These indices are now shown to the hook with open fd to it if
> you choose not to close them.  Is that okay for Windows guys?  I
> somehow had an impression that the other process may have
> trouble accessing a file that is still open elsewhere for
> writing.
>
> So I think the approach along the lines of your "hack" to close
> and tell lockfile API not to double-close is more appropriate.
> We would perhaps want "close_lock_file(struct lock_file *)" that
> calls close(lk->fd) and does lk->fd = -1 without rename/unlink,
> and replace these close() with that.
>
> I am sick today, feeling feverish, and not thinking straight,
> so I may be talking total nonsense...

I'll aplly and push out Kristian's one that apparently got
Tested-by from Brandon for tonight.

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

[Brandon Casey]

On Tue, 15 Jan 2008, Junio C Hamano wrote:

>> Junio C Hamano <gitster@pobox.com> writes:
>>
>> While I think the ones that are immediately followed by
>> commit_locked_index() can drop the close(fd) safely, I am not
>> sure about Kristian's changes to the other ones that we
>> currently close(fd) but do not commit nor rollback immediately.
>> These indices are now shown to the hook with open fd to it if
>> you choose not to close them.  Is that okay for Windows guys?  I
>> somehow had an impression that the other process may have
>> trouble accessing a file that is still open elsewhere for
>> writing.
>>
>> So I think the approach along the lines of your "hack" to close
>> and tell lockfile API not to double-close is more appropriate.
>> We would perhaps want "close_lock_file(struct lock_file *)" that
>> calls close(lk->fd) and does lk->fd = -1 without rename/unlink,
>> and replace these close() with that.
>>
>> I am sick today, feeling feverish, and not thinking straight,
>> so I may be talking total nonsense...

> I'll aplly and push out Kristian's one that apparently got
> Tested-by from Brandon for tonight.

I've got a followup patch coming that will remove the rest of
the redundant close()'s and I'll look into your suggestion for
close_lock_file() above.

Currently everything passes the test suite, I just need to do
some manual testing.

-brandon

[PATCH 1/2] Document lockfile API

[Junio C Hamano]

We have nice set of placeholders, but nobody stepped in to fill
the gap in the API documentation, so I am doing it myself.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/technical/api-lockfile.txt |   67 ++++++++++++++++++++++++++---
 1 files changed, 60 insertions(+), 7 deletions(-)

diff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt
index 73ac102..5b1553e 100644
--- a/Documentation/technical/api-lockfile.txt
+++ b/Documentation/technical/api-lockfile.txt
@@ -1,12 +1,65 @@
 lockfile API
 ============
 
-Talk about <lockfile.c>, things like:
+The lockfile API serves two purposes:
 
-* lockfile lifetime -- atexit(3) looks at them, do not put them on the
-  stack;
-* hold_lock_file_for_update()
-* commit_lock_file()
-* rollback_rock_file()
+* Mutual exclusion.  When we write out a new index file, first
+  we create a new file `$GIT_DIR/index.lock`, write the new
+  contents into it, and rename it to the final destination
+  `$GIT_DIR/index`.  We try to create the `$GIT_DIR/index.lock`
+  file with O_EXCL so that we can notice and fail when somebody
+  else is already trying to update the index file.
 
-(JC, Dscho, Shawn)
+* Automatic cruft removal.  After we create the "lock" file, we
+  may decide to `die()`, and we would want to make sure that we
+  remove the file that has not been committed to its final
+  destination.  This is done by remembering the lockfiles we
+  created in a linked list and cleaning them up from an
+  `atexit(3)` handler.  Outstanding lockfiles are also removed
+  when the program dies on a signal.
+
+
+The functions
+-------------
+
+hold_lock_file_for_update::
+
+	Take a pointer to `struct lock_file`, the filename of
+	the final destination (e.g. `$GIT_DIR/index`) and a flag
+	`die_on_error`.  Attempt to create a lockfile for the
+	destination and return the file descriptor for writing
+	to the file.  If `die_on_error` flag is true, it dies if
+	a lock is already taken for the file; otherwise it
+	returns a negative integer to the caller on failure.
+
+commit_lock_file::
+
+	Take a pointer to the `struct lock_file` initialized
+	with an earlier call to `hold_lock_file_for_update()`,
+	close the file descriptor and rename the lockfile to its
+	final destination.
+
+rollback_lock_file::
+
+	Take a pointer to the `struct lock_file` initialized
+	with an earlier call to `hold_lock_file_for_update()`,
+	close the file descriptor and remove the lockfile.
+
+Because the structure is used in an `atexit(3)` handler, its
+storage has to stay throughout the life of the program.  It
+cannot be an auto variable allocated on the stack.
+
+Call `commit_lock_file()` or `rollback_lock_file()` when you are
+done writing to the file descriptor.  If you do not call either
+and simply `exit(3)` from the program, an `atexit(3)` handler
+will close and remove the lockfile.
+
+You should not close the file descriptor you obtained from
+`hold_lock_file_for_update` function yourself.  The `struct
+lock_file` structure still remembers that the file descriptor
+needs to be closed, and a later call to `commit_lock_file()` or
+`rollback_lock_file()` will result in duplicate calls to
+`close(2)`.  Worse yet, if you `close(2)`, open another file
+descriptor for completely different purpose, and then call
+`commit_lock_file()` or `rollback_lock_file()`, they may close
+that unrelated file descriptor.
-- 
1.5.4.rc3.14.g44397

[PATCH 2/2] close_lock_file(): new function in the lockfile API

[Junio C Hamano]

The lockfile API is a handy way to obtain a file that is cleaned
up if you die().  But sometimes you would need this sequence to
work:

 1. hold_lock_file_for_update() to get a file descriptor for
    writing;

 2. write the contents out, without being able to decide if the
    results should be committed or rolled back;

 3. do something else that makes the decision --- and this
    "something else" needs the lockfile not to have an open file
    descriptor for writing (e.g. Windows do not want a open file
    to be renamed);

 4. call commit_lock_file() or rollback_lock_file() as
    appropriately.

This adds close_lock_file() you can call between step 2 and 3 in
the above sequence.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/technical/api-lockfile.txt |   11 +++++++++--
 cache.h                                  |    2 +-
 lockfile.c                               |    6 ++++++
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt
index 5b1553e..def5f2a 100644
--- a/Documentation/technical/api-lockfile.txt
+++ b/Documentation/technical/api-lockfile.txt
@@ -45,6 +45,11 @@ rollback_lock_file::
 	with an earlier call to `hold_lock_file_for_update()`,
 	close the file descriptor and remove the lockfile.
 
+close_lock_file::
+	Take a pointer to the `struct lock_file` initialized
+	with an earlier call to `hold_lock_file_for_update()`,
+	and close the file descriptor.
+
 Because the structure is used in an `atexit(3)` handler, its
 storage has to stay throughout the life of the program.  It
 cannot be an auto variable allocated on the stack.
@@ -54,8 +59,10 @@ done writing to the file descriptor.  If you do not call either
 and simply `exit(3)` from the program, an `atexit(3)` handler
 will close and remove the lockfile.
 
-You should not close the file descriptor you obtained from
-`hold_lock_file_for_update` function yourself.  The `struct
+If you need to close the file descriptor you obtained from
+`hold_lock_file_for_update` function yourself, do so by calling
+`close_lock_file()`.  You should never call `close(2)` yourself!
+Otherwise the `struct
 lock_file` structure still remembers that the file descriptor
 needs to be closed, and a later call to `commit_lock_file()` or
 `rollback_lock_file()` will result in duplicate calls to
diff --git a/cache.h b/cache.h
index 39331c2..5033b34 100644
--- a/cache.h
+++ b/cache.h
@@ -308,7 +308,7 @@ extern int commit_lock_file(struct lock_file *);
 extern int hold_locked_index(struct lock_file *, int);
 extern int commit_locked_index(struct lock_file *);
 extern void set_alternate_index_output(const char *);
-
+extern void close_lock_file(struct lock_file *);
 extern void rollback_lock_file(struct lock_file *);
 extern int delete_ref(const char *, const unsigned char *sha1);
 
diff --git a/lockfile.c b/lockfile.c
index f45d3ed..e57d850 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -201,3 +201,9 @@ void rollback_lock_file(struct lock_file *lk)
 	}
 	lk->filename[0] = 0;
 }
+
+void close_lock_file(struct lock_file *lk)
+{
+	close(lk->fd);
+	lk->fd = -1;
+}
-- 
1.5.4.rc3.14.g44397

Re: [PATCH 2/2] close_lock_file(): new function in the lockfile API

[Linus Torvalds]

On Wed, 16 Jan 2008, Junio C Hamano wrote:
> +
> +void close_lock_file(struct lock_file *lk)
> +{
> +	close(lk->fd);
> +	lk->fd = -1;
> +}

Since one of the main purposes of closing would be the error testing of 
writes that haven't made it out yet on filesystems like NFS that do 
open-close cache serialization, I'd suggest doing this as

	int close_lock_file(struct lock_file *lk)
	{
		int fd = lk->fd;
		lk->df = -1;
		return close(fd);
	} 

to give the return code.

		Linus

Re: [PATCH 2/2] close_lock_file(): new function in the lockfile API

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Wed, 16 Jan 2008, Junio C Hamano wrote:
>> +
>> +void close_lock_file(struct lock_file *lk)
>> +{
>> +	close(lk->fd);
>> +	lk->fd = -1;
>> +}
>
> Since one of the main purposes of closing would be the error testing of 
> writes that haven't made it out yet on filesystems like NFS that do 
> open-close cache serialization, I'd suggest doing this as
>
> 	int close_lock_file(struct lock_file *lk)
> 	{
> 		int fd = lk->fd;
> 		lk->df = -1;
> 		return close(fd);
> 	} 
>
> to give the return code.

Yup!  You are as always right.

Re: [PATCH 2/2] close_lock_file(): new function in the lockfile API

[Brandon Casey]

On Wed, 16 Jan 2008, Junio C Hamano wrote:

> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
>> On Wed, 16 Jan 2008, Junio C Hamano wrote:
>>> +
>>> +void close_lock_file(struct lock_file *lk)
>>> +{
>>> +	close(lk->fd);
>>> +	lk->fd = -1;
>>> +}
>>
>> Since one of the main purposes of closing would be the error testing of
>> writes that haven't made it out yet on filesystems like NFS that do
>> open-close cache serialization, I'd suggest doing this as
>>
>> 	int close_lock_file(struct lock_file *lk)
>> 	{
>> 		int fd = lk->fd;
>> 		lk->df = -1;
>> 		return close(fd);
>> 	}
>>
>> to give the return code.
>
> Yup!  You are as always right.

My patch does this, though I understand it may take some time to review.

I left the lk->fd unmodified when close() failed in case the caller
would like to include it in an error message.

-brandon

Re: [PATCH 2/2] close_lock_file(): new function in the lockfile API

[Junio C Hamano]

Brandon Casey <casey@nrlssc.navy.mil> writes:

> My patch does this, though I understand it may take some time to review.

This is what I have right now, squashed your change into [2/2] 
I sent earlier, along with a couple of further fixups.

Improvement since my v1 are:

 - close_lock_file() returns int, which is the return value from
   close(2);

 - remove_lock_file() avoids calling close(2) if
   close_lock_file() was called earlier on the lockfile;

 - commit_lock_file() does the same, and notices failure
   returned from close_lock_file().  In addition, it unlinks the
   lockfile and clears lk->filename upon failure;

 - commit_locked_index() does the same, and notices failure
   returned from close_lock_file() in alternate_index_output
   codepath.  It unlinks the lockfile and clears lk->filename
   upon failure;

 - The codepath in commit_locked_index() for writing to
   alternate_index_output clears the alternate_index_output
   variable when it is done.

Most of the above are thanks to the changes to lockfile.c in your
version and Linus's suggestion.

I am planning to take your patch (only the parts that fix the
callers, because the changes to lockfile.c are all included
here) on top of this one.

-- >8 --
close_lock_file(): new function in the lockfile API

The lockfile API is a handy way to obtain a file that is cleaned
up if you die().  But sometimes you would need this sequence to
work:

 1. hold_lock_file_for_update() to get a file descriptor for
    writing;

 2. write the contents out, without being able to decide if the
    results should be committed or rolled back;

 3. do something else that makes the decision --- and this
    "something else" needs the lockfile not to have an open file
    descriptor for writing (e.g. Windows do not want a open file
    to be renamed);

 4. call commit_lock_file() or rollback_lock_file() as
    appropriately.

This adds close_lock_file() you can call between step 2 and 3 in
the above sequence.

[jc: updated with Brandon's stricter error checking on return values
 from close() and a suggestion by Linus.]

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/technical/api-lockfile.txt |   15 +++++++++++--
 cache.h                                  |    2 +-
 lockfile.c                               |   32 ++++++++++++++++++++++++-----
 3 files changed, 39 insertions(+), 10 deletions(-)

diff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt
index 5b1553e..dd89404 100644
--- a/Documentation/technical/api-lockfile.txt
+++ b/Documentation/technical/api-lockfile.txt
@@ -37,7 +37,8 @@ commit_lock_file::
 	Take a pointer to the `struct lock_file` initialized
 	with an earlier call to `hold_lock_file_for_update()`,
 	close the file descriptor and rename the lockfile to its
-	final destination.
+	final destination.  Returns 0 upon success, a negative
+	value on failure to close(2) or rename(2).
 
 rollback_lock_file::
 
@@ -45,6 +46,12 @@ rollback_lock_file::
 	with an earlier call to `hold_lock_file_for_update()`,
 	close the file descriptor and remove the lockfile.
 
+close_lock_file::
+	Take a pointer to the `struct lock_file` initialized
+	with an earlier call to `hold_lock_file_for_update()`,
+	and close the file descriptor.  Returns 0 upon success,
+	a negative value on failure to close(2).
+
 Because the structure is used in an `atexit(3)` handler, its
 storage has to stay throughout the life of the program.  It
 cannot be an auto variable allocated on the stack.
@@ -54,8 +61,10 @@ done writing to the file descriptor.  If you do not call either
 and simply `exit(3)` from the program, an `atexit(3)` handler
 will close and remove the lockfile.
 
-You should not close the file descriptor you obtained from
-`hold_lock_file_for_update` function yourself.  The `struct
+If you need to close the file descriptor you obtained from
+`hold_lock_file_for_update` function yourself, do so by calling
+`close_lock_file()`.  You should never call `close(2)` yourself!
+Otherwise the `struct
 lock_file` structure still remembers that the file descriptor
 needs to be closed, and a later call to `commit_lock_file()` or
 `rollback_lock_file()` will result in duplicate calls to
diff --git a/cache.h b/cache.h
index 39331c2..24735bd 100644
--- a/cache.h
+++ b/cache.h
@@ -308,7 +308,7 @@ extern int commit_lock_file(struct lock_file *);
 extern int hold_locked_index(struct lock_file *, int);
 extern int commit_locked_index(struct lock_file *);
 extern void set_alternate_index_output(const char *);
-
+extern int close_lock_file(struct lock_file *);
 extern void rollback_lock_file(struct lock_file *);
 extern int delete_ref(const char *, const unsigned char *sha1);
 
diff --git a/lockfile.c b/lockfile.c
index f45d3ed..fcf9285 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -13,7 +13,8 @@ static void remove_lock_file(void)
 	while (lock_file_list) {
 		if (lock_file_list->owner == me &&
 		    lock_file_list->filename[0]) {
-			close(lock_file_list->fd);
+			if (lock_file_list->fd >= 0)
+				close(lock_file_list->fd);
 			unlink(lock_file_list->filename);
 		}
 		lock_file_list = lock_file_list->next;
@@ -159,11 +160,23 @@ int hold_lock_file_for_update(struct lock_file *lk, const char *path, int die_on
 	return fd;
 }
 
+int close_lock_file(struct lock_file *lk)
+{
+	int fd = lk->fd;
+	lk->fd = -1;
+	return close(fd);
+}
+
 int commit_lock_file(struct lock_file *lk)
 {
 	char result_file[PATH_MAX];
 	int i;
-	close(lk->fd);
+
+	if (lk->fd >= 0 && close_lock_file(lk)) {
+		unlink(lk->filename);
+		lk->filename[0] = 0;
+		return -1;
+	}
 	strcpy(result_file, lk->filename);
 	i = strlen(result_file) - 5; /* .lock */
 	result_file[i] = 0;
@@ -185,9 +198,15 @@ void set_alternate_index_output(const char *name)
 int commit_locked_index(struct lock_file *lk)
 {
 	if (alternate_index_output) {
-		int result = rename(lk->filename, alternate_index_output);
-		lk->filename[0] = 0;
-		return result;
+		const char *newname = alternate_index_output;
+		alternate_index_output = NULL;
+
+		if (lk->fd >= 0 && close_lock_file(lk)) {
+			unlink(lk->filename);
+			lk->filename[0] = 0;
+			return -1;
+		}
+		return rename(lk->filename, newname);
 	}
 	else
 		return commit_lock_file(lk);
@@ -196,7 +215,8 @@ int commit_locked_index(struct lock_file *lk)
 void rollback_lock_file(struct lock_file *lk)
 {
 	if (lk->filename[0]) {
-		close(lk->fd);
+		if (lk->fd >= 0)
+			close(lk->fd);
 		unlink(lk->filename);
 	}
 	lk->filename[0] = 0;
-- 
1.5.4.rc3.14.g44397

Re: [PATCH 2/2] close_lock_file(): new function in the lockfile API

[Brandon Casey]

Junio C Hamano wrote:
> Brandon Casey <casey@nrlssc.navy.mil> writes:
> 
>> My patch does this, though I understand it may take some time to review.
> 
> This is what I have right now, squashed your change into [2/2] 
> I sent earlier, along with a couple of further fixups.
> 

Mainly, I prefer to not modify the data structures when a failure occurs.

Generally when commit_lock_file fails, the caller die()'s and then
remove_lock_file will delete the temporary file. If the caller were to
not call die, I think it should call rollback_lock_file() similarly to
how unlock_ref() is called everywhere in refs.c.

So I think the semantics should be:

	lock_fd = hold_lock_file(&lock);
	<do_something>
	if (close_lock_file(&lock)) {
		rollback_lock_file(&lock);
		return error;
	}
	if (commit_lock_file(&lock)) {
		rollback_lock_file(&lock);
		return error;
	}


> diff --git a/lockfile.c b/lockfile.c
> index f45d3ed..fcf9285 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -13,7 +13,8 @@ static void remove_lock_file(void)
>  	while (lock_file_list) {
>  		if (lock_file_list->owner == me &&
>  		    lock_file_list->filename[0]) {
> -			close(lock_file_list->fd);
> +			if (lock_file_list->fd >= 0)
> +				close(lock_file_list->fd);
>  			unlink(lock_file_list->filename);
>  		}
>  		lock_file_list = lock_file_list->next;
> @@ -159,11 +160,23 @@ int hold_lock_file_for_update(struct lock_file *lk, const char *path, int die_on
>  	return fd;
>  }
>  
> +int close_lock_file(struct lock_file *lk)
> +{
> +	int fd = lk->fd;
> +	lk->fd = -1;
> +	return close(fd);
> +}

minor nit on this that I mentioned in another email.

> +
>  int commit_lock_file(struct lock_file *lk)
>  {
>  	char result_file[PATH_MAX];
>  	int i;
> -	close(lk->fd);
> +
> +	if (lk->fd >= 0 && close_lock_file(lk)) {
> +		unlink(lk->filename);
> +		lk->filename[0] = 0;
> +		return -1;
> +	}


I would rather have the caller call rollback_lock_file, or
fall back to remove_lock_file rather than do this unlinking
and modifying lk->filename here in commit_lock_file.


>  	strcpy(result_file, lk->filename);
>  	i = strlen(result_file) - 5; /* .lock */
>  	result_file[i] = 0;
> @@ -185,9 +198,15 @@ void set_alternate_index_output(const char *name)
>  int commit_locked_index(struct lock_file *lk)
>  {
>  	if (alternate_index_output) {
> -		int result = rename(lk->filename, alternate_index_output);
> -		lk->filename[0] = 0;
> -		return result;
> +		const char *newname = alternate_index_output;
> +		alternate_index_output = NULL;
> +
> +		if (lk->fd >= 0 && close_lock_file(lk)) {
> +			unlink(lk->filename);
> +			lk->filename[0] = 0;
> +			return -1;
> +		}

ditto here.

> +		return rename(lk->filename, newname);


If rename succeeds, we'll try to do an unnecessary unlink atexit.


Having said those things, I will defer to your more experienced judgment.

-brandon

Re: [PATCH 2/2] close_lock_file(): new function in the lockfile API

[Junio C Hamano]

Brandon Casey <casey@nrlssc.navy.mil> writes:

> Mainly, I prefer to not modify the data structures when a failure occurs.

Ok.  Is the rest of your patch that fixes callers Ok with that
semantics?  If so, I'd agree that is probably cleaner.  I'll
scrap the one we are discussing, resurrecting only the api
documentation part, and replace it with the lockfile.c changes
from your patch, along with the fixes to callers.
 

Re: [PATCH 2/2] close_lock_file(): new function in the lockfile API

[Brandon Casey]

Junio C Hamano wrote:
> Brandon Casey <casey@nrlssc.navy.mil> writes:
> 
>> Mainly, I prefer to not modify the data structures when a failure occurs.
> 
> Ok.  Is the rest of your patch that fixes callers Ok with that
> semantics?

yes.

>  If so, I'd agree that is probably cleaner.  I'll
> scrap the one we are discussing, resurrecting only the api
> documentation part, and replace it with the lockfile.c changes
> from your patch, along with the fixes to callers.

Most of that patch is straight forward, just removing close().

I think you should consider how to handle fdopen on the lock
descriptor and the fact that start_command closes the lock
file descriptor in create_bundle().

After we fdopen, we should always fclose() and never close().
This isn't enforced.

I merely assigned the file descriptor to -1 when it was safe
(i.e. after fclose), and added a comment. We could add another
function which did this automatically, but maybe that is too
much effort, especially in the bundle case.

-brandon

Re: [PATCH 2/2] close_lock_file(): new function in the lockfile API

[Brandon Casey]

Brandon Casey wrote:
> I think you should consider how to handle fdopen on the lock
> descriptor

This happens in
builtin-pack-refs.c:pack_refs
fast-import.c:dump_marks

-brandon

Re: [PATCH 2/2] close_lock_file(): new function in the lockfile API

[Junio C Hamano]

Brandon Casey <casey@nrlssc.navy.mil> writes:

> My patch does this, though I understand it may take some time to review.
>
> I left the lk->fd unmodified when close() failed in case the caller
> would like to include it in an error message.

But that would bring us back to the same double-close issue,
wouldn't it?

	if (close_lock_file(lock))
		die("Oops, failed to close fd %d", lock->fd);

is not enough.  You need to do:

	if (close_lock_file(lock)) {
        	int fd = lock->fd;
                lock->fd = -1;
		die("Oops, failed to close fd %d", fd);
	}

to avoid atexit handler closing the lock->fd.

Worse yet, a careless caller may do:

	close_lock_file(lock);

	... do something that opens a new fd, perhaps for
        ... mmaping a packfile in

	rollback_lock_file(lock);

	... Oops, we cannot mmap the packfile.

Re: [PATCH 2/2] close_lock_file(): new function in the lockfile API

[Brandon Casey]

On Wed, 16 Jan 2008, Junio C Hamano wrote:

> Brandon Casey <casey@nrlssc.navy.mil> writes:
>
>> My patch does this, though I understand it may take some time to review.
>>
>> I left the lk->fd unmodified when close() failed in case the caller
>> would like to include it in an error message.
>
> But that would bring us back to the same double-close issue,
> wouldn't it?

Yes it would. Although I knew it would happen, I was disregarding
_that_ double close case for some reason.

You're right, your's and Linus's version is better.

-brandon

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

[Johannes Sixt]

Junio C Hamano schrieb:
> While I think the ones that are immediately followed by
> commit_locked_index() can drop the close(fd) safely, I am not
> sure about Kristian's changes to the other ones that we
> currently close(fd) but do not commit nor rollback immediately.
> These indices are now shown to the hook with open fd to it if
> you choose not to close them.  Is that okay for Windows guys?  I
> somehow had an impression that the other process may have
> trouble accessing a file that is still open elsewhere for
> writing.

The trouble is that on Windows open files cannot be deleted or renamed.
Hence, if an index file remains open, the hooks won't be able to modify
them (because of the create-new-file-then-rename-over-old tactics).

> So I think the approach along the lines of your "hack" to close
> and tell lockfile API not to double-close is more appropriate.
> We would perhaps want "close_lock_file(struct lock_file *)" that
> calls close(lk->fd) and does lk->fd = -1 without rename/unlink,
> and replace these close() with that.

Yes!

-- Hannes

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

[Junio C Hamano]

Brandon Casey <casey@nrlssc.navy.mil> writes:

> Brandon Casey wrote:
>> I got this message from git-commit:
>> 
>> $ git commit -a
>> <edit message, :wq>
>> fatal: Out of memory? mmap failed: Bad file descriptor
>> Create commit <my_prompt_string>

I think from your earlier reports we already know that the issue
is that somebody is closing a file descriptor we opened for a
packfile and makes the code to shuffle the window that is used
to access the packfile unhappy because it uses the fd to mmap.

> I ran git-bisect and the result is below. Doesn't look like
> much help though.

That change alone does look innocuous, but indeed is around the
place the code finds that the necessary fd is already closed.

> The other possibly useful info is that running
> 'git commit -a -C HEAD --amend' does not cause the error.

A huge difference between "-C HEAD" and a commit with an editor
is that the former bypasses the git-status code to fill the
commit message template.  In addition to that, the latter spawns
a new process.

It could be the editor codepath may be closing the fd when it
shouldn't, or some atexit() thing is triggering incorrectly.
IIRC the fd incorrectly closed was #6, so it is not likely that
process spawning code that may shuffle low fds is the culprit.

As Linus said already (and Alex suggested earlier in the nearby
thread), strace output might be a good place to help digging
this issue further, instead of us idly speculating.

What platform is this on?

Does it reliably reproduce for any commit in the repository, or
reliably reproduce for one particular commit, or sometimes
reprooduce for one particular commit?