Re: [PATCH 05/10] git_inflate(): skip zlib_post_call() sanity check on Z_NEED_DICT

From: Jeff King <peff@peff.net>
Date: Mar 4, 2025, 07:08 UTC
Message-ID: <20250304070818.GA1283943@coredump.intra.peff.net>
In-Reply-To: <Z8EDxd3f/U+EdoLO@nand.local>

On Thu, Feb 27, 2025 at 07:31:01PM -0500, Taylor Blau wrote:

Show 29 quoted lines

> It's been a few weeks since I last looked at this patch, but I have a
> vague recollection that we chose the second approach while discussing
> this together.
> 
> And indeed, looking at the copy I have from that session, it looks like
> we did
> 
> --- 8< ---
> diff --git a/git-zlib.c b/git-zlib.c
> index 651dd9e07c..265d3074e1 100644
> --- a/git-zlib.c
> +++ b/git-zlib.c
> @@ -122,6 +122,8 @@ int git_inflate(git_zstream *strm, int flush)
>                                  ? 0 : flush);
>                 if (status == Z_MEM_ERROR)
>                         die("inflate: out of memory");
> +               if (status == Z_NEED_DICT)
> +                       break;
>                 zlib_post_call(strm);
> 
>                 /*
> --- >8 ---
> 
> I actually have a vague preference towards that approach, since I too do
> not anticipate that we'd ever need/want to support Z_NEED_DICT (and if
> we did, we could always change from option (2) to (3) later on).
> Likewise, the resulting diff is considerably smaller by line count,
> though on the other hand this diff is not all that more complicated
> overall.

Yes, that was definitely the patch we initially wrote. And I do agree that it's pretty unlikely that we'd use Z_NEED_DICT ever. But what I really disliked is that it leaves the git_zstream in a totally broken state, unsynced with the underlying z_stream.

If the caller takes it as a hard error and immediately throws away the stream that's not too bad. Although even then it gets a little weird: we have to call git_inflated_end(), which itself does the usual zlib pre/post calls. The "pre" call overwrites the zstream with old values, which zlib's inflateEnd() then operates on.

It seems to be OK in practice, but we are making assumptions about which parts of the struct zlib cares about. I guess it's unlikely that unexpectedly rewinding total_in would cause zlib to misbehave, but it's probably better not to be too intimate with it.

And of course if somebody ever does want to play with Z_NEED_DICT, it's a bit of a booby trap we've left for them. We know today that they will need to change from option (2) to (3), but in that hypothetical future they will have to first figure out why their z_stream is corrupted. ;)

-Peff