Re: [PATCH] Avoid infinite loop in malformed packfiles

From: René Scharfe <l.s.r@web.de>
Date: Aug 23, 2020, 06:26 UTC
Message-ID: <672843a1-b98c-7567-a078-a2dacd4b7074@web.de>
In-Reply-To: <20200823031151.10985-1-ori@eigenstate.org>

Am 23.08.20 um 05:11 schrieb Ori Bernstein:

Show 10 quoted lines

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

Show 19 quoted lines

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

Show 11 quoted lines

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

Show 5 quoted lines

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