Re: [PATCH] [GSOC] ref-filter: solve bugs caused by enumeration

From: ZheNing Hu <adlternative@gmail.com>
Date: May 6, 2021, 10:39 UTC
Message-ID: <CAOLTT8S9TbRWGPeGKR9=cDnaw2RMukawV48dPEK1KL0X_87udA@mail.gmail.com>
In-Reply-To: <xmqqk0oczao3.fsf@gitster.g>

> This time you avoided the word enumeration, and it made it clearer.
> The word commonly used is "condition" where you said "judgment", I
> think, and wit that it would probably be even more clear.
>

OK.

Show 9 quoted lines

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

Yes, that's what it means. I got it wrong before, of course used_atom.u is not a enum, it's union.

Show 7 quoted lines

> So the fix was to see what atom it is by checking its name member?
> Is starts_with() a reliable test?  A fictitious atom "pushe" will be
> different from "push" or "push:<something>", but will still pass
> that test, so from the point of view of future-proofing the tests,
> shouldn't it do the same check as the one at the beginning of
> remote_ref_atom_parser()?
>

I think it's not necesssary. Before we call `populate_value()`,
`parse_ref_filter_atom()` only allow user use atom which match
 "valid_atom" entry fully, something like "pushe" will be rejected
and process die with "fatal: unknown field name: pushe", so it didn't
pass this "starts_with()" test.

        /* Is the atom a valid one? */
        for (i = 0; i < ARRAY_SIZE(valid_atom); i++) {
                int len = strlen(valid_atom[i].name);
                if (len == atom_len && !memcmp(valid_atom[i].name, sp, len))
                        break;
        }

        if (ARRAY_SIZE(valid_atom) <= i)
                return strbuf_addf_ret(err, -1, _("unknown field name: %.*s"),
                                       (int)(ep-atom), atom);

> Thanks.

Thanks. -- ZheNing Hu