Re: mk/use-size-t-in-zlib [was: Re: What's cooking in git.git (Jan 2019, #05; Tue, 29)]

["Torsten Bögershausen" <tboegi@web.de>]

On Thu, Jan 31, 2019 at 06:59:17PM +0100, Thomas Braun wrote:
> > Junio C Hamano <gitster@pobox.com> hat am 29. Januar 2019 um 23:15 geschrieben:
>
> [...]
>
> > * mk/use-size-t-in-zlib (2018-10-15) 1 commit
> >  - zlib.c: use size_t for size
> >
> >  The wrapper to call into zlib followed our long tradition to use
> >  "unsigned long" for sizes of regions in memory, which have been
> >  updated to use "size_t".
> >
>
> I've started playing around with the patch from Thorsten [1] for getting unsigned long replaced in more places so that you can commit large files on platforms like Windows there unsigned long is 32-bit even on 64-bit OSes.
>
> And the first thing which bugs out when I do a quick test with committing a large file and fsck the repo is in zlib.c:
>
> 	if (s->z.total_out != s->total_out + bytes_produced)
> 		BUG("total_out mismatch");
>
> here s->z.total_out is an unsigned long and s->total_out is size_t and this triggers the BUG message once the unsigned long wraps. There is even an FAQ entry for zlib at [2] which warns about that potential issue.
>
> So I would think that something like
>
> ----------->8
>
> diff --git a/zlib.c b/zlib.c
> index 197a1acc7b..9cc6421eba 100644
> --- a/zlib.c
> +++ b/zlib.c
> @@ -51,13 +51,9 @@ static void zlib_post_call(git_zstream *s)
>
>         bytes_consumed = s->z.next_in - s->next_in;
>         bytes_produced = s->z.next_out - s->next_out;
> -       if (s->z.total_out != s->total_out + bytes_produced)
> -               BUG("total_out mismatch");
> -       if (s->z.total_in != s->total_in + bytes_consumed)
> -               BUG("total_in mismatch");
>
> -       s->total_out = s->z.total_out;
> -       s->total_in = s->z.total_in;
> +       s->total_out += bytes_produced;
> +       s->total_in += bytes_consumed;
>         s->next_in = s->z.next_in;
>         s->next_out = s->z.next_out;
>         s->avail_in -= bytes_consumed;
>
> -----------8<
>
> would make the patch [3] more complete IMHO.
>
> Another potential issue in that patch is that the signature change in git_deflate_bound forces size to unsigned long on the call to deflateBound (for newer zlib versions) and if that conversion is not faithful this will certainly not work.
>
> Just my 2cents I'm not vetoing anything here,
> Thomas

Thanks for digging.

Do you think that you can provide a new version of the patch ?
Or, may be better,  a patch on top of that ?


>
> [1]: http://public-inbox.org/git/20181120050456.16715-1-tboegi@web.de/
> [2]: http://www.zlib.net/zlib_faq.html#faq32
> [3]: http://public-inbox.org/git/20181012204229.11890-1-tboegi@web.de/