"Derrick Stolee via GitGitGadget" <[email protected]> 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?
> + 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.
