Re: [PATCH v4 02/14] commit-graph: prepare for commit-graph chains

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 6, 2019, 21:28 UTC
Message-ID: <xmqqd0jqqu8p.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <d0dc154a27c80a8113ff47eb60a6b6f98fbeec91.1559830527.git.gitgitgadget@gmail.com>

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 9 quoted lines

> +static void load_oid_from_graph(struct commit_graph *g, int pos, struct object_id *oid)
> +{
> +	uint32_t lex_index;
> +
> +	if (!g)
> +		BUG("NULL commit-graph");
> +
> +	while (pos < g->num_commits_in_base)
> +		g = g->base_graph;

If a rogue caller calls this function with pos < 0, this loop would eventually exhaust the chain and make g==NULL, I think. Shouldn't a similar assert exist upfront for "if (pos < 0)" or perhaps make pos unsigned int instead?

> +	if (pos >= g->num_commits + g->num_commits_in_base)
> +		BUG("position %d is beyond the scope of this commit-graph (%d local + %d base commits)",
> +		    pos, g->num_commits, g->num_commits_in_base);

Where does 'pos' typically come from? Taken from a parent commit field of a commit-graph file or something like that?

As this is a "BUG()" and not a "die()", the callers of this function are responsible for making sure that, even if they are fed a set of corrupt commit-graph files, they never feed 'pos' that is out of bounds to this function. The same is true for the other BUG() in fill_commit_in_graph().

I am wondering if they have already sufficient protection, or if we are better off having die() instead saying "corrupted commit graph file" or something. I dunno.