Re: [PATCH v3 21/23] try_remove_empty_parents(): don't accommodate consecutive slashes

> My assumption was that only valid reference names should ever be allowed
> to be inserted into a `ref_transaction` entry. But Peff is right that
> sometimes the reference name is checked by `refname_is_safe()` rather
> than `check_refname_format()`. Let's audit this more carefully...
> 
> * `ref_transaction_add_update()` relies on its caller doing the check
>   (this fact is documented). Its callers are:
>   * `ref_transaction_update()` (the usual codepath), which checks the
>     reference itself using either check_refname_format() or
>     refname_is_safe() depending on what kind of update it is.
>   * `split_head_update()` passes the literal string "HEAD".
>   * `split_symref_update()` picks apart reference updates that go
>     through existing symbolic references. As such I don't think they
>     are an attack surface. It doesn't do any checking itself (here
>     we're talking about its `referent` argument). It has only one
>     caller:
>     * `lock_ref_for_update()`, which gets `referent` from:
>       * `files_read_raw_ref()`, which gets the value either:
>         * by reading a filesystem-level symlink's contents and
>           checking it with `check_refname_format()`, or
>         * reading a symref from the filesystem. In this case, *the
>           value is not checked*.
> 
> Obviously this chain of custody is disconcertingly long and complicated.
> And the gap for symrefs should probably be fixed, even though they are
> hopefully trustworthy.

> I think the best thing to do is to drop this patch from the patch
> series, and address these checks in a separate series. (There are more
> known problems in this area, for example that the checks in
> `check_refname_format()` are not a strict superset of the checks in
> `refname_is_safe()`.)