Re: [PATCH] Avoid infinite loop in malformed packfiles

From: Junio C Hamano <gitster@pobox.com>
To: Ori Bernstein <ori@eigenstate.org>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] Avoid infinite loop in malformed packfiles
Date: Mon, 24 Aug 2020 10:33:21 -0700	[thread overview]
Message-ID: <xmqqsgcc54pq.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <20200823031151.10985-1-ori@eigenstate.org> (Ori Bernstein's message of "Sat, 22 Aug 2020 20:11:52 -0700")

Ori Bernstein <ori@eigenstate.org> writes:

> diff --git a/packfile.c b/packfile.c
> index 6ab5233613..321e002c50 100644
> --- a/packfile.c
> +++ b/packfile.c
> @@ -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);

The "j" length field is not used anywhere in the codebase for
portability concerns, I think.  "d" is for signed, but curpos
is an unsigned off_t.  I think

	"... %"PRIuMAX" from %s", (uintmax_t)curpos, ...

would match how we write this kind of thing everywhere else in the
code, e.g. showing obj_offset in packed_to_object_type() in the same
file in an error message.

> @@ -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
>  #define UNPACK_ENTRY_STACK_PREALLOC 64
>  struct unpack_entry_stack_ent {
>  	off_t obj_offset;

What escape hatch would the end-users have when they have a
legitimate packfile that has a truly deep delta chain, by the way?

Thanks.