Re: [PATCH] Avoid infinite loop in malformed packfiles

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 24, 2020, 17:33 UTC
Message-ID: <xmqqsgcc54pq.fsf@gitster.c.googlers.com>
In-Reply-To: <20200823031151.10985-1-ori@eigenstate.org>

Ori Bernstein <ori@eigenstate.org> writes:

Show 11 quoted lines

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

Show 8 quoted lines

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