{"thread":{"id":"55625","subject":"[PATCH] [GSOC] ref-filter: solve bugs caused by enumeration","startedAt":"2021-05-05T15:31:08Z","lastAt":"2021-05-13T15:13:42Z","messageCount":31,"participants":["ZheNing Hu via GitGitGadget","Junio C Hamano","ZheNing Hu"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"423693","messageId":"pull.949.git.1620228664666.gitgitgadget@gmail.com","threadId":"55625","inReplyTo":null,"subject":"[PATCH] [GSOC] ref-filter: solve bugs caused by enumeration","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-05-05T15:31:04Z","receivedAt":"2021-05-05T15:31:08Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu \n\nJohannes Schindelin seems to have introduced a bug in\ncc72385f(for-each-ref: let upstream/push optionally\nreport the remote name), it use `atom->u.remote_ref.option`\nwhich is a member of enumeration in the judgment statement.\nWhen we use other members in the enumeration `used_atom.u`,\nand it happened to fill in `remote_ref.push`, this judgment\nmay still be established and produces errors. So replace the\njudgment statement with `starts_with(name, \"push\")` to fix\nthe error.\n\nSigned-off-by: ZheNing Hu \n---\n [GSOC] ref-filter: solve bugs caused by enumeration\n \n This is the problem I found in the implementation of git for-each-ref\n --format=\"%(notes)\".\n \n I added a new option to the enum used_atom.u , but the program seems to\n output the value of \"%(push)\".\n \n After research, it should be the problem of incorrect use of enumeration\n values here.\n \n Thanks, may the force be with you!\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-949%2Fadlternative%2Fref-filter-enum-bug-fix-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-949/adlternative/ref-filter-enum-bug-fix-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/949\n\n ref-filter.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex a0adb4551d87..f467f2fbbb73 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1730,7 +1730,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)\n \t\t\telse\n \t\t\t\tv->s = xstrdup(\"\");\n \t\t\tcontinue;\n-\t\t} else if (atom->u.remote_ref.push) {\n+\t\t} else if (starts_with(name, \"push\")) {\n \t\t\tconst char *branch_name;\n \t\t\tv->s = xstrdup(\"\");\n \t\t\tif (!skip_prefix(ref->refname, \"refs/heads/\",\n\nbase-commit: 311531c9de557d25ac087c1637818bd2aad6eb3a\n-- \ngitgitgadget\n"},{"id":"423731","messageId":"xmqqfsz01vc2.fsf@gitster.g","threadId":"55625","inReplyTo":"pull.949.git.1620228664666.gitgitgadget@gmail.com","subject":"Re: [PATCH] [GSOC] ref-filter: solve bugs caused by enumeration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-06T01:53:33Z","receivedAt":"2021-05-06T01:53:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"ZheNing Hu via GitGitGadget\" writes:\n\n> From: ZheNing Hu \n>\n> Johannes Schindelin seems to have introduced a bug in\n> cc72385f(for-each-ref: let upstream/push optionally\n> report the remote name), it use `atom->u.remote_ref.option`\n> which is a member of enumeration in the judgment statement.\n\nSorry but I am not sure if our readers would understand what \"a\nmember of enumeration in the judgment statement\" is (I certainly do\nnot), and even more importantly, \"bugs caused by enumeration\" on the\ntitle does not hint much about what problem the patch is trying to\nsolve.\n\n> When we use other members in the enumeration `used_atom.u`,\n> and it happened to fill in `remote_ref.push`, this judgment\n> may still be established and produces errors. So replace the\n> judgment statement with `starts_with(name, \"push\")` to fix\n> the error.\n\nAnd this paragraph does not enlighten all that much, unfortunately.\n\nIs it that a check refers to one member of a union without making\nsure that member is the one in effect in the union? I am most\npuzzled by the mention of \"enumeration\" when there does not appear\nto be any enum involved.\n\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-949%2Fadlternative%2Fref-filter-enum-bug-fix-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-949/adlternative/ref-filter-enum-bug-fix-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/949\n>\n> ref-filter.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/ref-filter.c b/ref-filter.c\n> index a0adb4551d87..f467f2fbbb73 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -1730,7 +1730,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)\n> \t\t\telse\n> \t\t\t\tv->s = xstrdup(\"\");\n> \t\t\tcontinue;\n> -\t\t} else if (atom->u.remote_ref.push) {\n> +\t\t} else if (starts_with(name, \"push\")) {\n> \t\t\tconst char *branch_name;\n> \t\t\tv->s = xstrdup(\"\");\n> \t\t\tif (!skip_prefix(ref->refname, \"refs/heads/\",\n>\n> base-commit: 311531c9de557d25ac087c1637818bd2aad6eb3a\n"},{"id":"423739","messageId":"CAOLTT8SmvRaohV+v2C9eFXyc8O+di5PfZJeWNinmm8X=Ckdveg@mail.gmail.com","threadId":"55625","inReplyTo":"xmqqfsz01vc2.fsf@gitster.g","subject":"Re: [PATCH] [GSOC] ref-filter: solve bugs caused by enumeration","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-05-06T05:02:51Z","receivedAt":"2021-05-06T05:03:12Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Junio C Hamano 于2021年5月6日周四 上午9:53写道：\n>\n> \"ZheNing Hu via GitGitGadget\" writes:\n>\n> > From: ZheNing Hu \n> >\n> > Johannes Schindelin seems to have introduced a bug in\n> > cc72385f(for-each-ref: let upstream/push optionally\n> > report the remote name), it use `atom->u.remote_ref.option`\n> > which is a member of enumeration in the judgment statement.\n>\n> Sorry but I am not sure if our readers would understand what \"a\n> member of enumeration in the judgment statement\" is (I certainly do\n> not), and even more importantly, \"bugs caused by enumeration\" on the\n> title does not hint much about what problem the patch is trying to\n> solve.\n>\n> > When we use other members in the enumeration `used_atom.u`,\n> > and it happened to fill in `remote_ref.push`, this judgment\n> > may still be established and produces errors. So replace the\n> > judgment statement with `starts_with(name, \"push\")` to fix\n> > the error.\n>\n> And this paragraph does not enlighten all that much, unfortunately.\n>\n> Is it that a check refers to one member of a union without making\n> sure that member is the one in effect in the union? I am most\n> puzzled by the mention of \"enumeration\" when there does not appear\n> to be any enum involved.\n>\n\nSorry, I didn't make it clear. I re-describe the problem first, and then\nmodify the commit messages.\n\nSuppose we are dealing with \"%(notes)\", the name member of our\n`used_atom` item at this time is \"notes\", and its member union `u` uses\na `struct notes_option`, we fill some values in `used_atom.u.notes_option`,\n\nWhen we traverse in `used_atom` array in `populate_value()` and previous\njudgement like \"if (starts_with(name, \"refname\"))\" will failed, because we\nare dealing with atom \"notes\", but in judgement \"else if\n(atom->u.remote_ref.push)\",\nThe value we fill in `used_atom.u.notes_option` just makes\n`used_atom.u.remote_ref.push` non-zero. This leads us into the wrong case.\n\nIs this clearer?\n\nThanks.\n--\nZheNing Hu\n"},{"id":"423740","messageId":"xmqqk0oczao3.fsf@gitster.g","threadId":"55625","inReplyTo":"CAOLTT8SmvRaohV+v2C9eFXyc8O+di5PfZJeWNinmm8X=Ckdveg@mail.gmail.com","subject":"Re: [PATCH] [GSOC] ref-filter: solve bugs caused by enumeration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-06T05:35:56Z","receivedAt":"2021-05-06T05:36:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"ZheNing Hu writes:\n\n>> Is it that a check refers to one member of a union without making\n>> sure that member is the one in effect in the union? I am most\n>> puzzled by the mention of \"enumeration\" when there does not appear\n>> to be any enum involved.\n>\n> Sorry, I didn't make it clear. I re-describe the problem first, and then\n> modify the commit messages.\n>\n> Suppose we are dealing with \"%(notes)\", the name member of our\n> `used_atom` item at this time is \"notes\", and its member union `u` uses\n> a `struct notes_option`, we fill some values in `used_atom.u.notes_option`,\n>\n> When we traverse in `used_atom` array in `populate_value()` and previous\n> judgement like \"if (starts_with(name, \"refname\"))\" will failed, because we\n> are dealing with atom \"notes\", but in judgement \"else if\n> (atom->u.remote_ref.push)\",\n> The value we fill in `used_atom.u.notes_option` just makes\n> `used_atom.u.remote_ref.push` non-zero. This leads us into the wrong case.\n>\n> Is this clearer?\n\nThis time you avoided the word enumeration, and it made it clearer.\nThe word commonly used is \"condition\" where you said \"judgment\", I\nthink, and wit that it would probably be even more clear.\n\nused_atom.u is an union, and it has different members depending on\nwhat atom the auxiliary data the union part of the \"struct\nused_atom\" wants to record. At most only one of the members can be\nvalid at any one time. Since the code checks u.remote_ref without\neven making sure if the atom is \"push\" or \"push:\" (which are only\ntwo cases that u.remote_ref.push becomes valid), but u.remote_ref\nshares the same storage for other members of the union, the check\nwas reading from an invalid member, which was the bug.\n\nSo the fix was to see what atom it is by checking its name member?\nIs starts_with() a reliable test? A fictitious atom \"pushe\" will be\ndifferent from \"push\" or \"push:\", but will still pass\nthat test, so from the point of view of future-proofing the tests,\nshouldn't it do the same check as the one at the beginning of\nremote_ref_atom_parser()?\n\nThanks.\n"},{"id":"423766","messageId":"CAOLTT8S9TbRWGPeGKR9=cDnaw2RMukawV48dPEK1KL0X_87udA@mail.gmail.com","threadId":"55625","inReplyTo":"xmqqk0oczao3.fsf@gitster.g","subject":"Re: [PATCH] [GSOC] ref-filter: solve bugs caused by enumeration","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-05-06T10:39:17Z","receivedAt":"2021-05-06T10:39:35Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"> This time you avoided the word enumeration, and it made it clearer.\n> The word commonly used is \"condition\" where you said \"judgment\", I\n> think, and wit that it would probably be even more clear.\n>\n\nOK.\n\n> used_atom.u is an union, and it has different members depending on\n> what atom the auxiliary data the union part of the \"struct\n> used_atom\" wants to record. At most only one of the members can be\n> valid at any one time. Since the code checks u.remote_ref without\n> even making sure if the atom is \"push\" or \"push:\" (which are only\n> two cases that u.remote_ref.push becomes valid), but u.remote_ref\n> shares the same storage for other members of the union, the check\n> was reading from an invalid member, which was the bug.\n>\n\nYes, that's what it means. I got it wrong before, of course used_atom.u\nis not a enum, it's union.\n\n> So the fix was to see what atom it is by checking its name member?\n> Is starts_with() a reliable test? A fictitious atom \"pushe\" will be\n> different from \"push\" or \"push:\", but will still pass\n> that test, so from the point of view of future-proofing the tests,\n> shouldn't it do the same check as the one at the beginning of\n> remote_ref_atom_parser()?\n>\n\nI think it's not necesssary. Before we call `populate_value()`,\n`parse_ref_filter_atom()` only allow user use atom which match\n \"valid_atom\" entry fully, something like \"pushe\" will be rejected\nand process die with \"fatal: unknown field name: pushe\", so it didn't\npass this \"starts_with()\" test.\n\n /* Is the atom a valid one? */\n for (i = 0; i < ARRAY_SIZE(valid_atom); i++) {\n int len = strlen(valid_atom[i].name);\n if (len == atom_len && !memcmp(valid_atom[i].name, sp, len))\n break;\n }\n\n if (ARRAY_SIZE(valid_atom) <= i)\n return strbuf_addf_ret(err, -1, _(\"unknown field name: %.*s\"),\n (int)(ep-atom), atom);\n\n> Thanks.\n\nThanks.\n--\nZheNing Hu\n"},{"id":"423769","messageId":"xmqqtungxg4y.fsf@gitster.g","threadId":"55625","inReplyTo":"CAOLTT8S9TbRWGPeGKR9=cDnaw2RMukawV48dPEK1KL0X_87udA@mail.gmail.com","subject":"Re: [PATCH] [GSOC] ref-filter: solve bugs caused by enumeration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-06T11:20:45Z","receivedAt":"2021-05-06T11:20:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"ZheNing Hu writes:\n\n>> So the fix was to see what atom it is by checking its name member?\n>> Is starts_with() a reliable test? A fictitious atom \"pushe\" will be\n>> different from \"push\" or \"push:\", but will still pass\n>> that test, so from the point of view of future-proofing the tests,\n>> shouldn't it do the same check as the one at the beginning of\n>> remote_ref_atom_parser()?\n>>\n>\n> I think it's not necesssary. Before we call `populate_value()`, ...\n\nI do not care if it is not needed with the \"current\" set of atoms we\naccept. The example \"pushe\", which obviously will not be accepted\nby today's code, was all about future-proofing.\n"},{"id":"423770","messageId":"CAOLTT8TQXAh9vu21d5rvaHe=+eqtFU8MCVhorfGmqYRohDThUg@mail.gmail.com","threadId":"55625","inReplyTo":"xmqqtungxg4y.fsf@gitster.g","subject":"Re: [PATCH] [GSOC] ref-filter: solve bugs caused by enumeration","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-05-06T11:52:03Z","receivedAt":"2021-05-06T11:52:19Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Junio C Hamano 于2021年5月6日周四 下午7:20写道：\n>\n> ZheNing Hu writes:\n>\n> >> So the fix was to see what atom it is by checking its name member?\n> >> Is starts_with() a reliable test? A fictitious atom \"pushe\" will be\n> >> different from \"push\" or \"push:\", but will still pass\n> >> that test, so from the point of view of future-proofing the tests,\n> >> shouldn't it do the same check as the one at the beginning of\n> >> remote_ref_atom_parser()?\n> >>\n> >\n> > I think it's not necesssary. Before we call `populate_value()`, ...\n>\n> I do not care if it is not needed with the \"current\" set of atoms we\n> accept. The example \"pushe\", which obviously will not be accepted\n> by today's code, was all about future-proofing.\n\nWell, what you said makes sense, now I think something like this will be\nmore secure. In the future, other people may be grateful for such a choice. :)\n\n@@ -1730,7 +1759,7 @@ static int populate_value(struct ref_array_item\n*ref, struct strbuf *err)\n else\n v->s = xstrdup(\"\");\n continue;\n- } else if (starts_with(name, \"push\")) {\n+ } else if (starts_with(name, \"push\") &&\natom->u.remote_ref.push) {\n\n\n--\nZheNing Hu\n"},{"id":"423797","messageId":"pull.949.v2.git.1620318676776.gitgitgadget@gmail.com","threadId":"55625","inReplyTo":"pull.949.git.1620228664666.gitgitgadget@gmail.com","subject":"[PATCH v2] [GSOC] ref-filter: fix read invalid union member bug","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-05-06T16:31:16Z","receivedAt":"2021-05-06T16:31:21Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu \n\nused_atom.u is an union, and it has different members depending on\nwhat atom the auxiliary data the union part of the \"struct\nused_atom\" wants to record. At most only one of the members can be\nvalid at any one time. Since the code checks u.remote_ref without\neven making sure if the atom is \"push\" or \"push:\" (which are only\ntwo cases that u.remote_ref.push becomes valid), but u.remote_ref\nshares the same storage for other members of the union, the check\nwas reading from an invalid member, which was the bug.\n\nModify the condition here to first check whether the atom name\nstarts with \"push\", and then check u.remote_ref, to avoid reading\nthe value of invalid member of the union.\n\nHelped-by: Junio C Hamano \nSigned-off-by: ZheNing Hu \n---\n [GSOC] ref-filter: fix read invalid union member bug\n \n Change from last version: Modified the documentation description with\n the help of Junio. And modify the processing method of the condition:\n check whether the name of the atom starts with \"push\" and whether\n u.remote_ref is non-zero.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-949%2Fadlternative%2Fref-filter-enum-bug-fix-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-949/adlternative/ref-filter-enum-bug-fix-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/949\n\nRange-diff vs v1:\n\n 1: e51ca176f76b ! 1: 0e1923c9d722 [GSOC] ref-filter: solve bugs caused by enumeration\n @@ Metadata\n Author: ZheNing Hu \n \n ## Commit message ##\n - [GSOC] ref-filter: solve bugs caused by enumeration\n + [GSOC] ref-filter: fix read invalid union member bug\n \n - Johannes Schindelin seems to have introduced a bug in\n - cc72385f(for-each-ref: let upstream/push optionally\n - report the remote name), it use `atom->u.remote_ref.option`\n - which is a member of enumeration in the judgment statement.\n - When we use other members in the enumeration `used_atom.u`,\n - and it happened to fill in `remote_ref.push`, this judgment\n - may still be established and produces errors. So replace the\n - judgment statement with `starts_with(name, \"push\")` to fix\n - the error.\n + used_atom.u is an union, and it has different members depending on\n + what atom the auxiliary data the union part of the \"struct\n + used_atom\" wants to record. At most only one of the members can be\n + valid at any one time. Since the code checks u.remote_ref without\n + even making sure if the atom is \"push\" or \"push:\" (which are only\n + two cases that u.remote_ref.push becomes valid), but u.remote_ref\n + shares the same storage for other members of the union, the check\n + was reading from an invalid member, which was the bug.\n \n + Modify the condition here to first check whether the atom name\n + starts with \"push\", and then check u.remote_ref, to avoid reading\n + the value of invalid member of the union.\n +\n + Helped-by: Junio C Hamano \n Signed-off-by: ZheNing Hu \n \n ## ref-filter.c ##\n @@ ref-filter.c: static int populate_value(struct ref_array_item *ref, struct strbu\n \t\t\t\tv->s = xstrdup(\"\");\n \t\t\tcontinue;\n -\t\t} else if (atom->u.remote_ref.push) {\n -+\t\t} else if (starts_with(name, \"push\")) {\n ++\t\t} else if (starts_with(name, \"push\") && atom->u.remote_ref.push) {\n \t\t\tconst char *branch_name;\n \t\t\tv->s = xstrdup(\"\");\n \t\t\tif (!skip_prefix(ref->refname, \"refs/heads/\",\n\n\n ref-filter.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex a0adb4551d87..750b25914b82 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1730,7 +1730,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)\n \t\t\telse\n \t\t\t\tv->s = xstrdup(\"\");\n \t\t\tcontinue;\n-\t\t} else if (atom->u.remote_ref.push) {\n+\t\t} else if (starts_with(name, \"push\") && atom->u.remote_ref.push) {\n \t\t\tconst char *branch_name;\n \t\t\tv->s = xstrdup(\"\");\n \t\t\tif (!skip_prefix(ref->refname, \"refs/heads/\",\n\nbase-commit: 311531c9de557d25ac087c1637818bd2aad6eb3a\n-- \ngitgitgadget\n"},{"id":"423822","messageId":"xmqq8s4ry2ye.fsf@gitster.g","threadId":"55625","inReplyTo":"CAOLTT8TQXAh9vu21d5rvaHe=+eqtFU8MCVhorfGmqYRohDThUg@mail.gmail.com","subject":"Re: [PATCH] [GSOC] ref-filter: solve bugs caused by enumeration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-06T21:20:09Z","receivedAt":"2021-05-06T21:20:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"ZheNing Hu writes:\n\n> ... now I think something like this will be more secure. In the\n> future, other people may be grateful for such a choice. :)\n>\n> @@ -1730,7 +1759,7 @@ static int populate_value(struct ref_array_item\n> *ref, struct strbuf *err)\n> else\n> v->s = xstrdup(\"\");\n> continue;\n> - } else if (starts_with(name, \"push\")) {\n> + } else if (starts_with(name, \"push\") &&\n> atom->u.remote_ref.push) {\n\nHmph, I do no think that would be futureproof at all. If a new atom\n\"pushe\" gets added, it is not all that unlikely that it would add\nits own member to the same union. name here would be \"pushe\" and\nstarts with \"push\", and upon parsing \"pushe\", its own member in the\nunion, atom->u.X, would have been assigned to, but the code we see\nhere still accesses atom->u.remote_ref.*, so you still have the same\nproblem you started to solve, no?\n\nThe check we use in remote_ref_atom_parser() to see if the atom is\nabout pushing, i.e.\n\n\tif (!strcmp(atom->name, \"push\") || starts_with(atom->name, \"push:\"))\n\nis unlikely to be invalidated in future changes, as this is very\nmuch end-user facing and changing the condition will break existing\nscripts, so that is what I was expecting you to use instead.\n\n\n\n"},{"id":"423851","messageId":"CAOLTT8QWadP9ovrUC_7m86dKh1PsawUyZ=msK5tx1utTpuCXMQ@mail.gmail.com","threadId":"55625","inReplyTo":"xmqq8s4ry2ye.fsf@gitster.g","subject":"Re: [PATCH] [GSOC] ref-filter: solve bugs caused by enumeration","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-05-07T04:32:25Z","receivedAt":"2021-05-07T04:32:45Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"> Hmph, I do no think that would be futureproof at all. If a new atom\n> \"pushe\" gets added, it is not all that unlikely that it would add\n> its own member to the same union. name here would be \"pushe\" and\n> starts with \"push\", and upon parsing \"pushe\", its own member in the\n> union, atom->u.X, would have been assigned to, but the code we see\n> here still accesses atom->u.remote_ref.*, so you still have the same\n> problem you started to solve, no?\n>\n\nWell, this \"pushe\" has `atom->u.X` which is different from \"push\" and\n\"push:\". This is indeed worth worrying about.\n\n> The check we use in remote_ref_atom_parser() to see if the atom is\n> about pushing, i.e.\n>\n> if (!strcmp(atom->name, \"push\") || starts_with(atom->name, \"push:\"))\n>\n> is unlikely to be invalidated in future changes, as this is very\n> much end-user facing and changing the condition will break existing\n> scripts, so that is what I was expecting you to use instead.\n>\n\nBut I am afraid that the cost we paid for string matching here is too high,\nSince the string matching here is to illustrate that we use one of the members\nof the union, why can't we directly add a \"union_type\" member to used_atom?\nif we have this, we can just switch the \"union_type\" flag and also make the\nprogram correct. Perhaps this would be better than the large number of string\nmatches have done here.\n\n>\n>\n\nThanks.\n--\nZheNing Hu\n"},{"id":"423853","messageId":"xmqq5yzvw3kk.fsf@gitster.g","threadId":"55625","inReplyTo":"CAOLTT8QWadP9ovrUC_7m86dKh1PsawUyZ=msK5tx1utTpuCXMQ@mail.gmail.com","subject":"Re: [PATCH] [GSOC] ref-filter: solve bugs caused by enumeration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-07T04:49:47Z","receivedAt":"2021-05-07T04:49:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"ZheNing Hu writes:\n\n> But I am afraid that the cost we paid for string matching here is too high,\n\nIf that is truly the concern (I do not know without measuring),\nperhaps we should add a member next to the union to say which one of\nthe union members is valid, so that you can say\n\n if (atom->atom_type == ATOM_TYPE_REMOTE_REF &&\n atom->u.remote_ref.push)\n\n(introduce an enum and define ATOM_TYPE_* after the member in the\nunion).\n\nThat would help futureproofing the code even further, as a new\nsynonym of \"push\" introduced laster [*] would not invalidate the check you are\nadding there.\n\n\n[Footnote]\n\n* remote_ref_atom_parser() in the future may begin like so:\n\n-\tif (!strcmp(atom->name, \"push\") || starts_with(atom->name, \"push:\"))\n+\tif (!strcmp(atom->name, \"push\") || starts_with(atom->name, \"push:\") ||\n+ !strcmp(atom->name, \"a-synonym-for-push\"))\n\t\tatom->u.remote_ref.push = 1;\n\n"},{"id":"423856","messageId":"CAOLTT8RhVZQJX8z1gY5UM1jv0imZ4K9UnD14MgJFfvqBBiAZQg@mail.gmail.com","threadId":"55625","inReplyTo":"xmqq5yzvw3kk.fsf@gitster.g","subject":"Re: [PATCH] [GSOC] ref-filter: solve bugs caused by enumeration","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-05-07T05:09:19Z","receivedAt":"2021-05-07T05:09:38Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"> If that is truly the concern (I do not know without measuring),\n> perhaps we should add a member next to the union to say which one of\n> the union members is valid, so that you can say\n>\n> if (atom->atom_type == ATOM_TYPE_REMOTE_REF &&\n> atom->u.remote_ref.push)\n>\n> (introduce an enum and define ATOM_TYPE_* after the member in the\n> union).\n>\n\nYes, I think so. Since the content of this part needs to be modified for\n the parsing of all atoms, I will put it in a separate topic to complete.\n\n> That would help futureproofing the code even further, as a new\n> synonym of \"push\" introduced laster [*] would not invalidate the check you are\n> adding there.\n>\n\nYes, this enhances its generalization ability.\n\n>\n> [Footnote]\n>\n> * remote_ref_atom_parser() in the future may begin like so:\n>\n> - if (!strcmp(atom->name, \"push\") || starts_with(atom->name, \"push:\"))\n> + if (!strcmp(atom->name, \"push\") || starts_with(atom->name, \"push:\") ||\n> + !strcmp(atom->name, \"a-synonym-for-push\"))\n> atom->u.remote_ref.push = 1;\n>\n\n--\nZheNing Hu\n"},{"id":"423934","messageId":"pull.949.v3.git.1620487572222.gitgitgadget@gmail.com","threadId":"55625","inReplyTo":"pull.949.v2.git.1620318676776.gitgitgadget@gmail.com","subject":"[PATCH v3] [GSOC] ref-filter: fix read invalid union member bug","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-05-08T15:26:11Z","receivedAt":"2021-05-08T15:26:17Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu \n\nused_atom.u is an union, and it has different members depending on\nwhat atom the auxiliary data the union part of the \"struct\nused_atom\" wants to record. At most only one of the members can be\nvalid at any one time. Since the code checks u.remote_ref without\neven making sure if the atom is \"push\" or \"push:\" (which are only\ntwo cases that u.remote_ref.push becomes valid), but u.remote_ref\nshares the same storage for other members of the union, the check\nwas reading from an invalid member, which was the bug.\n\nModify the condition here to check whether the atom name\nequals to \"push\" or starts with \"push:\", to avoid reading the\nvalue of invalid member of the union.\n\nHelped-by: Junio C Hamano \nSigned-off-by: ZheNing Hu \n---\n [GSOC] ref-filter: fix read invalid union member bug\n \n Change from last version:\n Modify the processing method of the condition: check whether the name of\n the atom equals to \"push\" or starts with \"pushs\", which can enhanced\n security, although it may bring string match overhead.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-949%2Fadlternative%2Fref-filter-enum-bug-fix-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-949/adlternative/ref-filter-enum-bug-fix-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/949\n\nRange-diff vs v2:\n\n 1: 0e1923c9d722 ! 1: 21cf7a44e168 [GSOC] ref-filter: fix read invalid union member bug\n @@ Commit message\n \n used_atom.u is an union, and it has different members depending on\n what atom the auxiliary data the union part of the \"struct\n - used_atom\" wants to record. At most only one of the members can be\n - valid at any one time. Since the code checks u.remote_ref without\n + used_atom\" wants to record. At most only one of the members can be\n + valid at any one time. Since the code checks u.remote_ref without\n even making sure if the atom is \"push\" or \"push:\" (which are only\n two cases that u.remote_ref.push becomes valid), but u.remote_ref\n shares the same storage for other members of the union, the check\n was reading from an invalid member, which was the bug.\n \n - Modify the condition here to first check whether the atom name\n - starts with \"push\", and then check u.remote_ref, to avoid reading\n - the value of invalid member of the union.\n + Modify the condition here to check whether the atom name\n + equals to \"push\" or starts with \"push:\", to avoid reading the\n + value of invalid member of the union.\n \n Helped-by: Junio C Hamano \n Signed-off-by: ZheNing Hu \n @@ ref-filter.c: static int populate_value(struct ref_array_item *ref, struct strbu\n \t\t\t\tv->s = xstrdup(\"\");\n \t\t\tcontinue;\n -\t\t} else if (atom->u.remote_ref.push) {\n -+\t\t} else if (starts_with(name, \"push\") && atom->u.remote_ref.push) {\n ++\t\t} else if (!strcmp(atom->name, \"push\") || starts_with(atom->name, \"push:\")) {\n \t\t\tconst char *branch_name;\n \t\t\tv->s = xstrdup(\"\");\n \t\t\tif (!skip_prefix(ref->refname, \"refs/heads/\",\n\n\n ref-filter.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex a0adb4551d87..213d3773ada3 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1730,7 +1730,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)\n \t\t\telse\n \t\t\t\tv->s = xstrdup(\"\");\n \t\t\tcontinue;\n-\t\t} else if (atom->u.remote_ref.push) {\n+\t\t} else if (!strcmp(atom->name, \"push\") || starts_with(atom->name, \"push:\")) {\n \t\t\tconst char *branch_name;\n \t\t\tv->s = xstrdup(\"\");\n \t\t\tif (!skip_prefix(ref->refname, \"refs/heads/\",\n\nbase-commit: 311531c9de557d25ac087c1637818bd2aad6eb3a\n-- \ngitgitgadget\n"},{"id":"424007","messageId":"xmqqh7jbqck1.fsf@gitster.g","threadId":"55625","inReplyTo":"pull.949.v3.git.1620487572222.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] [GSOC] ref-filter: fix read invalid union member bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-10T07:21:18Z","receivedAt":"2021-05-10T07:21:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"ZheNing Hu via GitGitGadget\" writes:\n\n> From: ZheNing Hu \n>\n> used_atom.u is an union, and it has different members depending on\n> what atom the auxiliary data the union part of the \"struct\n> used_atom\" wants to record. At most only one of the members can be\n> valid at any one time. Since the code checks u.remote_ref without\n> even making sure if the atom is \"push\" or \"push:\" (which are only\n> two cases that u.remote_ref.push becomes valid), but u.remote_ref\n> shares the same storage for other members of the union, the check\n> was reading from an invalid member, which was the bug.\n>\n> Modify the condition here to check whether the atom name\n> equals to \"push\" or starts with \"push:\", to avoid reading the\n> value of invalid member of the union.\n>\n> Helped-by: Junio C Hamano \n> Signed-off-by: ZheNing Hu \n> ---\n> [GSOC] ref-filter: fix read invalid union member bug\n> \n> Change from last version:\n> Modify the processing method of the condition: check whether the name of\n> the atom equals to \"push\" or starts with \"pushs\", which can enhanced\n> security, although it may bring string match overhead.\n\nI do not think this would have much security implication either\nway. What it buys us is the future-proofing.\n\nI think it is OK to make this change without the enum thing to have\nit graduate early as a fix to the existing code. The enum thing can\ncome on top.\n\nWill queue. Thanks.\n"},{"id":"424008","messageId":"xmqqcztzqc9j.fsf@gitster.g","threadId":"55625","inReplyTo":"pull.949.v3.git.1620487572222.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] [GSOC] ref-filter: fix read invalid union member bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-10T07:27:36Z","receivedAt":"2021-05-10T07:27:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"ZheNing Hu via GitGitGadget\" writes:\n\n> From: ZheNing Hu \n>\n> used_atom.u is an union, and it has different members depending on\n> what atom the auxiliary data the union part of the \"struct\n> used_atom\" wants to record. At most only one of the members can be\n> valid at any one time. Since the code checks u.remote_ref without\n> even making sure if the atom is \"push\" or \"push:\" (which are only\n> two cases that u.remote_ref.push becomes valid), but u.remote_ref\n> shares the same storage for other members of the union, the check\n> was reading from an invalid member, which was the bug.\n>\n> Modify the condition here to check whether the atom name\n> equals to \"push\" or starts with \"push:\", to avoid reading the\n> value of invalid member of the union.\n>\n> Helped-by: Junio C Hamano \n> Signed-off-by: ZheNing Hu \n> ---\n\nJust a final sanity check. Is this a recent breakage or was the\ncode introduced at cc72385f (for-each-ref: let upstream/push\noptionally report the remote name, 2017-10-05) broken from the\nbeginning?\n\nI am wondering if it is easy to add a test to cover the codepath\nthat is affected by this change.\n\nThanks.\n\n> diff --git a/ref-filter.c b/ref-filter.c\n> index a0adb4551d87..213d3773ada3 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -1730,7 +1730,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)\n> \t\t\telse\n> \t\t\t\tv->s = xstrdup(\"\");\n> \t\t\tcontinue;\n> -\t\t} else if (atom->u.remote_ref.push) {\n> +\t\t} else if (!strcmp(atom->name, \"push\") || starts_with(atom->name, \"push:\")) {\n> \t\t\tconst char *branch_name;\n> \t\t\tv->s = xstrdup(\"\");\n> \t\t\tif (!skip_prefix(ref->refname, \"refs/heads/\",\n>\n> base-commit: 311531c9de557d25ac087c1637818bd2aad6eb3a\n"},{"id":"424028","messageId":"CAOLTT8T3yqbM8Ucey6Jfs2U75AP33B4d8W_Ab7ZYDQQDZX-4CQ@mail.gmail.com","threadId":"55625","inReplyTo":"xmqqh7jbqck1.fsf@gitster.g","subject":"Re: [PATCH v3] [GSOC] ref-filter: fix read invalid union member bug","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-05-10T12:35:01Z","receivedAt":"2021-05-10T12:52:22Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Junio C Hamano 于2021年5月10日周一 下午3:21写道：\n>\n> \"ZheNing Hu via GitGitGadget\" writes:\n>\n> > From: ZheNing Hu \n> >\n> > used_atom.u is an union, and it has different members depending on\n> > what atom the auxiliary data the union part of the \"struct\n> > used_atom\" wants to record. At most only one of the members can be\n> > valid at any one time. Since the code checks u.remote_ref without\n> > even making sure if the atom is \"push\" or \"push:\" (which are only\n> > two cases that u.remote_ref.push becomes valid), but u.remote_ref\n> > shares the same storage for other members of the union, the check\n> > was reading from an invalid member, which was the bug.\n> >\n> > Modify the condition here to check whether the atom name\n> > equals to \"push\" or starts with \"push:\", to avoid reading the\n> > value of invalid member of the union.\n> >\n> > Helped-by: Junio C Hamano \n> > Signed-off-by: ZheNing Hu \n> > ---\n> > [GSOC] ref-filter: fix read invalid union member bug\n> >\n> > Change from last version:\n> > Modify the processing method of the condition: check whether the name of\n> > the atom equals to \"push\" or starts with \"pushs\", which can enhanced\n> > security, although it may bring string match overhead.\n>\n> I do not think this would have much security implication either\n> way. What it buys us is the future-proofing.\n>\n\nAh, truely.\n\n> I think it is OK to make this change without the enum thing to have\n> it graduate early as a fix to the existing code. The enum thing can\n> come on top.\n>\n\nIndeed. \"enum atom_type\" is for ref-filter performance optimization and get\nsome other benefits like quick index. So I put it in another topic.\n\n> Will queue. Thanks.\n\nThanks.\n--\nZheNing Hu\n"},{"id":"424029","messageId":"CAOLTT8R3ys5ohujVRKiQa3vijrzdGzLThU5hsG-estiyUgOqhQ@mail.gmail.com","threadId":"55625","inReplyTo":"xmqqcztzqc9j.fsf@gitster.g","subject":"Re: [PATCH v3] [GSOC] ref-filter: fix read invalid union member bug","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-05-10T12:51:10Z","receivedAt":"2021-05-10T13:41:06Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"> Just a final sanity check. Is this a recent breakage or was the\n> code introduced at cc72385f (for-each-ref: let upstream/push\n> optionally report the remote name, 2017-10-05) broken from the\n> beginning?\n>\n\nWell, The trigger condition is very special, but the bug was introduced\nat that time. Let's see the \"bug\" example below.\n\n> I am wondering if it is easy to add a test to cover the codepath\n> that is affected by this change.\n>\n> Thanks.\n>\n\nWell, because this bug must require that the seventeenth bit of\n`used_atom.u` is not 0, it took me a long time to find this bug.\nin `used_atom.u`, only the member \"color\" and \"contents\" which\nsize is bigger than 17 bytes, but \"%(contents:trailer:only)\" only fill\nthe 16th byte of `used_atom.u`.\n\n\"Fortunately\", I found it.\n\ngit for-each-ref --format='%(color:#aa22ac)'\n\nI will add test for it!\n\nThanks!\n--\nZheNing Hu\n"},{"id":"424044","messageId":"pull.949.v4.git.1620658904283.gitgitgadget@gmail.com","threadId":"55625","inReplyTo":"pull.949.v3.git.1620487572222.gitgitgadget@gmail.com","subject":"[PATCH v4] [GSOC] ref-filter: fix read invalid union member bug","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-05-10T15:01:43Z","receivedAt":"2021-05-10T15:24:15Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu \n\nused_atom.u is an union, and it has different members depending on\nwhat atom the auxiliary data the union part of the \"struct\nused_atom\" wants to record. At most only one of the members can be\nvalid at any one time. Since the code checks u.remote_ref without\neven making sure if the atom is \"push\" or \"push:\" (which are only\ntwo cases that u.remote_ref.push becomes valid), but u.remote_ref\nshares the same storage for other members of the union, the check\nwas reading from an invalid member, which was the bug.\n\nModify the condition here to check whether the atom name\nequals to \"push\" or starts with \"push:\", to avoid reading the\nvalue of invalid member of the union.\n\nHelped-by: Junio C Hamano \nSigned-off-by: ZheNing Hu \n---\n [GSOC] ref-filter: fix read invalid union member bug\n \n Change from last version:\n Prove that the bug may appear when using %(color) atom. And add\n corresponding test for it.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-949%2Fadlternative%2Fref-filter-enum-bug-fix-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-949/adlternative/ref-filter-enum-bug-fix-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/949\n\nRange-diff vs v3:\n\n 1: 21cf7a44e168 ! 1: 8c6c0368a590 [GSOC] ref-filter: fix read invalid union member bug\n @@ ref-filter.c: static int populate_value(struct ref_array_item *ref, struct strbu\n \t\t\tconst char *branch_name;\n \t\t\tv->s = xstrdup(\"\");\n \t\t\tif (!skip_prefix(ref->refname, \"refs/heads/\",\n +\n + ## t/t6302-for-each-ref-filter.sh ##\n +@@ t/t6302-for-each-ref-filter.sh: test_expect_success '%(color) must fail' '\n + \ttest_must_fail git for-each-ref --format=\"%(color)%(refname)\"\n + '\n + \n ++test_expect_success '%(color:#aa22ac) must success' '\n ++\tcat >expect <<-\\EOF &&\n ++\trefs/heads/main\n ++\trefs/heads/side\n ++\trefs/odd/spot\n ++\trefs/tags/annotated-tag\n ++\trefs/tags/doubly-annotated-tag\n ++\trefs/tags/doubly-signed-tag\n ++\trefs/tags/four\n ++\trefs/tags/one\n ++\trefs/tags/signed-tag\n ++\trefs/tags/three\n ++\trefs/tags/two\n ++\tEOF\n ++\tgit for-each-ref --format=\"%(color:#aa22ac)%(refname)\" >actual &&\n ++\ttest_cmp expect actual\n ++'\n ++\n + test_expect_success 'left alignment is default' '\n + \tcat >expect <<-\\EOF &&\n + \trefname is refs/heads/main |refs/heads/main\n\n\n ref-filter.c | 2 +-\n t/t6302-for-each-ref-filter.sh | 18 ++++++++++++++++++\n 2 files changed, 19 insertions(+), 1 deletion(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex a0adb4551d87..213d3773ada3 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1730,7 +1730,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)\n \t\t\telse\n \t\t\t\tv->s = xstrdup(\"\");\n \t\t\tcontinue;\n-\t\t} else if (atom->u.remote_ref.push) {\n+\t\t} else if (!strcmp(atom->name, \"push\") || starts_with(atom->name, \"push:\")) {\n \t\t\tconst char *branch_name;\n \t\t\tv->s = xstrdup(\"\");\n \t\t\tif (!skip_prefix(ref->refname, \"refs/heads/\",\ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex 9866b1b57368..38a7d83830aa 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -117,6 +117,24 @@ test_expect_success '%(color) must fail' '\n \ttest_must_fail git for-each-ref --format=\"%(color)%(refname)\"\n '\n \n+test_expect_success '%(color:#aa22ac) must success' '\n+\tcat >expect <<-\\EOF &&\n+\trefs/heads/main\n+\trefs/heads/side\n+\trefs/odd/spot\n+\trefs/tags/annotated-tag\n+\trefs/tags/doubly-annotated-tag\n+\trefs/tags/doubly-signed-tag\n+\trefs/tags/four\n+\trefs/tags/one\n+\trefs/tags/signed-tag\n+\trefs/tags/three\n+\trefs/tags/two\n+\tEOF\n+\tgit for-each-ref --format=\"%(color:#aa22ac)%(refname)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'left alignment is default' '\n \tcat >expect <<-\\EOF &&\n \trefname is refs/heads/main |refs/heads/main\n\nbase-commit: 311531c9de557d25ac087c1637818bd2aad6eb3a\n-- \ngitgitgadget\n"},{"id":"424091","messageId":"xmqqcztyovf7.fsf@gitster.g","threadId":"55625","inReplyTo":"pull.949.v4.git.1620658904283.gitgitgadget@gmail.com","subject":"Re: [PATCH v4] [GSOC] ref-filter: fix read invalid union member bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-11T02:29:00Z","receivedAt":"2021-05-11T02:29:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"ZheNing Hu via GitGitGadget\" writes:\n\n> Change from last version:\n> Prove that the bug may appear when using %(color) atom. And add\n> corresponding test for it.\n\n> ++test_expect_success '%(color:#aa22ac) must success' '\n\ns/success/succeed/\n\nBut more importantly, I am not sure how this is supposed to\ndemonstrate existing breakage around the %(push). Did you mean to\nuse %(push) instead of %(refname) or something?\n\n\n> ++\tcat >expect <<-\\EOF &&\n> ++\trefs/heads/main\n> ++\trefs/heads/side\n> ++\trefs/odd/spot\n> ++\trefs/tags/annotated-tag\n> ++\trefs/tags/doubly-annotated-tag\n> ++\trefs/tags/doubly-signed-tag\n> ++\trefs/tags/four\n> ++\trefs/tags/one\n> ++\trefs/tags/signed-tag\n> ++\trefs/tags/three\n> ++\trefs/tags/two\n> ++\tEOF\n> ++\tgit for-each-ref --format=\"%(color:#aa22ac)%(refname)\" >actual &&\n> ++\ttest_cmp expect actual\n> ++'\n> ++\n> + test_expect_success 'left alignment is default' '\n> + \tcat >expect <<-\\EOF &&\n> + \trefname is refs/heads/main |refs/heads/main\n>\n>\n> ref-filter.c | 2 +-\n> t/t6302-for-each-ref-filter.sh | 18 ++++++++++++++++++\n> 2 files changed, 19 insertions(+), 1 deletion(-)\n>\n> diff --git a/ref-filter.c b/ref-filter.c\n> index a0adb4551d87..213d3773ada3 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -1730,7 +1730,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)\n> \t\t\telse\n> \t\t\t\tv->s = xstrdup(\"\");\n> \t\t\tcontinue;\n> -\t\t} else if (atom->u.remote_ref.push) {\n> +\t\t} else if (!strcmp(atom->name, \"push\") || starts_with(atom->name, \"push:\")) {\n> \t\t\tconst char *branch_name;\n> \t\t\tv->s = xstrdup(\"\");\n> \t\t\tif (!skip_prefix(ref->refname, \"refs/heads/\",\n> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\n> index 9866b1b57368..38a7d83830aa 100755\n> --- a/t/t6302-for-each-ref-filter.sh\n> +++ b/t/t6302-for-each-ref-filter.sh\n> @@ -117,6 +117,24 @@ test_expect_success '%(color) must fail' '\n> \ttest_must_fail git for-each-ref --format=\"%(color)%(refname)\"\n> '\n> \n> +test_expect_success '%(color:#aa22ac) must success' '\n> +\tcat >expect <<-\\EOF &&\n> +\trefs/heads/main\n> +\trefs/heads/side\n> +\trefs/odd/spot\n> +\trefs/tags/annotated-tag\n> +\trefs/tags/doubly-annotated-tag\n> +\trefs/tags/doubly-signed-tag\n> +\trefs/tags/four\n> +\trefs/tags/one\n> +\trefs/tags/signed-tag\n> +\trefs/tags/three\n> +\trefs/tags/two\n> +\tEOF\n> +\tgit for-each-ref --format=\"%(color:#aa22ac)%(refname)\" >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_expect_success 'left alignment is default' '\n> \tcat >expect <<-\\EOF &&\n> \trefname is refs/heads/main |refs/heads/main\n>\n> base-commit: 311531c9de557d25ac087c1637818bd2aad6eb3a\n"},{"id":"424110","messageId":"CAOLTT8QR4Ga41ADKhgB4=C7VgGbJEe5G5HSbcjRb51H2yQVRPA@mail.gmail.com","threadId":"55625","inReplyTo":"xmqqcztyovf7.fsf@gitster.g","subject":"Re: [PATCH v4] [GSOC] ref-filter: fix read invalid union member bug","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-05-11T06:28:32Z","receivedAt":"2021-05-11T06:28:53Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Junio C Hamano 于2021年5月11日周二 上午10:29写道：\n>\n> \"ZheNing Hu via GitGitGadget\" writes:\n>\n> > Change from last version:\n> > Prove that the bug may appear when using %(color) atom. And add\n> > corresponding test for it.\n>\n> > ++test_expect_success '%(color:#aa22ac) must success' '\n>\n> s/success/succeed/\n>\n> But more importantly, I am not sure how this is supposed to\n> demonstrate existing breakage around the %(push). Did you mean to\n> use %(push) instead of %(refname) or something?\n>\n\nAh, I don’t think this scene of damage is related to using %(push)\nor %(refname).\n\nWe are just in the process of using `populate_value()`, if the atom we\nspecify meets the following conditions, then the condition of\natom->u.remote_ref.push!=0 will be established.\n\n1. The atom that triggers the bug , its \"if\" condition order must after\n\"if (atom->u.remote_ref.push)\", such as %(refname) or %(worktreepath),\nthey can be executed correctly because their order is before \"push\".\n\n2. The member size in used_atom.u corresponding to the atom must\nlarger than 17 bytes, because the offset of \"u.remote_ref.push\" in\n\"u.remote_ref\" is 17, the satisfied atoms are only \"%(color)\" and\n \"%(contents)\", their corresponding members are u.color and u.contents.\n\n3. We happen to be able to fill in the 17th position of these structures,\n which makes atom->u.remote_ref.push not equal to 0 established.\n\nSo this kind of bug is not related to %(push), an atom that satisfies\nthe above conditions will make `if (atom->u.remote_ref.push)` be true.\nthen execute the program logic related to `%(push)`.\n\nNow, we only have `%(color)` can trigger it \"sometime\",\nIt is unpredictable to fill in the 17th byte of used_atom.u.color,\nso we cannot track all the atoms related to this bug.\n\ngit for-each-ref --format=\"%(color:#aa22ac)%(refname)\"\ngit for-each-ref --format=\"%(color:#aa22ad)%(refname)\"\n\nwill trigger the bug.\n\ngit for-each-ref --format=\"%(color:#aa22ae)%(refname)\"\ngit for-each-ref --format=\"%(color:#aa22af)%(refname)\"\n\nwill not trigger the bug.\n\nIn other words, we cannot use a perfect test set to cover all broken.\nSo now `%(color:#aa22ac)` is enough for explain the problem of this bug.\n\nThanks.\n--\nZheNing Hu\n"},{"id":"424119","messageId":"xmqqlf8lmxca.fsf@gitster.g","threadId":"55625","inReplyTo":"CAOLTT8QR4Ga41ADKhgB4=C7VgGbJEe5G5HSbcjRb51H2yQVRPA@mail.gmail.com","subject":"Re: [PATCH v4] [GSOC] ref-filter: fix read invalid union member bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-11T09:30:29Z","receivedAt":"2021-05-11T09:30:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"ZheNing Hu writes:\n\n> We are just in the process of using `populate_value()`, if the atom we\n> specify meets the following conditions, then the condition of\n> atom->u.remote_ref.push!=0 will be established.\n>\n> 1. The atom that triggers the bug , its \"if\" condition order must after\n> \"if (atom->u.remote_ref.push)\", such as %(refname) or %(worktreepath),\n> they can be executed correctly because their order is before \"push\".\n>\n> 2. The member size in used_atom.u corresponding to the atom must\n> larger than 17 bytes, because the offset of \"u.remote_ref.push\" in\n> \"u.remote_ref\" is 17, the satisfied atoms are only \"%(color)\" and\n> \"%(contents)\", their corresponding members are u.color and u.contents.\n>\n> 3. We happen to be able to fill in the 17th position of these structures,\n> which makes atom->u.remote_ref.push not equal to 0 established.\n>\n> So this kind of bug is not related to %(push), an atom that satisfies\n> the above conditions will make `if (atom->u.remote_ref.push)` be true.\n> then execute the program logic related to `%(push)`.\n>\n> Now, we only have `%(color)` can trigger it \"sometime\",\n> It is unpredictable to fill in the 17th byte of used_atom.u.color,\n> so we cannot track all the atoms related to this bug.\n>\n> git for-each-ref --format=\"%(color:#aa22ac)%(refname)\"\n> git for-each-ref --format=\"%(color:#aa22ad)%(refname)\"\n>\n> will trigger the bug.\n>\n> git for-each-ref --format=\"%(color:#aa22ae)%(refname)\"\n> git for-each-ref --format=\"%(color:#aa22af)%(refname)\"\n>\n> will not trigger the bug.\n>\n> In other words, we cannot use a perfect test set to cover all broken.\n> So now `%(color:#aa22ac)` is enough for explain the problem of this bug.\n\nWell, the thing is,\n\n $ git checkout 49f38e2d ;# (The fifteenth batch, 2021-05-10)\n $ git am -s mbox\n $ git show --stat --oneline\n 39509d100a (HEAD) ref-filter: fix read invalid union member bug\n ref-filter.c | 2 +-\n t/t6302-for-each-ref-filter.sh | 18 ++++++++++++++++++\n 2 files changed, 19 insertions(+), 1 deletion(-)\n $ git show ref-filter.c | git apply -R ;# revert only the fix\n $ make -j32 && make -C t T=t6302-*.sh\n\ndoes not seem to break anything. Perhaps there is something more\nthan the \"17th byte\" thing (like structure padding that may vary\ndepending on the compiler and architecture)?\n"},{"id":"424122","messageId":"CAOLTT8SiEVR62LwuagnpbZ4XxiUMe2M2WPjaorzSJmfERD6BMw@mail.gmail.com","threadId":"55625","inReplyTo":"xmqqlf8lmxca.fsf@gitster.g","subject":"Re: [PATCH v4] [GSOC] ref-filter: fix read invalid union member bug","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-05-11T11:47:01Z","receivedAt":"2021-05-11T11:47:14Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"> Well, the thing is,\n>\n> $ git checkout 49f38e2d ;# (The fifteenth batch, 2021-05-10)\n> $ git am -s mbox\n> $ git show --stat --oneline\n> 39509d100a (HEAD) ref-filter: fix read invalid union member bug\n> ref-filter.c | 2 +-\n> t/t6302-for-each-ref-filter.sh | 18 ++++++++++++++++++\n> 2 files changed, 19 insertions(+), 1 deletion(-)\n> $ git show ref-filter.c | git apply -R ;# revert only the fix\n> $ make -j32 && make -C t T=t6302-*.sh\n>\n> does not seem to break anything. Perhaps there is something more\n> than the \"17th byte\" thing (like structure padding that may vary\n> depending on the compiler and architecture)?\n\nFine, I guess the reason for this mystery is I \"push\" this branch to github\nand you haven't done it. That may not be due to the platform. Because I\ncan see no this bug happening when I use a new git repo without \"git push\",\nand I test in archlinux or deepin, this bug will happen in these environments.\n\nI take back what I said before, maybe this is really related to push.\n\nLet's see what happens:\n\n#!/bin/sh\nmkdir test\ncd test\ngit init\necho 1>1\ngit add .\ngit branch -M main\ngit commit -m \"test\"\ngit remote add origin nowhere\ngit config branch.main.remote origin\ngit config branch.main.merge refs/heads/main\ngit for-each-ref --format=\"%(color:#aa22ac)%(objectname)\"\n\nThese two \"git config\" is for simulating a push environment.\n\nI guess you also saw this bug:\n\nBUG: ref-filter.c:1544: unhandled RR_* enum\n\nI will use it for the test.\n\nThanks.\n--\nZheNing Hu\n"},{"id":"424129","messageId":"xmqq7dk5mn2s.fsf@gitster.g","threadId":"55625","inReplyTo":"CAOLTT8SiEVR62LwuagnpbZ4XxiUMe2M2WPjaorzSJmfERD6BMw@mail.gmail.com","subject":"Re: [PATCH v4] [GSOC] ref-filter: fix read invalid union member bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-11T13:12:11Z","receivedAt":"2021-05-11T13:12:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"ZheNing Hu writes:\n\n>> Well, the thing is,\n>>\n>> $ git checkout 49f38e2d ;# (The fifteenth batch, 2021-05-10)\n>> $ git am -s mbox\n>> $ git show --stat --oneline\n>> 39509d100a (HEAD) ref-filter: fix read invalid union member bug\n>> ref-filter.c | 2 +-\n>> t/t6302-for-each-ref-filter.sh | 18 ++++++++++++++++++\n>> 2 files changed, 19 insertions(+), 1 deletion(-)\n>> $ git show ref-filter.c | git apply -R ;# revert only the fix\n>> $ make -j32 && make -C t T=t6302-*.sh\n>>\n>> does not seem to break anything. Perhaps there is something more\n>> than the \"17th byte\" thing (like structure padding that may vary\n>> depending on the compiler and architecture)?\n>\n> Fine, I guess the reason for this mystery is I \"push\" this branch to github\n> and you haven't done it. That may not be due to the platform. Because I\n> can see no this bug happening when I use a new git repo without \"git push\",\n> and I test in archlinux or deepin, this bug will happen in these environments.\n\nSorry, you lost me. I was talking about what happens in the new\ntest you added to t6302 not failing as designed, and there shouldn't\nbe \"I've pushed but you haven't pushed to GitHub\" distinction. The\ntest is running in a brand-new repository just created for the sole\npurpose of running the test after all.\n\n> #!/bin/sh\n> mkdir test\n> cd test\n> git init\n> echo 1>1\n> git add .\n> git branch -M main\n> git commit -m \"test\"\n> git remote add origin nowhere\n> git config branch.main.remote origin\n> git config branch.main.merge refs/heads/main\n> git for-each-ref --format=\"%(color:#aa22ac)%(objectname)\"\n>\n> These two \"git config\" is for simulating a push environment.\n\nSo in short, the test script added to t6302 in the v4 patch was not\ntesting what it was supposed to be testing, as it didn't have the\nconfiguration items related to %(push) atom necessary to trigger the\nerror? \n\nThat I can believe. I was starting to worry if there was something\nmore subtle going on, but I am glad that it was only an uncooked\npatch submitted without checking.\n\n> I guess you also saw this bug:\n>\n> BUG: ref-filter.c:1544: unhandled RR_* enum\n\nNo, I didn't. I just tried to make sure the new test was truly\nchecking the existing breakage by partially reverting the code fix,\nand saw that the new test did not fail.\n\nThanks.\n"},{"id":"424131","messageId":"CAOLTT8RTtKiCX1EfrMAVdWWxRbpF2nW3ri1NSVQ-tqmpdDUOvw@mail.gmail.com","threadId":"55625","inReplyTo":"xmqq7dk5mn2s.fsf@gitster.g","subject":"Re: [PATCH v4] [GSOC] ref-filter: fix read invalid union member bug","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-05-11T13:31:02Z","receivedAt":"2021-05-11T13:31:23Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"> > Fine, I guess the reason for this mystery is I \"push\" this branch to github\n> > and you haven't done it. That may not be due to the platform. Because I\n> > can see no this bug happening when I use a new git repo without \"git push\",\n> > and I test in archlinux or deepin, this bug will happen in these environments.\n>\n> Sorry, you lost me. I was talking about what happens in the new\n> test you added to t6302 not failing as designed, and there shouldn't\n> be \"I've pushed but you haven't pushed to GitHub\" distinction. The\n> test is running in a brand-new repository just created for the sole\n> purpose of running the test after all.\n>\n\nWell, there are some expression problems here, I just want to express:\nThis bug is only triggered after I push and Git adds some config during\nthe push process. And then those config effort %(push) behavior.\n\n> So in short, the test script added to t6302 in the v4 patch was not\n> testing what it was supposed to be testing, as it didn't have the\n> configuration items related to %(push) atom necessary to trigger the\n> error?\n>\n\nTruly.\n\n> That I can believe. I was starting to worry if there was something\n> more subtle going on, but I am glad that it was only an uncooked\n> patch submitted without checking.\n>\n\nMe too, I want to complain a little bit: this bug is too difficult to locate.\n\n> > I guess you also saw this bug:\n> >\n> > BUG: ref-filter.c:1544: unhandled RR_* enum\n>\n> No, I didn't. I just tried to make sure the new test was truly\n> checking the existing breakage by partially reverting the code fix,\n> and saw that the new test did not fail.\n>\n\nI understand.\n\n> Thanks.\n\nThanks.\n--\nZheNing Hu\n"},{"id":"424139","messageId":"pull.949.v5.git.1620747320947.gitgitgadget@gmail.com","threadId":"55625","inReplyTo":"pull.949.v4.git.1620658904283.gitgitgadget@gmail.com","subject":"[PATCH v5] [GSOC] ref-filter: fix read invalid union member bug","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-05-11T15:35:20Z","receivedAt":"2021-05-11T15:35:28Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu \n\nused_atom.u is an union, and it has different members depending on\nwhat atom the auxiliary data the union part of the \"struct\nused_atom\" wants to record. At most only one of the members can be\nvalid at any one time. Since the code checks u.remote_ref without\neven making sure if the atom is \"push\" or \"push:\" (which are only\ntwo cases that u.remote_ref.push becomes valid), but u.remote_ref\nshares the same storage for other members of the union, the check\nwas reading from an invalid member, which was the bug.\n\nModify the condition here to check whether the atom name\nequals to \"push\" or starts with \"push:\", to avoid reading the\nvalue of invalid member of the union.\n\nHelped-by: Junio C Hamano \nSigned-off-by: ZheNing Hu \n---\n [GSOC] ref-filter: fix read invalid union member bug\n \n I and Junio discussed the situation that this bug might actually occur.\n \n The damage that can be found currently is using %(colors:#aa22ac) or\n some other %(colors) atoms. But Junio found that testing\n %(colors:#aa22ac) alone did not show the expected bug in the commit\n before the repair.\n \n So I conducted an experiment:\n \n When we use git push, Git will add some config, these configurations\n will affect the result of the execution process related to atom %(push)\n in populate_value().\n \n Change from last version: added a new test, which added two\n configurations:\n \n git config branch.main.remote origin git config branch.main.merge\n refs/heads/main\n \n used to simulate the configuration changes brought by git push.\n \n Finally, a test on the broken atom %(colors:#aa22ac). In the commit\n before the repair, breakage occurs. In the commit after the repair,\n breakage disappeared.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-949%2Fadlternative%2Fref-filter-enum-bug-fix-v5\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-949/adlternative/ref-filter-enum-bug-fix-v5\nPull-Request: https://github.com/gitgitgadget/git/pull/949\n\nRange-diff vs v4:\n\n 1: 8c6c0368a590 ! 1: b546477e8c87 [GSOC] ref-filter: fix read invalid union member bug\n @@ t/t6302-for-each-ref-filter.sh: test_expect_success '%(color) must fail' '\n \ttest_must_fail git for-each-ref --format=\"%(color)%(refname)\"\n '\n \n -+test_expect_success '%(color:#aa22ac) must success' '\n ++test_expect_success '%(color:#aa22ac) must successed' '\n ++\ttest_when_finished \"cd .. && rm -rf ./test\" &&\n ++\tmkdir test &&\n ++\tcd test &&\n ++\tgit init &&\n +\tcat >expect <<-\\EOF &&\n +\trefs/heads/main\n -+\trefs/heads/side\n -+\trefs/odd/spot\n -+\trefs/tags/annotated-tag\n -+\trefs/tags/doubly-annotated-tag\n -+\trefs/tags/doubly-signed-tag\n -+\trefs/tags/four\n -+\trefs/tags/one\n -+\trefs/tags/signed-tag\n -+\trefs/tags/three\n -+\trefs/tags/two\n +\tEOF\n ++\tgit add . &&\n ++\tgit branch -M main &&\n ++\tgit commit -m \"test\" &&\n ++\tgit remote add origin nowhere &&\n ++\tgit config branch.main.remote origin &&\n ++\tgit config branch.main.merge refs/heads/main &&\n +\tgit for-each-ref --format=\"%(color:#aa22ac)%(refname)\" >actual &&\n +\ttest_cmp expect actual\n +'\n\n\n ref-filter.c | 2 +-\n t/t6302-for-each-ref-filter.sh | 18 ++++++++++++++++++\n 2 files changed, 19 insertions(+), 1 deletion(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex a0adb4551d87..213d3773ada3 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1730,7 +1730,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)\n \t\t\telse\n \t\t\t\tv->s = xstrdup(\"\");\n \t\t\tcontinue;\n-\t\t} else if (atom->u.remote_ref.push) {\n+\t\t} else if (!strcmp(atom->name, \"push\") || starts_with(atom->name, \"push:\")) {\n \t\t\tconst char *branch_name;\n \t\t\tv->s = xstrdup(\"\");\n \t\t\tif (!skip_prefix(ref->refname, \"refs/heads/\",\ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex 9866b1b57368..309cf699506f 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -117,6 +117,24 @@ test_expect_success '%(color) must fail' '\n \ttest_must_fail git for-each-ref --format=\"%(color)%(refname)\"\n '\n \n+test_expect_success '%(color:#aa22ac) must successed' '\n+\ttest_when_finished \"cd .. && rm -rf ./test\" &&\n+\tmkdir test &&\n+\tcd test &&\n+\tgit init &&\n+\tcat >expect <<-\\EOF &&\n+\trefs/heads/main\n+\tEOF\n+\tgit add . &&\n+\tgit branch -M main &&\n+\tgit commit -m \"test\" &&\n+\tgit remote add origin nowhere &&\n+\tgit config branch.main.remote origin &&\n+\tgit config branch.main.merge refs/heads/main &&\n+\tgit for-each-ref --format=\"%(color:#aa22ac)%(refname)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'left alignment is default' '\n \tcat >expect <<-\\EOF &&\n \trefname is refs/heads/main |refs/heads/main\n\nbase-commit: 311531c9de557d25ac087c1637818bd2aad6eb3a\n-- \ngitgitgadget\n"},{"id":"424246","messageId":"xmqq35usivi4.fsf@gitster.g","threadId":"55625","inReplyTo":"pull.949.v5.git.1620747320947.gitgitgadget@gmail.com","subject":"Re: [PATCH v5] [GSOC] ref-filter: fix read invalid union member bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-12T01:36:03Z","receivedAt":"2021-05-12T01:40:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"ZheNing Hu via GitGitGadget\" writes:\n\n> From: ZheNing Hu \n>\n> used_atom.u is an union, and it has different members depending on\n> what atom the auxiliary data the union part of the \"struct\n> used_atom\" wants to record. At most only one of the members can be\n> valid at any one time. Since the code checks u.remote_ref without\n> even making sure if the atom is \"push\" or \"push:\" (which are only\n> two cases that u.remote_ref.push becomes valid), but u.remote_ref\n> shares the same storage for other members of the union, the check\n> was reading from an invalid member, which was the bug.\n>\n> Modify the condition here to check whether the atom name\n> equals to \"push\" or starts with \"push:\", to avoid reading the\n> value of invalid member of the union.\n>\n> Helped-by: Junio C Hamano \n> Signed-off-by: ZheNing Hu \n> ---\n\nThanks.\n\n> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\n> index 9866b1b57368..309cf699506f 100755\n> --- a/t/t6302-for-each-ref-filter.sh\n> +++ b/t/t6302-for-each-ref-filter.sh\n> @@ -117,6 +117,24 @@ test_expect_success '%(color) must fail' '\n> \ttest_must_fail git for-each-ref --format=\"%(color)%(refname)\"\n> '\n> \n> +test_expect_success '%(color:#aa22ac) must successed' '\n\n\"succeed\".\n\n> +\ttest_when_finished \"cd .. && rm -rf ./test\" &&\n\nNot a very good practice to chdir around, even if you have\nwhen-finished clean-up. Not worth risking \"rm -rf\" at random\nplaces when for example somebody breaks when-finished.\n\nInstead...\n\n> +\tmkdir test &&\n\nPlace everything below ...\n\n> +\tcd test &&\n> +\tgit init &&\n> +\tcat >expect <<-\\EOF &&\n> +\trefs/heads/main\n> +\tEOF\n> +\tgit add . &&\n> +\tgit branch -M main &&\n\nThis is in a freshly created repository without commit. Does\n\"branch -M\" work for such an unborn branch? Perhaps start this\nblock like so:\n\n\tgit init test &&\n\t(\n\t\tcd test &&\n\t\ttest_commit initial &&\n\t\tgit branch -M main &&\n\t\tcat >expect <<-\\EOF &&\n\t\trefs/heads/main\n refs/tags/initial\n\t\tEOF\n\t\tgit remote add origin nowhere &&\n\t\t...\n\n> +\tgit commit -m \"test\" &&\n> +\tgit remote add origin nowhere &&\n> +\tgit config branch.main.remote origin &&\n> +\tgit config branch.main.merge refs/heads/main &&\n> +\tgit for-each-ref --format=\"%(color:#aa22ac)%(refname)\" >actual &&\n> +\ttest_cmp expect actual\n\n... up to here inside a (subshell).\n\nBy the way, your use of \"git branch -M\" makes the test work whether\nthe default initial branch name is still 'master' or already 'main'\nby forcing the branch used for testing to be 'main'. Clever ;-).\n\nWill queue with all of the above suggestions squashed in; please see\nif the result is good when it is pushed out later today.\n\nThanks.\n\n> +'\n> +\n> test_expect_success 'left alignment is default' '\n> \tcat >expect <<-\\EOF &&\n> \trefname is refs/heads/main |refs/heads/main\n>\n> base-commit: 311531c9de557d25ac087c1637818bd2aad6eb3a\n"},{"id":"424312","messageId":"CAOLTT8SroeHvZRiRMoeBAPo-nuAvYOzxRx=fq=DxnpUfy4zVsw@mail.gmail.com","threadId":"55625","inReplyTo":"xmqq35usivi4.fsf@gitster.g","subject":"Re: [PATCH v5] [GSOC] ref-filter: fix read invalid union member bug","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-05-12T10:37:05Z","receivedAt":"2021-05-12T10:37:25Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"> > + test_when_finished \"cd .. && rm -rf ./test\" &&\n>\n> Not a very good practice to chdir around, even if you have\n> when-finished clean-up. Not worth risking \"rm -rf\" at random\n> places when for example somebody breaks when-finished.\n>\n\nOK.\n\n> Instead...\n>\n> > + mkdir test &&\n>\n> Place everything below ...\n>\n> > + cd test &&\n> > + git init &&\n> > + cat >expect <<-\\EOF &&\n> > + refs/heads/main\n> > + EOF\n> > + git add . &&\n> > + git branch -M main &&\n>\n> This is in a freshly created repository without commit. Does\n> \"branch -M\" work for such an unborn branch? Perhaps start this\n> block like so:\n>\n\nI think \"git branch -M\" before \"git commit\" also will be fine.\nOf course it's okay to put it after commit and before \"test_cmp\".\n\n> git init test &&\n> (\n> cd test &&\n> test_commit initial &&\n> git branch -M main &&\n> cat >expect <<-\\EOF &&\n> refs/heads/main\n> refs/tags/initial\n> EOF\n> git remote add origin nowhere &&\n> ...\n>\n> > + git commit -m \"test\" &&\n> > + git remote add origin nowhere &&\n> > + git config branch.main.remote origin &&\n> > + git config branch.main.merge refs/heads/main &&\n> > + git for-each-ref --format=\"%(color:#aa22ac)%(refname)\" >actual &&\n> > + test_cmp expect actual\n>\n> ... up to here inside a (subshell).\n>\n\nI get it now. By executing cd in the subshell, the \"cd ..\" step can be\nomitted.\n\n> By the way, your use of \"git branch -M\" makes the test work whether\n> the default initial branch name is still 'master' or already 'main'\n> by forcing the branch used for testing to be 'main'. Clever ;-).\n>\n\nHh, real knowledge comes from practice. Yesterday, I tried it on the old\nversion of git on some classmates’ computers, and there was an error,\nso using \"git branch -M main\" can avoid this error.\n\n> Will queue with all of the above suggestions squashed in; please see\n> if the result is good when it is pushed out later today.\n>\n> Thanks.\n>\n\nThanks!\n--\nZheNing Hu\n"},{"id":"424319","messageId":"pull.949.v6.git.1620821572126.gitgitgadget@gmail.com","threadId":"55625","inReplyTo":"pull.949.v5.git.1620747320947.gitgitgadget@gmail.com","subject":"[PATCH v6] [GSOC] ref-filter: fix read invalid union member bug","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-05-12T12:12:51Z","receivedAt":"2021-05-12T12:12:56Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu \n\nused_atom.u is an union, and it has different members depending on\nwhat atom the auxiliary data the union part of the \"struct\nused_atom\" wants to record. At most only one of the members can be\nvalid at any one time. Since the code checks u.remote_ref without\neven making sure if the atom is \"push\" or \"push:\" (which are only\ntwo cases that u.remote_ref.push becomes valid), but u.remote_ref\nshares the same storage for other members of the union, the check\nwas reading from an invalid member, which was the bug.\n\nModify the condition here to check whether the atom name\nequals to \"push\" or starts with \"push:\", to avoid reading the\nvalue of invalid member of the union.\n\nHelped-by: Junio C Hamano \nSigned-off-by: ZheNing Hu \n---\n [GSOC] ref-filter: fix read invalid union member bug\n \n Change from last version: Modify the test content to make the test more\n correct and complete.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-949%2Fadlternative%2Fref-filter-enum-bug-fix-v6\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-949/adlternative/ref-filter-enum-bug-fix-v6\nPull-Request: https://github.com/gitgitgadget/git/pull/949\n\nRange-diff vs v5:\n\n 1: b546477e8c87 ! 1: 518cc41a78a4 [GSOC] ref-filter: fix read invalid union member bug\n @@ t/t6302-for-each-ref-filter.sh: test_expect_success '%(color) must fail' '\n \ttest_must_fail git for-each-ref --format=\"%(color)%(refname)\"\n '\n \n -+test_expect_success '%(color:#aa22ac) must successed' '\n -+\ttest_when_finished \"cd .. && rm -rf ./test\" &&\n ++test_expect_success '%(color:#aa22ac) must succeed' '\n +\tmkdir test &&\n -+\tcd test &&\n -+\tgit init &&\n -+\tcat >expect <<-\\EOF &&\n -+\trefs/heads/main\n -+\tEOF\n -+\tgit add . &&\n -+\tgit branch -M main &&\n -+\tgit commit -m \"test\" &&\n -+\tgit remote add origin nowhere &&\n -+\tgit config branch.main.remote origin &&\n -+\tgit config branch.main.merge refs/heads/main &&\n -+\tgit for-each-ref --format=\"%(color:#aa22ac)%(refname)\" >actual &&\n -+\ttest_cmp expect actual\n ++\tgit init test &&\n ++\t(\n ++\t\tcd test &&\n ++\t\ttest_commit initial &&\n ++\t\tgit branch -M main &&\n ++\t\tcat >expect <<-\\EOF &&\n ++\t\trefs/heads/main\n ++\t\trefs/tags/initial\n ++\t\tEOF\n ++\t\tgit remote add origin nowhere &&\n ++\t\tgit config branch.main.remote origin &&\n ++\t\tgit config branch.main.merge refs/heads/main &&\n ++\t\tgit for-each-ref --format=\"%(color:#aa22ac)%(refname)\" >actual &&\n ++\t\ttest_cmp expect actual\n ++\t)\n +'\n +\n test_expect_success 'left alignment is default' '\n\n\n ref-filter.c | 2 +-\n t/t6302-for-each-ref-filter.sh | 19 +++++++++++++++++++\n 2 files changed, 20 insertions(+), 1 deletion(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex a0adb4551d87..213d3773ada3 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1730,7 +1730,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)\n \t\t\telse\n \t\t\t\tv->s = xstrdup(\"\");\n \t\t\tcontinue;\n-\t\t} else if (atom->u.remote_ref.push) {\n+\t\t} else if (!strcmp(atom->name, \"push\") || starts_with(atom->name, \"push:\")) {\n \t\t\tconst char *branch_name;\n \t\t\tv->s = xstrdup(\"\");\n \t\t\tif (!skip_prefix(ref->refname, \"refs/heads/\",\ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex 9866b1b57368..b64abe4184b8 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -117,6 +117,25 @@ test_expect_success '%(color) must fail' '\n \ttest_must_fail git for-each-ref --format=\"%(color)%(refname)\"\n '\n \n+test_expect_success '%(color:#aa22ac) must succeed' '\n+\tmkdir test &&\n+\tgit init test &&\n+\t(\n+\t\tcd test &&\n+\t\ttest_commit initial &&\n+\t\tgit branch -M main &&\n+\t\tcat >expect <<-\\EOF &&\n+\t\trefs/heads/main\n+\t\trefs/tags/initial\n+\t\tEOF\n+\t\tgit remote add origin nowhere &&\n+\t\tgit config branch.main.remote origin &&\n+\t\tgit config branch.main.merge refs/heads/main &&\n+\t\tgit for-each-ref --format=\"%(color:#aa22ac)%(refname)\" >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_expect_success 'left alignment is default' '\n \tcat >expect <<-\\EOF &&\n \trefname is refs/heads/main |refs/heads/main\n\nbase-commit: 311531c9de557d25ac087c1637818bd2aad6eb3a\n-- \ngitgitgadget\n"},{"id":"424398","messageId":"xmqqfsyredsr.fsf@gitster.g","threadId":"55625","inReplyTo":"pull.949.v6.git.1620821572126.gitgitgadget@gmail.com","subject":"Re: [PATCH v6] [GSOC] ref-filter: fix read invalid union member bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-05-12T23:24:20Z","receivedAt":"2021-05-12T23:32:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"ZheNing Hu via GitGitGadget\" writes:\n\n> +test_expect_success '%(color:#aa22ac) must succeed' '\n> +\tmkdir test &&\n\nYou do not need \"mkdir test\" here, as \"git init test\" would take\ncare of it.\n\nOn the other hand, you'd want to get 'test' directory removed after\nthis passes, so \n\n\ttest_when_finished rm -fr test &&\n\nwould be useful.\n\nOther than that, looks good to me.\n\nThanks.\n\n> +\tgit init test &&\n> +\t(\n> +\t\tcd test &&\n> +\t\ttest_commit initial &&\n> +\t\tgit branch -M main &&\n> +\t\tcat >expect <<-\\EOF &&\n> +\t\trefs/heads/main\n> +\t\trefs/tags/initial\n> +\t\tEOF\n> +\t\tgit remote add origin nowhere &&\n> +\t\tgit config branch.main.remote origin &&\n> +\t\tgit config branch.main.merge refs/heads/main &&\n> +\t\tgit for-each-ref --format=\"%(color:#aa22ac)%(refname)\" >actual &&\n> +\t\ttest_cmp expect actual\n> +\t)\n> +'\n> +\n> test_expect_success 'left alignment is default' '\n> \tcat >expect <<-\\EOF &&\n> \trefname is refs/heads/main |refs/heads/main\n>\n> base-commit: 311531c9de557d25ac087c1637818bd2aad6eb3a\n"},{"id":"424451","messageId":"CAOLTT8R0sXRLB5=ePGi9dSpKR+YFSYLtDPvoD8PhNzneF0=NrQ@mail.gmail.com","threadId":"55625","inReplyTo":"xmqqfsyredsr.fsf@gitster.g","subject":"Re: [PATCH v6] [GSOC] ref-filter: fix read invalid union member bug","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2021-05-13T09:29:54Z","receivedAt":"2021-05-13T09:30:09Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Junio C Hamano 于2021年5月13日周四 上午7:24写道：\n>\n> \"ZheNing Hu via GitGitGadget\" writes:\n>\n> > +test_expect_success '%(color:#aa22ac) must succeed' '\n> > + mkdir test &&\n>\n> You do not need \"mkdir test\" here, as \"git init test\" would take\n> care of it.\n>\n\nMake sence.\n\n> On the other hand, you'd want to get 'test' directory removed after\n> this passes, so\n>\n> test_when_finished rm -fr test &&\n>\n> would be useful.\n>\n\nWell, deleting or not deleting the directory \"test\" is not a big problem here.\nDelete it is ok.\n\n> Other than that, looks good to me.\n>\n> Thanks.\n>\n\nThanks.\n--\nZheNing Hu\n"},{"id":"424476","messageId":"pull.949.v7.git.1620918814489.gitgitgadget@gmail.com","threadId":"55625","inReplyTo":"pull.949.v6.git.1620821572126.gitgitgadget@gmail.com","subject":"[PATCH v7] [GSOC] ref-filter: fix read invalid union member bug","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-05-13T15:13:33Z","receivedAt":"2021-05-13T15:13:42Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu \n\nused_atom.u is an union, and it has different members depending on\nwhat atom the auxiliary data the union part of the \"struct\nused_atom\" wants to record. At most only one of the members can be\nvalid at any one time. Since the code checks u.remote_ref without\neven making sure if the atom is \"push\" or \"push:\" (which are only\ntwo cases that u.remote_ref.push becomes valid), but u.remote_ref\nshares the same storage for other members of the union, the check\nwas reading from an invalid member, which was the bug.\n\nModify the condition here to check whether the atom name\nequals to \"push\" or starts with \"push:\", to avoid reading the\nvalue of invalid member of the union.\n\nHelped-by: Junio C Hamano \nSigned-off-by: ZheNing Hu \n---\n [GSOC] ref-filter: fix read invalid union member bug\n \n Change from last version: Modify the test content, reduce additional\n 'mkdir' operation and remove temp-dir \"test\" when finished.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-949%2Fadlternative%2Fref-filter-enum-bug-fix-v7\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-949/adlternative/ref-filter-enum-bug-fix-v7\nPull-Request: https://github.com/gitgitgadget/git/pull/949\n\nRange-diff vs v6:\n\n 1: 518cc41a78a4 ! 1: 50bef8439a85 [GSOC] ref-filter: fix read invalid union member bug\n @@ t/t6302-for-each-ref-filter.sh: test_expect_success '%(color) must fail' '\n '\n \n +test_expect_success '%(color:#aa22ac) must succeed' '\n -+\tmkdir test &&\n ++\ttest_when_finished rm -fr test &&\n +\tgit init test &&\n +\t(\n +\t\tcd test &&\n\n\n ref-filter.c | 2 +-\n t/t6302-for-each-ref-filter.sh | 19 +++++++++++++++++++\n 2 files changed, 20 insertions(+), 1 deletion(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex a0adb4551d87..213d3773ada3 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1730,7 +1730,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)\n \t\t\telse\n \t\t\t\tv->s = xstrdup(\"\");\n \t\t\tcontinue;\n-\t\t} else if (atom->u.remote_ref.push) {\n+\t\t} else if (!strcmp(atom->name, \"push\") || starts_with(atom->name, \"push:\")) {\n \t\t\tconst char *branch_name;\n \t\t\tv->s = xstrdup(\"\");\n \t\t\tif (!skip_prefix(ref->refname, \"refs/heads/\",\ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex 9866b1b57368..5070d0cf1e80 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -117,6 +117,25 @@ test_expect_success '%(color) must fail' '\n \ttest_must_fail git for-each-ref --format=\"%(color)%(refname)\"\n '\n \n+test_expect_success '%(color:#aa22ac) must succeed' '\n+\ttest_when_finished rm -fr test &&\n+\tgit init test &&\n+\t(\n+\t\tcd test &&\n+\t\ttest_commit initial &&\n+\t\tgit branch -M main &&\n+\t\tcat >expect <<-\\EOF &&\n+\t\trefs/heads/main\n+\t\trefs/tags/initial\n+\t\tEOF\n+\t\tgit remote add origin nowhere &&\n+\t\tgit config branch.main.remote origin &&\n+\t\tgit config branch.main.merge refs/heads/main &&\n+\t\tgit for-each-ref --format=\"%(color:#aa22ac)%(refname)\" >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_expect_success 'left alignment is default' '\n \tcat >expect <<-\\EOF &&\n \trefname is refs/heads/main |refs/heads/main\n\nbase-commit: 311531c9de557d25ac087c1637818bd2aad6eb3a\n-- \ngitgitgadget\n"}]}