* [BUG] git branch --merged $unknown_checksum segfaults
@ 2012-02-27 12:26 Bernhard Reutner-Fischer
2012-02-27 15:11 ` [PATCH] branch: don't assume the merge filter ref exists Carlos Martín Nieto
0 siblings, 1 reply; 8+ messages in thread
From: Bernhard Reutner-Fischer @ 2012-02-27 12:26 UTC (permalink / raw)
To: git
Hi,
I hope this minor buglet is not known already..
I did not look further.
Seen with
$ dpkg-query -W git
git 1:1.7.2.5-3
and
git 1:1.7.9-1
cheers,
git init
git branch --merged 0000000000000000000000000000000000000000
Segmentation fault
touch f
git commit -m one f
[master (root-commit) f11ad60] one
0 files changed, 0 insertions(+), 0 deletions(-)
create mode 100644 f
git branch --merged $((echo "obase=16;ibase=16;$(git log -n1 --format='%H'| tr '[[:lower:]]' '[[:upper:]]')+1") | bc)
Segmentation fault
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] branch: don't assume the merge filter ref exists
2012-02-27 12:26 [BUG] git branch --merged $unknown_checksum segfaults Bernhard Reutner-Fischer
@ 2012-02-27 15:11 ` Carlos Martín Nieto
2012-02-27 18:05 ` Junio C Hamano
2012-02-27 19:30 ` Jeff King
0 siblings, 2 replies; 8+ messages in thread
From: Carlos Martín Nieto @ 2012-02-27 15:11 UTC (permalink / raw)
To: Bernhard Reutner-Fischer; +Cc: git
print_ref_list looks up the merge_filter_ref and assumes that a valid
pointer is returned. When the object doesn't exist, it tries to
dereference a NULL pointer. This can be the case when git branch
--merged is given an argument that isn't a valid commit name.
Check whether the lookup returns a NULL pointer and die with an error
if it does. Add a test, while we're at it.
Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---
It certainly looks like --merged was only ever supposed to be used
with branch names, as it assumed that get_sha1() would catch the
errors.
I'm not sure if "bad object" or "invalid object" fits better. "bad
object" might have a stronger implication that it exists but is
corrupt.
builtin/branch.c | 3 +++
t/t3200-branch.sh | 4 ++++
2 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index cb17bc3..b63d5fe 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -530,6 +530,9 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
if (merge_filter != NO_FILTER) {
struct commit *filter;
filter = lookup_commit_reference_gently(merge_filter_ref, 0);
+ if (!filter)
+ die("bad object %s", sha1_to_hex(merge_filter_ref));
+
filter->object.flags |= UNINTERESTING;
add_pending_object(&ref_list.revs,
(struct object *) filter, "");
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index dd1aceb..9fe1d8f 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -653,4 +653,8 @@ test_expect_success 'refuse --edit-description on unborn branch for now' '
)
'
+test_expect_success '--merged catches invalid object names' '
+ test_must_fail git branch --merged 0000000000000000000000000000000000000000
+'
+
test_done
--
1.7.9.rc0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] branch: don't assume the merge filter ref exists
2012-02-27 15:11 ` [PATCH] branch: don't assume the merge filter ref exists Carlos Martín Nieto
@ 2012-02-27 18:05 ` Junio C Hamano
2012-02-27 19:30 ` Jeff King
1 sibling, 0 replies; 8+ messages in thread
From: Junio C Hamano @ 2012-02-27 18:05 UTC (permalink / raw)
To: Carlos Martín Nieto; +Cc: Bernhard Reutner-Fischer, git
Thanks.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] branch: don't assume the merge filter ref exists
2012-02-27 15:11 ` [PATCH] branch: don't assume the merge filter ref exists Carlos Martín Nieto
2012-02-27 18:05 ` Junio C Hamano
@ 2012-02-27 19:30 ` Jeff King
2012-02-27 19:33 ` Junio C Hamano
1 sibling, 1 reply; 8+ messages in thread
From: Jeff King @ 2012-02-27 19:30 UTC (permalink / raw)
To: Carlos Martín Nieto; +Cc: Bernhard Reutner-Fischer, git, Junio C Hamano
On Mon, Feb 27, 2012 at 04:11:53PM +0100, Carlos Martín Nieto wrote:
> print_ref_list looks up the merge_filter_ref and assumes that a valid
> pointer is returned. When the object doesn't exist, it tries to
> dereference a NULL pointer. This can be the case when git branch
> --merged is given an argument that isn't a valid commit name.
>
> Check whether the lookup returns a NULL pointer and die with an error
> if it does. Add a test, while we're at it.
>
> Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
> ---
>
> It certainly looks like --merged was only ever supposed to be used
> with branch names, as it assumed that get_sha1() would catch the
> errors.
>
> I'm not sure if "bad object" or "invalid object" fits better. "bad
> object" might have a stronger implication that it exists but is
> corrupt.
You would also get NULL if the object exists but is not a commit. Maybe:
die("object '%s' does not point to a commit", ...)
would be better? It covers the wrong-type case, and is still technically
true when the object does not exist.
-Peff
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] branch: don't assume the merge filter ref exists
2012-02-27 19:30 ` Jeff King
@ 2012-02-27 19:33 ` Junio C Hamano
2012-02-27 19:43 ` Jeff King
0 siblings, 1 reply; 8+ messages in thread
From: Junio C Hamano @ 2012-02-27 19:33 UTC (permalink / raw)
To: Jeff King; +Cc: Carlos Martín Nieto, Bernhard Reutner-Fischer, git
Jeff King <peff@peff.net> writes:
> You would also get NULL if the object exists but is not a commit. Maybe:
>
> die("object '%s' does not point to a commit", ...)
>
> would be better? It covers the wrong-type case, and is still technically
> true when the object does not exist.
For this particular message I like the above a lot better. The output
from "git grep -e 'invalid object' -e 'bad object'" seems to show that
the use of both are fairly evenly distributed.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] branch: don't assume the merge filter ref exists
2012-02-27 19:33 ` Junio C Hamano
@ 2012-02-27 19:43 ` Jeff King
2012-02-28 15:14 ` Carlos Martín Nieto
0 siblings, 1 reply; 8+ messages in thread
From: Jeff King @ 2012-02-27 19:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Carlos Martín Nieto, Bernhard Reutner-Fischer, git
On Mon, Feb 27, 2012 at 11:33:49AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > You would also get NULL if the object exists but is not a commit. Maybe:
> >
> > die("object '%s' does not point to a commit", ...)
> >
> > would be better? It covers the wrong-type case, and is still technically
> > true when the object does not exist.
>
> For this particular message I like the above a lot better. The output
> from "git grep -e 'invalid object' -e 'bad object'" seems to show that
> the use of both are fairly evenly distributed.
It looks like "bad object" generally comes from parse_object failing,
which makes sense. It either means object corruption or you fed a full
40-char sha1 that didn't exist (which, if you are being that specific,
probably is an indication of broken-ness in your repository).
It looks like "invalid object" comes from failing to access the subject
of an annotated tag or an entry in a tree, both of which would meet the
same criteria (corruption or a missing 40-char sha1).
I don't think bad versus invalid in existing cases is a big deal, as
they are both used consistently. But in this case, I think either would
be wrong, since it is equally likely that the user gave an existing, OK
object of the wrong type.
-Peff
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] branch: don't assume the merge filter ref exists
2012-02-27 19:43 ` Jeff King
@ 2012-02-28 15:14 ` Carlos Martín Nieto
2012-02-28 17:32 ` Junio C Hamano
0 siblings, 1 reply; 8+ messages in thread
From: Carlos Martín Nieto @ 2012-02-28 15:14 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Bernhard Reutner-Fischer, git
[-- Attachment #1: Type: text/plain, Size: 1251 bytes --]
On Mon, 2012-02-27 at 14:43 -0500, Jeff King wrote:
> On Mon, Feb 27, 2012 at 11:33:49AM -0800, Junio C Hamano wrote:
>
> > Jeff King <peff@peff.net> writes:
> >
> > > You would also get NULL if the object exists but is not a commit. Maybe:
> > >
> > > die("object '%s' does not point to a commit", ...)
> > >
> > > would be better? It covers the wrong-type case, and is still technically
> > > true when the object does not exist.
> >
> > For this particular message I like the above a lot better. The output
> > from "git grep -e 'invalid object' -e 'bad object'" seems to show that
> > the use of both are fairly evenly distributed.
>
> It looks like "bad object" generally comes from parse_object failing,
> which makes sense. It either means object corruption or you fed a full
> 40-char sha1 that didn't exist (which, if you are being that specific,
> probably is an indication of broken-ness in your repository).
Right. Another version of the fix I was playing with used parse_object
after get_sha1 in opt_parse_merge_filter to make sure there that the
objects did exist, so I copied that error message for this patch.
I see Junio's already squashed this in, and it's certainly a better
message.
cmn
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] branch: don't assume the merge filter ref exists
2012-02-28 15:14 ` Carlos Martín Nieto
@ 2012-02-28 17:32 ` Junio C Hamano
0 siblings, 0 replies; 8+ messages in thread
From: Junio C Hamano @ 2012-02-28 17:32 UTC (permalink / raw)
To: Carlos Martín Nieto; +Cc: Jeff King, Bernhard Reutner-Fischer, git
Carlos Martín Nieto <cmn@elego.de> writes:
> On Mon, 2012-02-27 at 14:43 -0500, Jeff King wrote:
>>
>> It looks like "bad object" generally comes from parse_object failing,
>> which makes sense. It either means object corruption or you fed a full
>> 40-char sha1 that didn't exist (which, if you are being that specific,
>> probably is an indication of broken-ness in your repository).
>
> Right. Another version of the fix I was playing with used parse_object
> after get_sha1 in opt_parse_merge_filter to make sure there that the
> objects did exist, so I copied that error message for this patch.
>
> I see Junio's already squashed this in, and it's certainly a better
> message.
Ok, so what I have is good for everybody. Will merge to "next" and soon
to "master" and "maint".
Thanks, both.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2012-02-28 17:32 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-02-27 12:26 [BUG] git branch --merged $unknown_checksum segfaults Bernhard Reutner-Fischer
2012-02-27 15:11 ` [PATCH] branch: don't assume the merge filter ref exists Carlos Martín Nieto
2012-02-27 18:05 ` Junio C Hamano
2012-02-27 19:30 ` Jeff King
2012-02-27 19:33 ` Junio C Hamano
2012-02-27 19:43 ` Jeff King
2012-02-28 15:14 ` Carlos Martín Nieto
2012-02-28 17:32 ` Junio C Hamano
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.