{"thread":{"id":"51806","subject":"[PATCH 1/2] tag: factor out get_tagged_oid()","startedAt":"2019-09-05T19:56:08Z","lastAt":"2019-09-06T17:51:27Z","messageCount":6,"participants":["René Scharfe","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"381908","messageId":"1385c954-d9ef-7ef6-6185-0dad885531ec@web.de","threadId":"51806","inReplyTo":null,"subject":"[PATCH 1/2] tag: factor out get_tagged_oid()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-09-05T19:55:55Z","receivedAt":"2019-09-05T19:56:08Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Add a function for accessing the ID of the object referenced by a tag\nsafely, i.e. without causing a segfault when encountering a broken tag\nwhere ->tagged is NULL.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n pack-bitmap.c | 4 +---\n revision.c    | 4 +---\n tag.c         | 7 +++++++\n tag.h         | 1 +\n 4 files changed, 10 insertions(+), 6 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex ed2befaac6..30842e1e74 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -709,9 +709,7 @@ struct bitmap_index *prepare_bitmap_walk(struct rev_info *revs)\n \t\t\telse\n \t\t\t\tobject_list_insert(object, &wants);\n\n-\t\t\tif (!tag->tagged)\n-\t\t\t\tdie(\"bad tag\");\n-\t\t\tobject = parse_object_or_die(&tag->tagged->oid, NULL);\n+\t\t\tobject = parse_object_or_die(get_tagged_oid(tag), NULL);\n \t\t}\n\n \t\tif (object->flags & UNINTERESTING)\ndiff --git a/revision.c b/revision.c\nindex 07412297f0..ee1b1552b9 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -404,9 +404,7 @@ static struct commit *handle_commit(struct rev_info *revs,\n \t\tstruct tag *tag = (struct tag *) object;\n \t\tif (revs->tag_objects && !(flags & UNINTERESTING))\n \t\t\tadd_pending_object(revs, object, tag->tag);\n-\t\tif (!tag->tagged)\n-\t\t\tdie(\"bad tag\");\n-\t\tobject = parse_object(revs->repo, &tag->tagged->oid);\n+\t\tobject = parse_object(revs->repo, get_tagged_oid(tag));\n \t\tif (!object) {\n \t\t\tif (revs->ignore_missing_links || (flags & UNINTERESTING))\n \t\t\t\treturn NULL;\ndiff --git a/tag.c b/tag.c\nindex 5db870edb9..bfa0e31435 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -212,3 +212,10 @@ int parse_tag(struct tag *item)\n \tfree(data);\n \treturn ret;\n }\n+\n+struct object_id *get_tagged_oid(struct tag *tag)\n+{\n+\tif (!tag->tagged)\n+\t\tdie(\"bad tag\");\n+\treturn &tag->tagged->oid;\n+}\ndiff --git a/tag.h b/tag.h\nindex 03265fbfe2..3ce8e72192 100644\n--- a/tag.h\n+++ b/tag.h\n@@ -19,5 +19,6 @@ struct object *deref_tag(struct repository *r, struct object *, const char *, in\n struct object *deref_tag_noverify(struct object *);\n int gpg_verify_tag(const struct object_id *oid,\n \t\t   const char *name_to_report, unsigned flags);\n+struct object_id *get_tagged_oid(struct tag *tag);\n\n #endif /* TAG_H */\n--\n2.23.0\n"},{"id":"381910","messageId":"eaafa812-33cc-843b-391c-a9f8e9725777@web.de","threadId":"51806","inReplyTo":"1385c954-d9ef-7ef6-6185-0dad885531ec@web.de","subject":"[PATCH 2/2] use get_tagged_oid()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-09-05T19:59:42Z","receivedAt":"2019-09-05T19:59:51Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Avoid derefencing ->tagged without checking for NULL by using the\nconvenience wrapper for getting the ID of the tagged object.  It die()s\nwhen encountering a broken tag instead of segfaulting.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n builtin/describe.c | 2 +-\n builtin/log.c      | 5 +++--\n builtin/replace.c  | 2 +-\n packfile.c         | 2 +-\n ref-filter.c       | 4 ++--\n 5 files changed, 8 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex 200154297d..e048f85484 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -313,7 +313,7 @@ static void describe_commit(struct object_id *oid, struct strbuf *dst)\n \t\t */\n \t\tappend_name(n, dst);\n \t\tif (longformat)\n-\t\t\tappend_suffix(0, n->tag ? &n->tag->tagged->oid : oid, dst);\n+\t\t\tappend_suffix(0, n->tag ? get_tagged_oid(n->tag) : oid, dst);\n \t\tif (suffix)\n \t\t\tstrbuf_addstr(dst, suffix);\n \t\treturn;\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 44b10b3415..c4b35fdaf9 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -627,6 +627,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n \t\t\tbreak;\n \t\tcase OBJ_TAG: {\n \t\t\tstruct tag *t = (struct tag *)o;\n+\t\t\tstruct object_id *oid = get_tagged_oid(t);\n\n \t\t\tif (rev.shown_one)\n \t\t\t\tputchar('\\n');\n@@ -638,10 +639,10 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n \t\t\trev.shown_one = 1;\n \t\t\tif (ret)\n \t\t\t\tbreak;\n-\t\t\to = parse_object(the_repository, &t->tagged->oid);\n+\t\t\to = parse_object(the_repository, oid);\n \t\t\tif (!o)\n \t\t\t\tret = error(_(\"could not read object %s\"),\n-\t\t\t\t\t    oid_to_hex(&t->tagged->oid));\n+\t\t\t\t\t    oid_to_hex(oid));\n \t\t\tobjects[i].item = o;\n \t\t\ti--;\n \t\t\tbreak;\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex 644b21ca8d..2a4afb3b93 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -421,7 +421,7 @@ static int check_one_mergetag(struct commit *commit,\n \t\tif (get_oid(mergetag_data->argv[i], &oid) < 0)\n \t\t\treturn error(_(\"not a valid object name: '%s'\"),\n \t\t\t\t     mergetag_data->argv[i]);\n-\t\tif (oideq(&tag->tagged->oid, &oid))\n+\t\tif (oideq(get_tagged_oid(tag), &oid))\n \t\t\treturn 0; /* found */\n \t}\n\ndiff --git a/packfile.c b/packfile.c\nindex fc43a6c52c..a62ab4cb17 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -2139,7 +2139,7 @@ static int add_promisor_object(const struct object_id *oid,\n \t\t\toidset_insert(set, &parents->item->object.oid);\n \t} else if (obj->type == OBJ_TAG) {\n \t\tstruct tag *tag = (struct tag *) obj;\n-\t\toidset_insert(set, &tag->tagged->oid);\n+\t\toidset_insert(set, get_tagged_oid(tag));\n \t}\n \treturn 0;\n }\ndiff --git a/ref-filter.c b/ref-filter.c\nindex f27cfc8c3e..8dcc17c049 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1766,7 +1766,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)\n \t * If it is a tag object, see if we use a value that derefs\n \t * the object, and if we do grab the object it refers to.\n \t */\n-\toi_deref.oid = ((struct tag *)obj)->tagged->oid;\n+\toi_deref.oid = *get_tagged_oid((struct tag *)obj);\n\n \t/*\n \t * NEEDSWORK: This derefs tag only once, which\n@@ -1997,7 +1997,7 @@ static const struct object_id *match_points_at(struct oid_array *points_at,\n \tif (!obj)\n \t\tdie(_(\"malformed object at '%s'\"), refname);\n \tif (obj->type == OBJ_TAG)\n-\t\ttagged_oid = &((struct tag *)obj)->tagged->oid;\n+\t\ttagged_oid = get_tagged_oid((struct tag *)obj);\n \tif (tagged_oid && oid_array_lookup(points_at, tagged_oid) >= 0)\n \t\treturn tagged_oid;\n \treturn NULL;\n--\n2.23.0\n\n"},{"id":"381948","messageId":"20190906071320.GA14343@sigill.intra.peff.net","threadId":"51806","inReplyTo":"1385c954-d9ef-7ef6-6185-0dad885531ec@web.de","subject":"Re: [PATCH 1/2] tag: factor out get_tagged_oid()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-06T07:13:21Z","receivedAt":"2019-09-06T07:13:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 05, 2019 at 09:55:55PM +0200, René Scharfe wrote:\n\n> Add a function for accessing the ID of the object referenced by a tag\n> safely, i.e. without causing a segfault when encountering a broken tag\n> where ->tagged is NULL.\n\nThis approach seems to pretty reasonable. As somebody who's been\nthinking about this, I'd be curious to hear your thoughts on:\n\n  https://public-inbox.org/git/20190906065606.GC5122@sigill.intra.peff.net/\n\nwhich _in theory_ means tag->tagged would never be NULL (we'd catch it\nat the parsing stage and consider that an error). But we'd still\npotentially want to protect ourselves as you do here for code paths\nwhich don't necessarily check the parse result.\n\n-Peff\n"},{"id":"381958","messageId":"f44b7abc-cab1-0bd4-029a-1c84cf36ec7b@web.de","threadId":"51806","inReplyTo":"20190906071320.GA14343@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] tag: factor out get_tagged_oid()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-09-06T15:05:11Z","receivedAt":"2019-09-06T15:05:34Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 06.09.19 um 09:13 schrieb Jeff King:\n> On Thu, Sep 05, 2019 at 09:55:55PM +0200, René Scharfe wrote:\n>\n>> Add a function for accessing the ID of the object referenced by a tag\n>> safely, i.e. without causing a segfault when encountering a broken tag\n>> where ->tagged is NULL.\n>\n> This approach seems to pretty reasonable. As somebody who's been\n> thinking about this, I'd be curious to hear your thoughts on:\n>\n>   https://public-inbox.org/git/20190906065606.GC5122@sigill.intra.peff.net/\n>\n> which _in theory_ means tag->tagged would never be NULL (we'd catch it\n> at the parsing stage and consider that an error). But we'd still\n> potentially want to protect ourselves as you do here for code paths\n> which don't necessarily check the parse result.\n\nA tag referencing an unknown object sounds strange to me.  I imagine we\nmight get such a thing when the referenced object is lost (broken repo)\nor purpose-built from an attacker.  Could such a tag still be used for\nanything?  Are there other possible causes?  I suspect the answer to\nboth questions is \"no\", and then it makes sense to reject it as early\nas possible.\n\nBut I may be missing something.  In particular I'm confused by these\npatches from February 2008, which seem to suggest that such tags should\nnot be reported in all cases, but sometimes just silently ignored:\n\n   9684afd967 revision.c: handle tag->tagged == NULL\n   cc36934791 process_tag: handle tag->tagged == NULL\n   24e8a3c946 deref_tag: handle tag->tagged = NULL\n\nSo is there perhaps a use case for them after all?\n\nLeaving that aside: The parsed flag means we saw and checked the object\nalready.  That is true also for broken objects.  Clearing the flag can\ncause the same error to be reported multiple times.  How about setting\nit at the start as before, but returning -1 from parse_tag_buffer() if\n.parsed == 1 && .tagged == NULL?\n\nRené\n"},{"id":"381959","messageId":"c1ffd461-7283-eeaa-6f68-778c5a35e8eb@web.de","threadId":"51806","inReplyTo":"f44b7abc-cab1-0bd4-029a-1c84cf36ec7b@web.de","subject":"Re: [PATCH 1/2] tag: factor out get_tagged_oid()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-09-06T15:25:30Z","receivedAt":"2019-09-06T15:25:45Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 06.09.19 um 17:05 schrieb René Scharfe:\n> A tag referencing an unknown object sounds strange to me.  I imagine we\n> might get such a thing when the referenced object is lost (broken repo)\n> or purpose-built from an attacker.  Could such a tag still be used for\n> anything?  Are there other possible causes?\nForward compatibility perhaps, i.e. supporting tags (by ignoring them)\nthat point to a new type of object introduced by a future version.\n\nRené\n"},{"id":"381978","messageId":"20190906175124.GG23181@sigill.intra.peff.net","threadId":"51806","inReplyTo":"f44b7abc-cab1-0bd4-029a-1c84cf36ec7b@web.de","subject":"Re: [PATCH 1/2] tag: factor out get_tagged_oid()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-06T17:51:25Z","receivedAt":"2019-09-06T17:51:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 06, 2019 at 05:05:11PM +0200, René Scharfe wrote:\n\n> > This approach seems to pretty reasonable. As somebody who's been\n> > thinking about this, I'd be curious to hear your thoughts on:\n> >\n> >   https://public-inbox.org/git/20190906065606.GC5122@sigill.intra.peff.net/\n> >\n> > which _in theory_ means tag->tagged would never be NULL (we'd catch it\n> > at the parsing stage and consider that an error). But we'd still\n> > potentially want to protect ourselves as you do here for code paths\n> > which don't necessarily check the parse result.\n> \n> A tag referencing an unknown object sounds strange to me.  I imagine we\n> might get such a thing when the referenced object is lost (broken repo)\n> or purpose-built from an attacker.  Could such a tag still be used for\n> anything?  Are there other possible causes?  I suspect the answer to\n> both questions is \"no\", and then it makes sense to reject it as early\n> as possible.\n\nI don't think there's really a valid case. Keep in mind that this isn't\nchecking for the object on-disk at all. It's only checking:\n\n  - were we able to parse the tag at all (i.e., is it syntactically\n    valid)\n\n  - does it have a type we know about\n\n  - were we able to create an in-memory struct, meaning we did not see\n    the same oid with a different type elsewhere in the same process\n\nThe first and middle imply a corruption or malicious attack. The second\ncould mean forward-compatibility is a concern, but I think this is the\ntip of the iceberg. You really need a Git that understands the object\ntypes you're working with, or basically everything is going to fail.\n\nSo I'm not too concerned with tightening this code. The question to me\nis whether to do it in parse_tag(), or when accessing tag->tagged, or\nboth.\n\n> But I may be missing something.  In particular I'm confused by these\n> patches from February 2008, which seem to suggest that such tags should\n> not be reported in all cases, but sometimes just silently ignored:\n> \n>    9684afd967 revision.c: handle tag->tagged == NULL\n>    cc36934791 process_tag: handle tag->tagged == NULL\n>    24e8a3c946 deref_tag: handle tag->tagged = NULL\n> \n> So is there perhaps a use case for them after all?\n\nThere's not much discussion on the list from that thread, but searching\nfor messages from Martin around that the main goal was protecting\nagainst corruptions. So I think these NULL checks were attempting to do\nthe same thing we're doing here, just in a less centralized way.\n\n> Leaving that aside: The parsed flag means we saw and checked the object\n> already.  That is true also for broken objects.  Clearing the flag can\n> cause the same error to be reported multiple times.  How about setting\n> it at the start as before, but returning -1 from parse_tag_buffer() if\n> .parsed == 1 && .tagged == NULL?\n\nThat wouldn't cover all of the other reasons that we might have seen a\nparse failure (so it fixes \"tagged == NULL\", but means that a subsequent\nparse_tag() will silently return success).\n\nI think the multiple error messages may be OK given that this is\nexplicitly covering a corrupt situation. But the more robust way is\nadding an extra bit to say \"I parsed this already, and it was corrupt\"\nto cause parse_tag() to return an error reliably.\n\n-Peff\n"}]}