On 6/6/2019 5:28 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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");
>
>> + 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.
> 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
