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
From: Charles Bailey <hidden> Date: 2016-06-15 22:44:04
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.
From: Marco Costalba <hidden> Date: 2016-06-15 22:44:04
On Jan 11, 2008 11:11 PM, Brandon Casey [off-list ref] 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.
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
From: Jeff King <hidden> Date: 2016-06-15 22:44:04
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
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
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 [off-list ref]
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 [off-list ref]
: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
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
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:
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) = ?
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.
quoted
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
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(-)
@@ -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;returnfalse_lock.filename;}
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
From: Kristian Høgsberg <hidden> Date: 2016-06-15 22:44:05
On Tue, 2008-01-15 at 13:43 -0600, Brandon Casey wrote:
Linus Torvalds wrote:
quoted
On Tue, 15 Jan 2008, Brandon Casey wrote:
quoted
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.
Good catch, but that wasn't it. Still getting the same error.
quoted
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
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
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.
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
From: Junio C Hamano <hidden> Date: 2016-06-15 22:44:05
Linus Torvalds [off-list ref] writes:
On Tue, 15 Jan 2008, Brandon Casey wrote:
quoted
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*.
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...
From: Johannes Sixt <hidden> Date: 2016-06-15 22:44:05
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.