Message ID | pull.949.v3.git.1620487572222.gitgitgadget@gmail.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | [v3,GSOC] ref-filter: fix read invalid union member bug | expand |
"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: ZheNing Hu <adlternative@gmail.com> > > 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 <gitster@pobox.com> > Signed-off-by: ZheNing Hu <adlternative@gmail.com> > --- > [GSOC] ref-filter: fix read invalid union member bug > > Change from last version: > Modify the processing method of the condition: check whether the name of > the atom equals to "push" or starts with "pushs", which can enhanced > security, although it may bring string match overhead. I do not think this would have much security implication either way. What it buys us is the future-proofing. I think it is OK to make this change without the enum thing to have it graduate early as a fix to the existing code. The enum thing can come on top. Will queue. Thanks.
"ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: ZheNing Hu <adlternative@gmail.com> > > 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 <gitster@pobox.com> > Signed-off-by: ZheNing Hu <adlternative@gmail.com> > --- 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
Junio C Hamano <gitster@pobox.com> 于2021年5月10日周一 下午3:21写道: > > "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: ZheNing Hu <adlternative@gmail.com> > > > > 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 <gitster@pobox.com> > > Signed-off-by: ZheNing Hu <adlternative@gmail.com> > > --- > > [GSOC] ref-filter: fix read invalid union member bug > > > > Change from last version: > > Modify the processing method of the condition: check whether the name of > > the atom equals to "push" or starts with "pushs", which can enhanced > > security, although it may bring string match overhead. > > I do not think this would have much security implication either > way. What it buys us is the future-proofing. > Ah, truely. > I think it is OK to make this change without the enum thing to have > it graduate early as a fix to the existing code. The enum thing can > come on top. > Indeed. "enum atom_type" is for ref-filter performance optimization and get some other benefits like quick index. So I put it in another topic. > Will queue. Thanks. Thanks. -- 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? > Well, The trigger condition is very special, but the bug was introduced at that time. Let's see the "bug" example below. > I am wondering if it is easy to add a test to cover the codepath > that is affected by this change. > > Thanks. > Well, because this bug must require that the seventeenth bit of `used_atom.u` is not 0, it took me a long time to find this bug. in `used_atom.u`, only the member "color" and "contents" which size is bigger than 17 bytes, but "%(contents:trailer:only)" only fill the 16th byte of `used_atom.u`. "Fortunately", I found it. git for-each-ref --format='%(color:#aa22ac)' I will add test for it! 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/",