Re: [PATCH] packfile: avoid overflowing shift during decode
- From
Marc Strapetz <marc.strapetz@syntevo.com>
- Date
- Jan 10, 2022, 23:22 UTC
- Message-ID
- <5ab9257c-9eba-a171-86d6-3fe7d3a4faec@syntevo.com>
- In-Reply-To
- <xmqqpmr7v4gf.fsf@gitster.g>
On 11/11/2021 02:58, Junio C Hamano wrote:
Show 12 quoted lines
> Jonathan Tan <jonathantanmy@google.com> 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).
Show 7 quoted lines
> 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