Re: [PATCH 6/6] sequencer: fail early if invalid ref is given to 'update-ref' instruction
On Fri, Sep 30 2022, SZEDER Gábor wrote:
Show 9 quoted lines
> + if (item->command == TODO_UPDATE_REF) {
> + struct strbuf ref = STRBUF_INIT;
> + int ret = 0;
> +
> + item->commit = NULL;
> + item->arg_offset = bol - buf;
> + item->arg_len = (int)(eol - bol);
> +
> + strbuf_add(&ref, bol, item->arg_len);Just a nit and maybe not worth it, but we've done this allocation dance just because..
> + if (!starts_with(ref.buf, "refs/") ||
> + check_refname_format(ref.buf, 0))
...there isn't such a thing as checkn_refname_format() taking a "size_t len" or whatever.
So maybe not worth it, but if we do the equivalent of:
static checkn_refname_format(const char *refname, size_t len, unsigned int flags)
{
struct strbuf ref = STRBUF_INIT;
int ret;
strbuf_add(&ref, refname, len);
ret = check_refname_format(ref,buf, flags);
strbuf_release(&ref);
return ret;
}This caller could just (untested):
if (!starts_with(bol, "refs/") ||
checkn_refname_format(bol, eol - bol, 0))
return error(_("...%.*s", item->arg_len, bol));Which saves us the copy in case the "starts_with" test is all we need.
Even without such a helper, maybe:
int bad;
[...]
bad = (!starts_with(ref.buf, "refs/") ||
check_refname_format(ref.buf, 0));
strbuf_release(&buf);
if (bad)
return error(_("...%.*s", item->arg_len, bol));
return 0;Would make it clearer that the strbuf is just for the use of check_refname_format().
What you have already is also fine, this just sent me down a rabbit hole of re-learning that most of the string duplication we do for check_refname_format() could be avoided if it was slightly less stupid, i.e. accepted a "len" and "prefix" (i.e. "pretend your refname argument started with 'refs/heads/'", or whatever).
Show 24 quoted lines
> + ret = error(_("invalid ref for update-ref instruction: %s"), ref.buf);
> +
> + strbuf_release(&ref);
> + return ret;
> + }
> +
> end_of_object_name = (char *) bol + strcspn(bol, " \t\n");
> saved = *end_of_object_name;
> *end_of_object_name = '\0';
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 2e081b3914..b97f1e8b31 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1964,6 +1964,34 @@ test_expect_success 'respect user edits to update-ref steps' '
> test_cmp_rev HEAD refs/heads/no-conflict-branch
> '
>
> +test_expect_success 'update-refs with invalid refs' '
> + cat >fake-todo-4 <<-EOF &&
> + update-ref refs/heads/foo..bar
> + update-ref refs/heads/foo.lock
> + update-ref foo
> + update-ref foo/bar
> + pick $(git rev-parse HEAD)Another potentially hidden segfault/exit code for "git"
SZEDER Gábor
