Re: [PATCH] object-name: reject too-deep recursive ancestor queries

[Junio C Hamano <gitster@pobox.com>]

Taylor Blau <me@ttaylorr.com> writes:

> Since this is a local-only exploit, a user would have to be tricked into
> running such a query by an adversary. Even if they were successfully
> tricked into running the malicious query, the blast radius is limited to
> a local stack overflow, which does not have meaningful paths to remote
> code execution, arbitrary memory reads, or any more grave security
> concerns.
> ...

So the difference in practice is if we make a controlled call to
die() or just let it crash?  It still does sound worthwhile thing to
do to make sure we make a controlled death.  But ...

> +static int get_nth_ancestor_max_depth = 4096;
> +static int get_nth_ancestor_curr_depth;

... do we have a lock at a much higher level that prevents multiple
name-to-oid look-ups from running simultaneously, or something
similar, to make use of this static counter safe?  I am not offhand
sure how safe it is to assume that we'd always be single-threaded.
This variable leaves a bad taste in my mouth.

I am not offhand sure how hard it is to count the depth per
callpath; get_oid_1() is the sole caller of get_nth_ancestor(), so
if you rename the former into a separate helper with a new
"recursion_depth" parameter, create a thin wrapper around it that
starts the recursion at depth 0 and have everybody else (i.e.,
peel_onion() and get_oid_with_context_1()) call it, and have
get_nth_ancestor increment (and die as needed) the counter, would
that be sufficient to ensure that we count the depth per call
invocation?

Thanks.