Re: [PATCH 02/20] packfile.c: prevent overflow in `load_idx()`

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jul 13, 2023, 08:21 UTC
Message-ID: <5d2cf09f-34c7-9a88-bab2-8bf348dd13bb@gmail.com>
In-Reply-To: <d6902cd9e7f7f2a6b8044c8fb782a28c23e15600.1689205042.git.me@ttaylorr.com>

Hi Taylor

On 13/07/2023 00:37, Taylor Blau wrote:

Show 19 quoted lines

> Prevent an overflow when locating a pack's CRC offset when the number
> of packed items is greater than 2^32-1/hashsz by guarding the
> computation with an `st_mult()`.
> 
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>   packfile.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/packfile.c b/packfile.c
> index 89220f0e03..70acf1694b 100644
> --- a/packfile.c
> +++ b/packfile.c
> @@ -186,7 +186,7 @@ int load_idx(const char *path, const unsigned int hashsz, void *idx_map,
>   		     */
>   		    (sizeof(off_t) <= 4))
>   			return error("pack too large for current definition of off_t in %s", path);
> -		p->crc_offset = 8 + 4 * 256 + nr * hashsz;
> +		p->crc_offset = st_add(8 + 4 * 256, st_mult(nr, hashsz));

p->crc_offset is a uint32_t so we're still prone to truncation here unless we change the crc_offset member of struct packed_git to be a size_t. I haven't checked if the other users of crc_offset would need adjusting if we change its type.

Best Wishes

Phillip

>   	}
>   
>   	p->index_version = version;