Re: [PATCH 6/6] sequencer: fail early if invalid ref is given to 'update-ref' instruction

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sep 30, 2022, 17:09 UTC
Message-ID: <220930.86a66gvkse.gmgdl@evledraar.gmail.com>
In-Reply-To: <20220930140948.80367-7-szeder.dev@gmail.com>

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"