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;
}
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
