Re: [PATCH] packfile: avoid overflowing shift during decode

[Junio C Hamano <gitster@pobox.com>]

Jonathan Tan <jonathantanmy@google.com> writes:

> situation by dying if the left shift overflows, but this patch is still
> worthwhile as it makes a bad header be reported as a bad header, not a
> fatal left shift overflow.

Hmph, I am not sure if I like to see it put that way, but I think
the problem is not anything new.

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.

    Side note.  I suspect that this should already be the case; you
    can pack a large object whose size do not fit in u32 on a box
    whose ulong is u64, and you wouldn't be able to use such a
    packfile on a box where ulong is u32.  There is no "bad object
    header" in the pack stream per-se, but we cannot use it there.

After all, the reason why we chose to use the varint encoding is
because we do not have to change the file format when we extend
beyond the architectures of prevailing word size of the day.

Having said all that, I think the patch is correct.  We later use
the number "shift" as the shift count like so

	size += (c & 0x7f) << shift;

In order for the uppermost bit from (c & 0x7f) to be still inside
"size", it is not sufficient that "shift" does not exceed the
bit-size of "size".  We need to subtract the bitscanreverse of
(0x7f), which is 7, which is exactly what your patch does.

Looking good.  Thanks.


>  packfile.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> 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) {
>  			error("bad object header");
>  			size = used = 0;
>  			break;