{"thread":{"id":"29755","subject":"[BUG] git branch --merged $unknown_checksum segfaults","startedAt":"2012-02-27T12:26:09Z","lastAt":"2012-02-28T17:32:21Z","messageCount":8,"participants":["Bernhard Reutner-Fischer","Carlos Martín Nieto","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"185515","messageId":"20120227122609.GA26981@mx.loc","threadId":"29755","inReplyTo":null,"subject":"[BUG] git branch --merged $unknown_checksum segfaults","fromName":"Bernhard Reutner-Fischer","fromEmail":"rep.dot.nop@gmail.com","sentAt":"2012-02-27T12:26:09Z","receivedAt":"2012-02-27T12:26:09Z","isPatch":false,"sender":{"key":"rep.dot.nop@gmail.com","avatar":null},"body":"Hi,\n\nI hope this minor buglet is not known already..\nI did not look further.\n\nSeen with\n$ dpkg-query -W git\ngit\t1:1.7.2.5-3\nand\ngit\t1:1.7.9-1\ncheers,\n\ngit init\ngit branch --merged 0000000000000000000000000000000000000000\nSegmentation fault\ntouch f\ngit commit -m one f\n[master (root-commit) f11ad60] one\n 0 files changed, 0 insertions(+), 0 deletions(-)\n create mode 100644 f\ngit branch --merged $((echo \"obase=16;ibase=16;$(git log -n1 --format='%H'| tr '[[:lower:]]' '[[:upper:]]')+1\") | bc)\nSegmentation fault\n"},{"id":"185517","messageId":"1330355513-22351-1-git-send-email-cmn@elego.de","threadId":"29755","inReplyTo":"20120227122609.GA26981@mx.loc","subject":"[PATCH] branch: don't assume the merge filter ref exists","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2012-02-27T15:11:53Z","receivedAt":"2012-02-27T15:11:53Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"print_ref_list looks up the merge_filter_ref and assumes that a valid\npointer is returned. When the object doesn't exist, it tries to\ndereference a NULL pointer. This can be the case when git branch\n--merged is given an argument that isn't a valid commit name.\n\nCheck whether the lookup returns a NULL pointer and die with an error\nif it does. Add a test, while we're at it.\n\nSigned-off-by: Carlos Martín Nieto \n---\n\nIt certainly looks like --merged was only ever supposed to be used\nwith branch names, as it assumed that get_sha1() would catch the\nerrors.\n\nI'm not sure if \"bad object\" or \"invalid object\" fits better. \"bad\nobject\" might have a stronger implication that it exists but is\ncorrupt.\n\n builtin/branch.c | 3 +++\n t/t3200-branch.sh | 4 ++++\n 2 files changed, 7 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex cb17bc3..b63d5fe 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -530,6 +530,9 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru\n \tif (merge_filter != NO_FILTER) {\n \t\tstruct commit *filter;\n \t\tfilter = lookup_commit_reference_gently(merge_filter_ref, 0);\n+\t\tif (!filter)\n+\t\t\tdie(\"bad object %s\", sha1_to_hex(merge_filter_ref));\n+\n \t\tfilter->object.flags |= UNINTERESTING;\n \t\tadd_pending_object(&ref_list.revs,\n \t\t\t\t (struct object *) filter, \"\");\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex dd1aceb..9fe1d8f 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -653,4 +653,8 @@ test_expect_success 'refuse --edit-description on unborn branch for now' '\n \t)\n '\n \n+test_expect_success '--merged catches invalid object names' '\n+\ttest_must_fail git branch --merged 0000000000000000000000000000000000000000\n+'\n+\n test_done\n-- \n1.7.9.rc0\n"},{"id":"185532","messageId":"7vr4xg9mns.fsf@alter.siamese.dyndns.org","threadId":"29755","inReplyTo":"1330355513-22351-1-git-send-email-cmn@elego.de","subject":"Re: [PATCH] branch: don't assume the merge filter ref exists","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-27T18:05:59Z","receivedAt":"2012-02-27T18:05:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks.\n"},{"id":"185549","messageId":"20120227193044.GD1600@sigill.intra.peff.net","threadId":"29755","inReplyTo":"1330355513-22351-1-git-send-email-cmn@elego.de","subject":"Re: [PATCH] branch: don't assume the merge filter ref exists","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-27T19:30:44Z","receivedAt":"2012-02-27T19:30:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 27, 2012 at 04:11:53PM +0100, Carlos Martín Nieto wrote:\n\n> print_ref_list looks up the merge_filter_ref and assumes that a valid\n> pointer is returned. When the object doesn't exist, it tries to\n> dereference a NULL pointer. This can be the case when git branch\n> --merged is given an argument that isn't a valid commit name.\n> \n> Check whether the lookup returns a NULL pointer and die with an error\n> if it does. Add a test, while we're at it.\n> \n> Signed-off-by: Carlos Martín Nieto \n> ---\n> \n> It certainly looks like --merged was only ever supposed to be used\n> with branch names, as it assumed that get_sha1() would catch the\n> errors.\n> \n> I'm not sure if \"bad object\" or \"invalid object\" fits better. \"bad\n> object\" might have a stronger implication that it exists but is\n> corrupt.\n\nYou would also get NULL if the object exists but is not a commit. Maybe:\n\n die(\"object '%s' does not point to a commit\", ...)\n\nwould be better? It covers the wrong-type case, and is still technically\ntrue when the object does not exist.\n\n-Peff\n"},{"id":"185550","messageId":"7vk4386pgi.fsf@alter.siamese.dyndns.org","threadId":"29755","inReplyTo":"20120227193044.GD1600@sigill.intra.peff.net","subject":"Re: [PATCH] branch: don't assume the merge filter ref exists","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-27T19:33:49Z","receivedAt":"2012-02-27T19:33:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> You would also get NULL if the object exists but is not a commit. Maybe:\n>\n> die(\"object '%s' does not point to a commit\", ...)\n>\n> would be better? It covers the wrong-type case, and is still technically\n> true when the object does not exist.\n\nFor this particular message I like the above a lot better. The output\nfrom \"git grep -e 'invalid object' -e 'bad object'\" seems to show that\nthe use of both are fairly evenly distributed.\n"},{"id":"185551","messageId":"20120227194305.GE1600@sigill.intra.peff.net","threadId":"29755","inReplyTo":"7vk4386pgi.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] branch: don't assume the merge filter ref exists","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-02-27T19:43:05Z","receivedAt":"2012-02-27T19:43:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 27, 2012 at 11:33:49AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > You would also get NULL if the object exists but is not a commit. Maybe:\n> >\n> > die(\"object '%s' does not point to a commit\", ...)\n> >\n> > would be better? It covers the wrong-type case, and is still technically\n> > true when the object does not exist.\n> \n> For this particular message I like the above a lot better. The output\n> from \"git grep -e 'invalid object' -e 'bad object'\" seems to show that\n> the use of both are fairly evenly distributed.\n\nIt looks like \"bad object\" generally comes from parse_object failing,\nwhich makes sense. It either means object corruption or you fed a full\n40-char sha1 that didn't exist (which, if you are being that specific,\nprobably is an indication of broken-ness in your repository).\n\nIt looks like \"invalid object\" comes from failing to access the subject\nof an annotated tag or an entry in a tree, both of which would meet the\nsame criteria (corruption or a missing 40-char sha1).\n\nI don't think bad versus invalid in existing cases is a big deal, as\nthey are both used consistently. But in this case, I think either would\nbe wrong, since it is equally likely that the user gave an existing, OK\nobject of the wrong type.\n\n-Peff\n"},{"id":"185627","messageId":"1330442090.691.9.camel@centaur.lab.cmartin.tk","threadId":"29755","inReplyTo":"20120227194305.GE1600@sigill.intra.peff.net","subject":"Re: [PATCH] branch: don't assume the merge filter ref exists","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2012-02-28T15:14:50Z","receivedAt":"2012-02-28T15:14:50Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"On Mon, 2012-02-27 at 14:43 -0500, Jeff King wrote:\n> On Mon, Feb 27, 2012 at 11:33:49AM -0800, Junio C Hamano wrote:\n> \n> > Jeff King writes:\n> > \n> > > You would also get NULL if the object exists but is not a commit. Maybe:\n> > >\n> > > die(\"object '%s' does not point to a commit\", ...)\n> > >\n> > > would be better? It covers the wrong-type case, and is still technically\n> > > true when the object does not exist.\n> > \n> > For this particular message I like the above a lot better. The output\n> > from \"git grep -e 'invalid object' -e 'bad object'\" seems to show that\n> > the use of both are fairly evenly distributed.\n> \n> It looks like \"bad object\" generally comes from parse_object failing,\n> which makes sense. It either means object corruption or you fed a full\n> 40-char sha1 that didn't exist (which, if you are being that specific,\n> probably is an indication of broken-ness in your repository).\n\nRight. Another version of the fix I was playing with used parse_object\nafter get_sha1 in opt_parse_merge_filter to make sure there that the\nobjects did exist, so I copied that error message for this patch.\n\nI see Junio's already squashed this in, and it's certainly a better\nmessage.\n\n cmn\n\n"},{"id":"185639","messageId":"7vehte27a2.fsf@alter.siamese.dyndns.org","threadId":"29755","inReplyTo":"1330442090.691.9.camel@centaur.lab.cmartin.tk","subject":"Re: [PATCH] branch: don't assume the merge filter ref exists","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-28T17:32:21Z","receivedAt":"2012-02-28T17:32:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlos Martín Nieto writes:\n\n> On Mon, 2012-02-27 at 14:43 -0500, Jeff King wrote:\n>> \n>> It looks like \"bad object\" generally comes from parse_object failing,\n>> which makes sense. It either means object corruption or you fed a full\n>> 40-char sha1 that didn't exist (which, if you are being that specific,\n>> probably is an indication of broken-ness in your repository).\n>\n> Right. Another version of the fix I was playing with used parse_object\n> after get_sha1 in opt_parse_merge_filter to make sure there that the\n> objects did exist, so I copied that error message for this patch.\n>\n> I see Junio's already squashed this in, and it's certainly a better\n> message.\n\nOk, so what I have is good for everybody. Will merge to \"next\" and soon\nto \"master\" and \"maint\".\n\nThanks, both.\n"}]}