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

From: Derrick Stolee <stolee@gmail.com>
Date: Jun 7, 2019, 12:44 UTC
Message-ID: <4e2263fb-1499-7aae-d8f9-c0f789b83744@gmail.com>
In-Reply-To: <xmqqd0jqqu8p.fsf@gitster-ct.c.googlers.com>

On 6/6/2019 5:28 PM, Junio C Hamano wrote:

Show 16 quoted lines

> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> +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?

This is a good point. I will use 'uint32_t' since the only caller is insert_parent_or_die() which has an unsigned position. I did notice that insert_parent_or_die() uses "uint64_t pos" but its only caller passes a "uint32_t edge_value". The 32-bit value makes more sense because of the built-in limits in the commit-graph format for number of commits. I'll change insert_parent_or_die() to use 32-bits as well.

As for the while loop, it would also be good to rearrange the checks as follows:

        while (g && pos < g->num_commits_in_base)
                g = g->base_graph;

        if (!g)
                BUG("NULL commit-graph");

Show 7 quoted lines

> 
>> +	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?

It comes from the commit-graph file.

Show 8 quoted lines

> 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.

I can replace this with a die() that points to a corrupt commit-graph file. Perhaps "BUG()" made more sense while I was developing the feature and wanted to tell myself why the error condition happened. That doesn't make sense any more now that the feature is working.

Thanks, -Stolee