| Message ID | pull.949.v4.git.1620658904283.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4,GSOC] ref-filter: fix read invalid union member bug | expand |
"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes: > Change from last version: > Prove that the bug may appear when using %(color) atom. And add > corresponding test for it. > ++test_expect_success '%(color:#aa22ac) must success' ' s/success/succeed/ But more importantly, I am not sure how this is supposed to demonstrate existing breakage around the %(push). Did you mean to use %(push) instead of %(refname) or something? > ++ cat >expect <<-\EOF && > ++ refs/heads/main > ++ refs/heads/side > ++ refs/odd/spot > ++ refs/tags/annotated-tag > ++ refs/tags/doubly-annotated-tag > ++ refs/tags/doubly-signed-tag > ++ refs/tags/four > ++ refs/tags/one > ++ refs/tags/signed-tag > ++ refs/tags/three > ++ refs/tags/two > ++ EOF > ++ git for-each-ref --format="%(color:#aa22ac)%(refname)" >actual && > ++ test_cmp expect actual > ++' > ++ > + test_expect_success 'left alignment is default' ' > + cat >expect <<-\EOF && > + refname is refs/heads/main |refs/heads/main > > > ref-filter.c | 2 +- > t/t6302-for-each-ref-filter.sh | 18 ++++++++++++++++++ > 2 files changed, 19 insertions(+), 1 deletion(-) > > diff --git a/ref-filter.c b/ref-filter.c > index a0adb4551d87..213d3773ada3 100644 > --- a/ref-filter.c > +++ b/ref-filter.c > @@ -1730,7 +1730,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err) > else > v->s = xstrdup(""); > continue; > - } else if (atom->u.remote_ref.push) { > + } else if (!strcmp(atom->name, "push") || starts_with(atom->name, "push:")) { > const char *branch_name; > v->s = xstrdup(""); > if (!skip_prefix(ref->refname, "refs/heads/", > diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh > index 9866b1b57368..38a7d83830aa 100755 > --- a/t/t6302-for-each-ref-filter.sh > +++ b/t/t6302-for-each-ref-filter.sh > @@ -117,6 +117,24 @@ test_expect_success '%(color) must fail' ' > test_must_fail git for-each-ref --format="%(color)%(refname)" > ' > > +test_expect_success '%(color:#aa22ac) must success' ' > + cat >expect <<-\EOF && > + refs/heads/main > + refs/heads/side > + refs/odd/spot > + refs/tags/annotated-tag > + refs/tags/doubly-annotated-tag > + refs/tags/doubly-signed-tag > + refs/tags/four > + refs/tags/one > + refs/tags/signed-tag > + refs/tags/three > + refs/tags/two > + EOF > + git for-each-ref --format="%(color:#aa22ac)%(refname)" >actual && > + test_cmp expect actual > +' > + > test_expect_success 'left alignment is default' ' > cat >expect <<-\EOF && > refname is refs/heads/main |refs/heads/main > > base-commit: 311531c9de557d25ac087c1637818bd2aad6eb3a
Junio C Hamano <gitster@pobox.com> 于2021年5月11日周二 上午10:29写道： > > "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > Change from last version: > > Prove that the bug may appear when using %(color) atom. And add > > corresponding test for it. > > > ++test_expect_success '%(color:#aa22ac) must success' ' > > s/success/succeed/ > > But more importantly, I am not sure how this is supposed to > demonstrate existing breakage around the %(push). Did you mean to > use %(push) instead of %(refname) or something? > Ah, I don’t think this scene of damage is related to using %(push) or %(refname). We are just in the process of using `populate_value()`, if the atom we specify meets the following conditions, then the condition of atom->u.remote_ref.push!=0 will be established. 1. The atom that triggers the bug , its "if" condition order must after "if (atom->u.remote_ref.push)", such as %(refname) or %(worktreepath), they can be executed correctly because their order is before "push". 2. The member size in used_atom.u corresponding to the atom must larger than 17 bytes, because the offset of "u.remote_ref.push" in "u.remote_ref" is 17, the satisfied atoms are only "%(color)" and "%(contents)", their corresponding members are u.color and u.contents. 3. We happen to be able to fill in the 17th position of these structures, which makes atom->u.remote_ref.push not equal to 0 established. So this kind of bug is not related to %(push), an atom that satisfies the above conditions will make `if (atom->u.remote_ref.push)` be true. then execute the program logic related to `%(push)`. Now, we only have `%(color)` can trigger it "sometime", It is unpredictable to fill in the 17th byte of used_atom.u.color, so we cannot track all the atoms related to this bug. git for-each-ref --format="%(color:#aa22ac)%(refname)" git for-each-ref --format="%(color:#aa22ad)%(refname)" will trigger the bug. git for-each-ref --format="%(color:#aa22ae)%(refname)" git for-each-ref --format="%(color:#aa22af)%(refname)" will not trigger the bug. In other words, we cannot use a perfect test set to cover all broken. So now `%(color:#aa22ac)` is enough for explain the problem of this bug. Thanks. -- ZheNing Hu
ZheNing Hu <adlternative@gmail.com> writes: > We are just in the process of using `populate_value()`, if the atom we > specify meets the following conditions, then the condition of > atom->u.remote_ref.push!=0 will be established. > > 1. The atom that triggers the bug , its "if" condition order must after > "if (atom->u.remote_ref.push)", such as %(refname) or %(worktreepath), > they can be executed correctly because their order is before "push". > > 2. The member size in used_atom.u corresponding to the atom must > larger than 17 bytes, because the offset of "u.remote_ref.push" in > "u.remote_ref" is 17, the satisfied atoms are only "%(color)" and > "%(contents)", their corresponding members are u.color and u.contents. > > 3. We happen to be able to fill in the 17th position of these structures, > which makes atom->u.remote_ref.push not equal to 0 established. > > So this kind of bug is not related to %(push), an atom that satisfies > the above conditions will make `if (atom->u.remote_ref.push)` be true. > then execute the program logic related to `%(push)`. > > Now, we only have `%(color)` can trigger it "sometime", > It is unpredictable to fill in the 17th byte of used_atom.u.color, > so we cannot track all the atoms related to this bug. > > git for-each-ref --format="%(color:#aa22ac)%(refname)" > git for-each-ref --format="%(color:#aa22ad)%(refname)" > > will trigger the bug. > > git for-each-ref --format="%(color:#aa22ae)%(refname)" > git for-each-ref --format="%(color:#aa22af)%(refname)" > > will not trigger the bug. > > In other words, we cannot use a perfect test set to cover all broken. > So now `%(color:#aa22ac)` is enough for explain the problem of this bug. Well, the thing is, $ git checkout 49f38e2d ;# (The fifteenth batch, 2021-05-10) $ git am -s mbox $ git show --stat --oneline 39509d100a (HEAD) ref-filter: fix read invalid union member bug ref-filter.c | 2 +- t/t6302-for-each-ref-filter.sh | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) $ git show ref-filter.c | git apply -R ;# revert only the fix $ make -j32 && make -C t T=t6302-*.sh does not seem to break anything. Perhaps there is something more than the "17th byte" thing (like structure padding that may vary depending on the compiler and architecture)?
> Well, the thing is, > > $ git checkout 49f38e2d ;# (The fifteenth batch, 2021-05-10) > $ git am -s mbox > $ git show --stat --oneline > 39509d100a (HEAD) ref-filter: fix read invalid union member bug > ref-filter.c | 2 +- > t/t6302-for-each-ref-filter.sh | 18 ++++++++++++++++++ > 2 files changed, 19 insertions(+), 1 deletion(-) > $ git show ref-filter.c | git apply -R ;# revert only the fix > $ make -j32 && make -C t T=t6302-*.sh > > does not seem to break anything. Perhaps there is something more > than the "17th byte" thing (like structure padding that may vary > depending on the compiler and architecture)? Fine, I guess the reason for this mystery is I "push" this branch to github and you haven't done it. That may not be due to the platform. Because I can see no this bug happening when I use a new git repo without "git push", and I test in archlinux or deepin, this bug will happen in these environments. I take back what I said before, maybe this is really related to push. Let's see what happens: #!/bin/sh mkdir test cd test git init echo 1>1 git add . git branch -M main git commit -m "test" git remote add origin nowhere git config branch.main.remote origin git config branch.main.merge refs/heads/main git for-each-ref --format="%(color:#aa22ac)%(objectname)" These two "git config" is for simulating a push environment. I guess you also saw this bug: BUG: ref-filter.c:1544: unhandled RR_* enum I will use it for the test. Thanks. -- ZheNing Hu
ZheNing Hu <adlternative@gmail.com> writes: >> Well, the thing is, >> >> $ git checkout 49f38e2d ;# (The fifteenth batch, 2021-05-10) >> $ git am -s mbox >> $ git show --stat --oneline >> 39509d100a (HEAD) ref-filter: fix read invalid union member bug >> ref-filter.c | 2 +- >> t/t6302-for-each-ref-filter.sh | 18 ++++++++++++++++++ >> 2 files changed, 19 insertions(+), 1 deletion(-) >> $ git show ref-filter.c | git apply -R ;# revert only the fix >> $ make -j32 && make -C t T=t6302-*.sh >> >> does not seem to break anything. Perhaps there is something more >> than the "17th byte" thing (like structure padding that may vary >> depending on the compiler and architecture)? > > Fine, I guess the reason for this mystery is I "push" this branch to github > and you haven't done it. That may not be due to the platform. Because I > can see no this bug happening when I use a new git repo without "git push", > and I test in archlinux or deepin, this bug will happen in these environments. Sorry, you lost me. I was talking about what happens in the new test you added to t6302 not failing as designed, and there shouldn't be "I've pushed but you haven't pushed to GitHub" distinction. The test is running in a brand-new repository just created for the sole purpose of running the test after all. > #!/bin/sh > mkdir test > cd test > git init > echo 1>1 > git add . > git branch -M main > git commit -m "test" > git remote add origin nowhere > git config branch.main.remote origin > git config branch.main.merge refs/heads/main > git for-each-ref --format="%(color:#aa22ac)%(objectname)" > > These two "git config" is for simulating a push environment. So in short, the test script added to t6302 in the v4 patch was not testing what it was supposed to be testing, as it didn't have the configuration items related to %(push) atom necessary to trigger the error? That I can believe. I was starting to worry if there was something more subtle going on, but I am glad that it was only an uncooked patch submitted without checking. > I guess you also saw this bug: > > BUG: ref-filter.c:1544: unhandled RR_* enum No, I didn't. I just tried to make sure the new test was truly checking the existing breakage by partially reverting the code fix, and saw that the new test did not fail. Thanks.
> > Fine, I guess the reason for this mystery is I "push" this branch to github > > and you haven't done it. That may not be due to the platform. Because I > > can see no this bug happening when I use a new git repo without "git push", > > and I test in archlinux or deepin, this bug will happen in these environments. > > Sorry, you lost me. I was talking about what happens in the new > test you added to t6302 not failing as designed, and there shouldn't > be "I've pushed but you haven't pushed to GitHub" distinction. The > test is running in a brand-new repository just created for the sole > purpose of running the test after all. > Well, there are some expression problems here, I just want to express: This bug is only triggered after I push and Git adds some config during the push process. And then those config effort %(push) behavior. > So in short, the test script added to t6302 in the v4 patch was not > testing what it was supposed to be testing, as it didn't have the > configuration items related to %(push) atom necessary to trigger the > error? > Truly. > That I can believe. I was starting to worry if there was something > more subtle going on, but I am glad that it was only an uncooked > patch submitted without checking. > Me too, I want to complain a little bit: this bug is too difficult to locate. > > I guess you also saw this bug: > > > > BUG: ref-filter.c:1544: unhandled RR_* enum > > No, I didn't. I just tried to make sure the new test was truly > checking the existing breakage by partially reverting the code fix, > and saw that the new test did not fail. > I understand. > Thanks. Thanks. -- ZheNing Hu
diff --git a/ref-filter.c b/ref-filter.c index a0adb4551d87..213d3773ada3 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1730,7 +1730,7 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err) else v->s = xstrdup(""); continue; - } else if (atom->u.remote_ref.push) { + } else if (!strcmp(atom->name, "push") || starts_with(atom->name, "push:")) { const char *branch_name; v->s = xstrdup(""); if (!skip_prefix(ref->refname, "refs/heads/", diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh index 9866b1b57368..38a7d83830aa 100755 --- a/t/t6302-for-each-ref-filter.sh +++ b/t/t6302-for-each-ref-filter.sh @@ -117,6 +117,24 @@ test_expect_success '%(color) must fail' ' test_must_fail git for-each-ref --format="%(color)%(refname)" ' +test_expect_success '%(color:#aa22ac) must success' ' + cat >expect <<-\EOF && + refs/heads/main + refs/heads/side + refs/odd/spot + refs/tags/annotated-tag + refs/tags/doubly-annotated-tag + refs/tags/doubly-signed-tag + refs/tags/four + refs/tags/one + refs/tags/signed-tag + refs/tags/three + refs/tags/two + EOF + git for-each-ref --format="%(color:#aa22ac)%(refname)" >actual && + test_cmp expect actual +' + test_expect_success 'left alignment is default' ' cat >expect <<-\EOF && refname is refs/heads/main |refs/heads/main