Re: [PATCH 6/6] sequencer: fail early if invalid ref is given to 'update-ref' instruction
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2022-09-30 17:23:21
On Fri, Sep 30 2022, SZEDER Gábor wrote:
+ 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).
quoted hunk ↗ jump to hunk
+ 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"