Re: [PATCH 3/9] Convert unpack-objects to size_t

From: Martin Koegler <martin.koegler@chello.at>
Date: Aug 13, 2017, 18:25 UTC
Message-ID: <20170813182552.GA31079@mail.zuhause>
In-Reply-To: <CAN0heSpViT-Nz8OO7Bww3Fbm=mzA6wvm0EfgfRjsz+cVfvu3Yw@mail.gmail.com>

On Sat, Aug 12, 2017 at 04:07:50PM +0200, Martin Ågren wrote:

Show 10 quoted lines

> On 12 August 2017 at 10:47, Martin Koegler <martin.koegler@chello.at> wrote:
> "size" is handed over to a "git_zstream" and goes through zlib.c,
> eventually ending up in zlib, which is outside Git's control, and which
> seems to work with "uLong"s. How do these kind of changes interact with
> zlib? For example, I wonder about this line further down in get_data:
> 
> if (stream.total_out == size && ret == Z_STREAM_END)
> 
> If total_out isn't converted, I guess this would never hit if "size" is
> too large. And if total_out /is/ converted, I guess we'd risk truncation

I posted a patch changing git_zstream.

Show 8 quoted lines

> in zlib_pre_call in zlib.c. Maybe that might cause Git and zlib to have
> different ideas about how much data is available and/or should be
> processed. Maybe we could then hit things like this in git.c:
> 
> if (s->z.total_out != s->total_out + bytes_produced)
>         die("BUG: total_out mismatch");
> 
> I am not very familiar with zlib, so apologies if this is just noise...

You are right, if sizeof(size_t) != sizeof(unsigned long), there can be truncations.

Currently, an object size is read/passed as ulong including to the memory allocation functions. (x)malloc & Co take a length - so the whole GIT code might assume a larger object size than the memory allocation functions. Migrating everything to size_t means, that we move the truncation locations to places, where values enter/leave GIT.

My patches are just a starting point to fix the size handling. They concentrate of fixing data types - not avoiding any possible overflow. Merging them will already be a challenging task, because they touch many functions and will likely conflict with other changes (eg. moving functions).

Regards, Martin