[BUG] git branch --merged $unknown_checksum segfaults

[BUG] git branch --merged $unknown_checksum segfaults

[Bernhard Reutner-Fischer]

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

[PATCH] branch: don't assume the merge filter ref exists

[Carlos Martín Nieto]

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

Re: [PATCH] branch: don't assume the merge filter ref exists

[Junio C Hamano]

Thanks.

Re: [PATCH] branch: don't assume the merge filter ref exists

[Jeff King]

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

Re: [PATCH] branch: don't assume the merge filter ref exists

[Junio C Hamano]

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.

Re: [PATCH] branch: don't assume the merge filter ref exists

[Jeff King]

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

Re: [PATCH] branch: don't assume the merge filter ref exists

[Carlos Martín Nieto]

[-- 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 --]

Re: [PATCH] branch: don't assume the merge filter ref exists

[Junio C Hamano]

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.