From: Junio C Hamano <gitster@pobox.com>
To: git@vger.kernel.org
Cc: Jiri Olsa <olsajiri@gmail.com>,
Johannes Schindelin <Johannes.Schindelin@gmx.de>,
Linus Torvalds <torvalds@linux-foundation.org>,
Daniel Barkalow <barkalow@iabervon.org>
Subject: [PATCH] read-tree A B C: do not create a bogus index and do not segfault
Date: Thu, 12 Mar 2009 00:29:13 -0700 [thread overview]
Message-ID: <7v3adjjj1y.fsf_-_@gitster.siamese.dyndns.org> (raw)
In-Reply-To: <7vfxhjjkcm.fsf@gitster.siamese.dyndns.org> (Junio C. Hamano's message of "Thu, 12 Mar 2009 00:01:13 -0700")
"git read-tree A B C..." without the "-m" (merge) option is a way to read
these trees on top of each other to get an overlay of them.
An ancient commit ee6566e (Rewrite read-tree, 2005-09-05) passed the
ADD_CACHE_SKIP_DFCHECK flag when calling add_index_entry() to add the
paths obtained from these trees to the index, but it is an incorrect use
of the flag. The flag is meant to be used by callers who know the
addition of the entry does not introduce a D/F conflict to the index in
order to avoid the overhead of checking.
This bug resulted in a bogus index that records both "x" and "x/z" as a
blob after reading three trees that have paths ("x"), ("x", "y"), and
("x/z", "y") respectively. 34110cd (Make 'unpack_trees()' have a separate
source and destination index, 2008-03-06) refactored the callsites of
add_index_entry() incorrectly and added more codepaths that use this flag
when it shouldn't be used.
Also, 0190457 (Move 'unpack_trees()' over to 'traverse_trees()' interface,
2008-03-05) introduced a bug to call add_index_entry() for the tree that
does not have the path in it, passing NULL as a cache entry. This caused
reading multiple trees, one of which has path "x" but another doesn't, to
segfault.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* I suspect that we can probably remove SKIP_DFCHECK option by fixing
tree.c::read_tree(); the only caller the big comment at the beginning
of it talks about is overlay_tree_on_cache() in builtin-ls-files.c and
we haven't gained any new callers of the function.
It is a bit sad that a very good looking refactoring and rewriting
patch introduced this kind of regression that has gone unnoticed for a
long time. I managed to point three fingers and they turn out to be
all ancient commits before v1.5.5.
unpack-trees.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index cba0aca..5b0a8c1 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -49,7 +49,7 @@ static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,
memcpy(new, ce, size);
new->next = NULL;
new->ce_flags = (new->ce_flags & ~clear) | set;
- add_index_entry(&o->result, new, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE|ADD_CACHE_SKIP_DFCHECK);
+ add_index_entry(&o->result, new, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);
}
/* Unlink the last component and attempt to remove leading
@@ -283,9 +283,9 @@ static int unpack_nondirectories(int n, unsigned long mask, unsigned long dirmas
if (o->merge)
return call_unpack_fn(src, o);
- n += o->merge;
for (i = 0; i < n; i++)
- add_entry(o, src[i], 0, 0);
+ if (src[i] && src[i] != o->df_conflict_entry)
+ add_entry(o, src[i], 0, 0);
return 0;
}
--
1.6.2.249.g770a0
next prev parent reply other threads:[~2009-03-12 7:31 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2009-03-10 19:34 [BUG] - git-read-tree segfaults Jiri Olsa
2009-03-10 19:50 ` Johannes Schindelin
2009-03-10 20:21 ` Johannes Schindelin
2009-03-11 7:59 ` Jiri Olsa
2009-03-11 12:04 ` Johannes Schindelin
2009-03-12 5:57 ` Junio C Hamano
2009-03-12 7:01 ` Junio C Hamano
2009-03-12 7:29 ` Junio C Hamano [this message]
2009-03-12 14:46 ` [PATCH] read-tree A B C: do not create a bogus index and do not segfault Daniel Barkalow
2009-03-12 16:34 ` Linus Torvalds
2009-03-14 6:41 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7v3adjjj1y.fsf_-_@gitster.siamese.dyndns.org \
--to=gitster@pobox.com \
--cc=Johannes.Schindelin@gmx.de \
--cc=barkalow@iabervon.org \
--cc=git@vger.kernel.org \
--cc=olsajiri@gmail.com \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).