| Message ID | 994568940.109648.1548957557643@ox.hosteurope.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mk/use-size-t-in-zlib [was: Re: What's cooking in git.git (Jan 2019, #05; Tue, 29)] | expand |
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/
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;