threads / bug / 29755

[BUG] git branch --merged $unknown_checksum segfaults

Subject: [BUG] git branch --merged $unknown_checksum segfaults

## tl;dr

8 messages between Feb 27, 2012 and Feb 28, 2012.

replies: 7people: 4as markdown or json

Bernhard Reutner-Fischer· Feb 27, 2012, 12:26 UTC · lore
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· Feb 27, 2012, 15:11 UTC · re: Bernhard Reutner-Fischer · lore

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

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
Jeff King· Feb 27, 2012, 19:30 UTC · re: Carlos Martín Nieto · lore

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

On Mon, Feb 27, 2012 at 04:11:53PM +0100, Carlos Martín Nieto wrote:
Show 18 quoted lines
> 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
Junio C Hamano· Feb 27, 2012, 19:33 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 6 quoted lines
> 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· Feb 27, 2012, 19:43 UTC · re: Junio C Hamano · lore

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

On Mon, Feb 27, 2012 at 11:33:49AM -0800, Junio C Hamano wrote:
Show 12 quoted lines
> 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
Carlos Martín Nieto· Feb 28, 2012, 15:14 UTC · re: Jeff King · lore

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

On Mon, 2012-02-27 at 14:43 -0500, Jeff King wrote:
Show 19 quoted lines
> 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
Junio C Hamano· Feb 28, 2012, 17:32 UTC · re: Carlos Martín Nieto · lore

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

Carlos Martín Nieto <cmn@elego.de> writes:
Show 13 quoted lines
> 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.

← back to recent threads