{"thread":{"id":"65832","subject":"[PATCH] zlib: properly clamp to uLong","startedAt":"2026-06-18T13:50:21Z","lastAt":"2026-06-19T07:41:06Z","messageCount":3,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"545848","messageId":"pull.2153.git.1781790619424.gitgitgadget@gmail.com","threadId":"65832","inReplyTo":null,"subject":"[PATCH] zlib: properly clamp to uLong","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-06-18T13:50:18Z","receivedAt":"2026-06-18T13:50:21Z","isPatch":true,"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nOn platforms where `unsigned long` and `size_t` differ in bit size, we\nwant to clamp the buffers we pass to zlib to the former's size, as per\nd05d666977 (git-zlib: handle data streams larger than 4GB, 2026-05-08).\n\nThe logic introduced in that commit performs a clamping to the bits,\nthough, which fails to do what is needed here: If too many bytes are\navailable in the buffers, we need to clamp to the maximum value of an\n`unsigned long`. Otherwise, we ask zlib to use too small buffers, in the\nworst case using 0 as the size (think: a value whose 32 lowest bits are\nall zero).\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n    zlib: properly clamp to uLong\n    \n    I re-read this logic earlier this week... and I am quite convinced that\n    it needs to be fixed.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-2153%2Fdscho%2Ffix-ulong-clamping-for-zlib-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2153/dscho/fix-ulong-clamping-for-zlib-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/2153\n\n git-zlib.c | 13 +++++++++----\n 1 file changed, 9 insertions(+), 4 deletions(-)\n\ndiff --git a/git-zlib.c b/git-zlib.c\nindex b91cb323ae..d21adb3bf5 100644\n--- a/git-zlib.c\n+++ b/git-zlib.c\n@@ -38,12 +38,17 @@ static inline uInt zlib_buf_cap(unsigned long len)\n \treturn (ZLIB_BUF_MAX < len) ? ZLIB_BUF_MAX : len;\n }\n \n+static inline uLong zlib_uLong_cap(size_t s)\n+{\n+\treturn s < ULONG_MAX_VALUE ? (uLong)s : ULONG_MAX_VALUE;\n+}\n+\n static void zlib_pre_call(git_zstream *s)\n {\n \ts->z.next_in = s->next_in;\n \ts->z.next_out = s->next_out;\n-\ts->z.total_in = (uLong)(s->total_in & ULONG_MAX_VALUE);\n-\ts->z.total_out = (uLong)(s->total_out & ULONG_MAX_VALUE);\n+\ts->z.total_in = zlib_uLong_cap(s->total_in);\n+\ts->z.total_out = zlib_uLong_cap(s->total_out);\n \ts->z.avail_in = zlib_buf_cap(s->avail_in);\n \ts->z.avail_out = zlib_buf_cap(s->avail_out);\n }\n@@ -60,7 +65,7 @@ static void zlib_post_call(git_zstream *s, int status)\n \t * We track our own totals and verify only the low bits match.\n \t */\n \tif ((s->z.total_out & ULONG_MAX_VALUE) !=\n-\t    ((s->total_out + bytes_produced) & ULONG_MAX_VALUE))\n+\t    ((zlib_uLong_cap(s->total_out) + bytes_produced) & ULONG_MAX_VALUE))\n \t\tBUG(\"total_out mismatch\");\n \t/*\n \t * zlib does not update total_in when it returns Z_NEED_DICT,\n@@ -68,7 +73,7 @@ static void zlib_post_call(git_zstream *s, int status)\n \t */\n \tif (status != Z_NEED_DICT &&\n \t    (s->z.total_in & ULONG_MAX_VALUE) !=\n-\t    ((s->total_in + bytes_consumed) & ULONG_MAX_VALUE))\n+\t    ((zlib_uLong_cap(s->total_in) + bytes_consumed) & ULONG_MAX_VALUE))\n \t\tBUG(\"total_in mismatch\");\n \n \ts->total_out += bytes_produced;\n\nbase-commit: 7a094d68a27e321a99c8ab6b700909e503904bd9\n-- \ngitgitgadget\n"},{"id":"545870","messageId":"xmqqzf0rrdbp.fsf@gitster.g","threadId":"65832","inReplyTo":"pull.2153.git.1781790619424.gitgitgadget@gmail.com","subject":"Re: [PATCH] zlib: properly clamp to uLong","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-06-18T17:03:06Z","receivedAt":"2026-06-18T17:03:09Z","isPatch":true,"body":"The original in js/objects-larger-than-4gb-on-windows says things\nlike:\n\n+\ts->z.total_in = (uLong)(s->total_in & ULONG_MAX_VALUE);\n+\ts->z.total_out = (uLong)(s->total_out & ULONG_MAX_VALUE);\n\nYour patch ...\n\n> +static inline uLong zlib_uLong_cap(size_t s)\n> +{\n> +\treturn s < ULONG_MAX_VALUE ? (uLong)s : ULONG_MAX_VALUE;\n> +}\n> +\n>  static void zlib_pre_call(git_zstream *s)\n>  {\n>  \ts->z.next_in = s->next_in;\n>  \ts->z.next_out = s->next_out;\n> -\ts->z.total_in = (uLong)(s->total_in & ULONG_MAX_VALUE);\n> -\ts->z.total_out = (uLong)(s->total_out & ULONG_MAX_VALUE);\n> +\ts->z.total_in = zlib_uLong_cap(s->total_in);\n> +\ts->z.total_out = zlib_uLong_cap(s->total_out);\n\n... is an obvious fix for that.\n\n> @@ -60,7 +65,7 @@ static void zlib_post_call(git_zstream *s, int status)\n>  \t * We track our own totals and verify only the low bits match.\n>  \t */\n>  \tif ((s->z.total_out & ULONG_MAX_VALUE) !=\n> -\t    ((s->total_out + bytes_produced) & ULONG_MAX_VALUE))\n> +\t    ((zlib_uLong_cap(s->total_out) + bytes_produced) & ULONG_MAX_VALUE))\n>  \t\tBUG(\"total_out mismatch\");\n\nBecause we now clamp (not \"taking lower bits of\") s->total_out to a\nvalue between 0..4GB and store it in s->z.total_out in pre-call, let\nzlib do its thing that increments s->z.total_out modulo 4GB, and we\nclamp the s->total_out (before incrementing) the same way in\npost_call here, both sides of \"!=\" above even out.  But the comment\nbefore this comparison that claims that \"we ... verify only the low\nbits match\" is a bit off the reality, I suspect.\n\n> @@ -68,7 +73,7 @@ static void zlib_post_call(git_zstream *s, int status)\n>  \t */\n>  \tif (status != Z_NEED_DICT &&\n>  \t    (s->z.total_in & ULONG_MAX_VALUE) !=\n> -\t    ((s->total_in + bytes_consumed) & ULONG_MAX_VALUE))\n> +\t    ((zlib_uLong_cap(s->total_in) + bytes_consumed) & ULONG_MAX_VALUE))\n>  \t\tBUG(\"total_in mismatch\");\n>  \n>  \ts->total_out += bytes_produced;\n>\n> base-commit: 7a094d68a27e321a99c8ab6b700909e503904bd9\n"},{"id":"545924","messageId":"ef345cb3-e54b-65ce-9a41-bde56ea7108f@gmx.de","threadId":"65832","inReplyTo":"xmqqzf0rrdbp.fsf@gitster.g","subject":"Re: [PATCH] zlib: properly clamp to uLong","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2026-06-19T07:41:03Z","receivedAt":"2026-06-19T07:41:06Z","isPatch":true,"body":"Hi Junio,\n\nOn Thu, 18 Jun 2026, Junio C Hamano wrote:\n\n> [...]\n>\n> > @@ -60,7 +65,7 @@ static void zlib_post_call(git_zstream *s, int status)\n> >  \t * We track our own totals and verify only the low bits match.\n> >  \t */\n> >  \tif ((s->z.total_out & ULONG_MAX_VALUE) !=\n> > -\t    ((s->total_out + bytes_produced) & ULONG_MAX_VALUE))\n> > +\t    ((zlib_uLong_cap(s->total_out) + bytes_produced) & ULONG_MAX_VALUE))\n> >  \t\tBUG(\"total_out mismatch\");\n> \n> Because we now clamp (not \"taking lower bits of\") s->total_out to a\n> value between 0..4GB and store it in s->z.total_out in pre-call, let\n> zlib do its thing that increments s->z.total_out modulo 4GB, and we\n> clamp the s->total_out (before incrementing) the same way in post_call\n> here, both sides of \"!=\" above even out.\n\nTechnically, the range is 0..(4GB-1), but yes, that's exactly the idea.\n\nIf we clamped bit-wise, i.e. to the lower bits as is currently done, we\nwould _also_ stay within that range, but we'd restrict the total size\nunnecessarily in most cases (i.e. in all cases where `total_out` isn't one\nless than an exact multiple of 4GB). In the worst case, we'd restrict to 0\nbytes, in which case we would run into an infinite loop because zlib has\nno space to work with and we'd try again and again to whittle away a chunk\nof that large input.\n\n> But the comment before this comparison that claims that \"we ... verify\n> only the low bits match\" is a bit off the reality, I suspect.\n\nI am afraid that the comment is still true. The thing is, we're trying to\ncompare the _real_ `total_out + bytes_produced` to zlib's necessarily\nrestricted `total_out` (we cannot change the data type of that attribute\nof `struct z_stream_s`, it's not ours to change, it'll remain `uLong`\nbecause zlib made the same mistake as Git to choose that imprecise data\ntype for memory size calculations). The sum `total_out + bytes_produced`\nis of type `size_t`, the attribute `s->z.total_out` is of type `uLong`.\n\nTherefore, we still need to clamp bit-wise, as the _real_ `total_out +\nbytes_produced` may very well exceed the maximal value of\n`s->z.total_out`, and the zlib operation will _still_ have produced the\nexpected number of bytes, i.e. that sanity check should _pass_.\n\nIf anything, we _could_ consider dropping that masking of `s->z.total_out`\nto the maximal `unsigned long` value, seeing as `s->z.total_out` _is_ of\nthat data type and therefore cannot reasonably exceed that. But then,\nthere might emerge a zlib variant in the future that recapitulates Git's\neffort to use `size_t` where `size_t` is due, and compiling/linking\nagainst _that_ zlib variant would need this mask, otherwise the sanity\ncheck could fail for completely bogus reasons.\n\nSo: The comment is still correct, even with the adjusted logic.\n\nCiao,\nJohannes\n\n> \n> > @@ -68,7 +73,7 @@ static void zlib_post_call(git_zstream *s, int status)\n> >  \t */\n> >  \tif (status != Z_NEED_DICT &&\n> >  \t    (s->z.total_in & ULONG_MAX_VALUE) !=\n> > -\t    ((s->total_in + bytes_consumed) & ULONG_MAX_VALUE))\n> > +\t    ((zlib_uLong_cap(s->total_in) + bytes_consumed) & ULONG_MAX_VALUE))\n> >  \t\tBUG(\"total_in mismatch\");\n> >  \n> >  \ts->total_out += bytes_produced;\n> >\n> > base-commit: 7a094d68a27e321a99c8ab6b700909e503904bd9\n> \n"}]}