Re: [PATCH] Avoid infinite loop in malformed packfiles

["René Scharfe" <l.s.r@web.de>]

Am 23.08.20 um 05:11 schrieb Ori Bernstein:
> In packfile.c:1680, there's an infinite loop that tries to get
> to the base of a packfile. With offset deltas, the offset needs
> to be greater than 0, so it's always walking backwards, and the
> search is guaranteed to terminate.
>
> With reference deltas, there's no check for a cycle in the
> references, so a cyclic reference will cause git to loop
> infinitely, growing the delta_stack infinitely, which will
> cause it to consume all available memory as as a full CPU
> core.

"as as"?  Perhaps "and"?

> This change puts an arbitrary limit of 10,000 on the number
> of iterations we make when chasing down a base commit, to
> prevent looping forever, using all available memory growing
> the delta stack.
>
> Signed-off-by: Ori Bernstein <ori@eigenstate.org>
> ---
>  packfile.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/packfile.c b/packfile.c
> index 6ab5233613..321e002c50 100644
> --- a/packfile.c
> +++ b/packfile.c
> @@ -1633,6 +1633,7 @@ static void write_pack_access_log(struct packed_git *p, off_t obj_offset)
>
>  int do_check_packed_object_crc;
>
> +#define UNPACK_ENTRY_STACK_LIMIT 10000

b5c0cbd8083 (pack-objects: use bitfield for object_entry::depth,
2018-04-14) limited the delta depth for new packs to 4095, so 10000
seems reasonable.  Users with unreasonable packs would need to repack
them with an older version of Git, though.  Not sure if that would
affect anyone in practice.

>  #define UNPACK_ENTRY_STACK_PREALLOC 64

Hmm, setting a hard limit may allow to allocate the whole stack on the,
ehm, stack.  That would get rid of the hybrid stack/heap allocation and
thus simplify the code a bit.  10000 entries with 24 bytes each would be
quite big, though, but that might be OK without recursion.  (And not in
this patch anyway, of course.)

>  struct unpack_entry_stack_ent {
>  	off_t obj_offset;
> @@ -1715,6 +1716,12 @@ void *unpack_entry(struct repository *r, struct packed_git *p, off_t obj_offset,
>  			break;
>  		}
>
> +		if (delta_stack_nr > UNPACK_ENTRY_STACK_LIMIT) {
> +			error("overlong delta chain at offset %jd from %s",
> +			      (uintmax_t)curpos, p->pack_name);
> +			goto out;
> +		}

Other error handlers in this loop set data to NULL.  That's actually
unnecessary because it's NULL to begin with and the loop is exited after
setting it to some other value.  So not doing it here is fine.  (And a
separate cleanup patch could remove the dead stores in the other
handlers.)

> +
>  		/* push object, proceed to base */
>  		if (delta_stack_nr >= delta_stack_alloc
>  		    && delta_stack == small_delta_stack) {
>