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

[Thomas Braun <thomas.braun@virtuell-zuhause.de>]

> 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

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