From: Marc Strapetz Date: Mon, 10 Jan 2022 23:22:04 GMT Subject: Re: [PATCH] packfile: avoid overflowing shift during decode Message-ID: <5ab9257c-9eba-a171-86d6-3fe7d3a4faec@syntevo.com> In-Reply-To: On 11/11/2021 02:58, Junio C Hamano wrote: > Jonathan Tan writes: > >> diff --git a/packfile.c b/packfile.c >> index 89402cfc69..972c327e29 100644 >> --- a/packfile.c >> +++ b/packfile.c >> @@ -1068,7 +1068,7 @@ unsigned long unpack_object_header_buffer(const unsigned char *buf, >> size = c & 15; >> shift = 4; >> while (c & 0x80) { >> - if (len <= used || bitsizeof(long) <= shift) { >> + if (len <= used || (bitsizeof(long) - 7) <= shift) { This seems to cause troubles now for 32-bit systems (in my case Git for Windows 32-Bit): `shift` will go through 4, 11, 18 and for 25 it finally errors out. This means that objects >= 32MB can't be processed anymore. The condition should probably be changed to: + if (len <= used || (bitsizeof(long) - 7) < shift) { This still ensures that the shift can never overflow and on 32-bit systems restores the maximum size of 4G with a final shift of 127<<25 (the old condition `bitsizeof(long) <= shift` was perfectly valid for 32-bit systems). > Even when we have a packfile that is in a good shape, the current > machine (especially its wordsize) may not be capable of extracting > the object we are looking at from the packstream, when the object is > larger than the current machine's ulong can express. So it may be > an indication that your machine cannot use the packed object, but > may not be an indication that there is anything _wrong_ in the > object header. I was actually quite confused by the error message "bad object header". It made me investigate all sorts of other things before thoroughly stepping through this loop. -Marc