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

> > 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