From: Junio C Hamano Date: Mon, 10 May 2021 07:27:36 GMT Subject: Re: [PATCH v3] [GSOC] ref-filter: fix read invalid union member bug Message-ID: In-Reply-To: "ZheNing Hu via GitGitGadget" writes: > From: ZheNing Hu > > used_atom.u is an union, and it has different members depending on > what atom the auxiliary data the union part of the "struct > used_atom" wants to record. At most only one of the members can be > valid at any one time. Since the code checks u.remote_ref without > even making sure if the atom is "push" or "push:" (which are only > two cases that u.remote_ref.push becomes valid), but u.remote_ref > shares the same storage for other members of the union, the check > was reading from an invalid member, which was the bug. > > Modify the condition here to check whether the atom name > equals to "push" or starts with "push:", to avoid reading the > value of invalid member of the union. > > Helped-by: Junio C Hamano > Signed-off-by: ZheNing Hu > --- Just a final sanity check. Is this a recent breakage or was the code introduced at cc72385f (for-each-ref: let upstream/push optionally report the remote name, 2017-10-05) broken from the beginning? I am wondering if it is easy to add a test to cover the codepath that is affected by this change. Thanks. > 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/", > > base-commit: 311531c9de557d25ac087c1637818bd2aad6eb3a