# [BUG] git branch --merged $unknown_checksum segfaults 8 messages from 2012-02-27 to 2012-02-28. Participants: Bernhard Reutner-Fischer, Carlos Martín Nieto, Junio C Hamano, Jeff King. Thread: https://gitlist.dev/t/29755 ## Bernhard Reutner-Fischer, 2012-02-27 12:26 Subject: [BUG] git branch --merged $unknown_checksum segfaults Message-ID: <20120227122609.GA26981@mx.loc> URL: https://gitlist.dev/e/20120227122609.GA26981%40mx.loc ``` 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 ``` ## Carlos Martín Nieto, 2012-02-27 15:11 Subject: [PATCH] branch: don't assume the merge filter ref exists Message-ID: <1330355513-22351-1-git-send-email-cmn@elego.de> URL: https://gitlist.dev/e/1330355513-22351-1-git-send-email-cmn%40elego.de In-Reply-To: <20120227122609.GA26981@mx.loc> ``` 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 --- 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 ``` ## Junio C Hamano, 2012-02-27 18:05 Subject: Re: [PATCH] branch: don't assume the merge filter ref exists Message-ID: <7vr4xg9mns.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vr4xg9mns.fsf%40alter.siamese.dyndns.org In-Reply-To: <1330355513-22351-1-git-send-email-cmn@elego.de> ``` Thanks. ``` ## Jeff King, 2012-02-27 19:30 Subject: Re: [PATCH] branch: don't assume the merge filter ref exists Message-ID: <20120227193044.GD1600@sigill.intra.peff.net> URL: https://gitlist.dev/e/20120227193044.GD1600%40sigill.intra.peff.net In-Reply-To: <1330355513-22351-1-git-send-email-cmn@elego.de> ``` 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 > --- > > 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 ``` ## Junio C Hamano, 2012-02-27 19:33 Subject: Re: [PATCH] branch: don't assume the merge filter ref exists Message-ID: <7vk4386pgi.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vk4386pgi.fsf%40alter.siamese.dyndns.org In-Reply-To: <20120227193044.GD1600@sigill.intra.peff.net> ``` Jeff King 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. ``` ## Jeff King, 2012-02-27 19:43 Subject: Re: [PATCH] branch: don't assume the merge filter ref exists Message-ID: <20120227194305.GE1600@sigill.intra.peff.net> URL: https://gitlist.dev/e/20120227194305.GE1600%40sigill.intra.peff.net In-Reply-To: <7vk4386pgi.fsf@alter.siamese.dyndns.org> ``` On Mon, Feb 27, 2012 at 11:33:49AM -0800, Junio C Hamano wrote: > Jeff King 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 ``` ## Carlos Martín Nieto, 2012-02-28 15:14 Subject: Re: [PATCH] branch: don't assume the merge filter ref exists Message-ID: <1330442090.691.9.camel@centaur.lab.cmartin.tk> URL: https://gitlist.dev/e/1330442090.691.9.camel%40centaur.lab.cmartin.tk In-Reply-To: <20120227194305.GE1600@sigill.intra.peff.net> ``` 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 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 ``` ## Junio C Hamano, 2012-02-28 17:32 Subject: Re: [PATCH] branch: don't assume the merge filter ref exists Message-ID: <7vehte27a2.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vehte27a2.fsf%40alter.siamese.dyndns.org In-Reply-To: <1330442090.691.9.camel@centaur.lab.cmartin.tk> ``` Carlos Martín Nieto 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. ```