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

From: Junio C Hamano <gitster@pobox.com>
Date: May 6, 2021, 21:20 UTC
Message-ID: <xmqq8s4ry2ye.fsf@gitster.g>
In-Reply-To: <CAOLTT8TQXAh9vu21d5rvaHe=+eqtFU8MCVhorfGmqYRohDThUg@mail.gmail.com>

ZheNing Hu <adlternative@gmail.com> writes:

Show 11 quoted lines

> ... now I think something like this will be more secure. In the
> future, other people may be grateful for such a choice. :)
>
> @@ -1730,7 +1759,7 @@ static int populate_value(struct ref_array_item
> *ref, struct strbuf *err)
>                         else
>                                 v->s = xstrdup("");
>                         continue;
> -               } else if (starts_with(name, "push")) {
> +               } else if (starts_with(name, "push") &&
> atom->u.remote_ref.push) {

Hmph, I do no think that would be futureproof at all. If a new atom "pushe" gets added, it is not all that unlikely that it would add its own member to the same union. name here would be "pushe" and starts with "push", and upon parsing "pushe", its own member in the union, atom->u.X, would have been assigned to, but the code we see here still accesses atom->u.remote_ref.*, so you still have the same problem you started to solve, no?

The check we use in remote_ref_atom_parser() to see if the atom is about pushing, i.e.

	if (!strcmp(atom->name, "push") || starts_with(atom->name, "push:"))

is unlikely to be invalidated in future changes, as this is very much end-user facing and changing the condition will break existing scripts, so that is what I was expecting you to use instead.