Re: [PATCH v4] [GSOC] ref-filter: fix read invalid union member bug

2 messages, 2 authors, 2021-05-11 · open the first message on its own page

Re: [PATCH v4] [GSOC] ref-filter: fix read invalid union member bug

From: Junio C Hamano <hidden>
Date: 2021-05-11 02:29:05

"ZheNing Hu via GitGitGadget" [off-list ref] 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?

quoted hunk

     ++	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

Re: [PATCH v4] [GSOC] ref-filter: fix read invalid union member bug

From: ZheNing Hu <hidden>
Date: 2021-05-11 06:28:53

Junio C Hamano [off-list ref] 于2021年5月11日周二 上午10:29写道：

"ZheNing Hu via GitGitGadget" [off-list ref] writes:

quoted

    Change from last version:
    Prove that the bug may appear when using %(color) atom. And add
    corresponding test for it.

quoted

     ++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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help