Re: [PATCH v2 07/12] packfile, git-zlib: widen `use_pack()` and zstream avail fields to `size_t`

[jc: Sorry, I hit <SEND> before I was ready]

"Johannes Schindelin via GitGitGadget" <[email protected]>
writes:

> diff --git a/git-zlib.h b/git-zlib.h
> index 44380e8ad3..0b24b15bd0 100644
> --- a/git-zlib.h
> +++ b/git-zlib.h
> @@ -5,8 +5,8 @@
>  
>  typedef struct git_zstream {
>  	struct z_stream_s z;
> -	unsigned long avail_in;
> -	unsigned long avail_out;
> +	size_t avail_in;
> +	size_t avail_out;
>  	size_t total_in;
>  	size_t total_out;
>  	unsigned char *next_in;

We use 'size_t', which means we can use a buffer larger than 4 GB
on systems where 'size_t' is wider than a 32-bit 'unsigned long'.
But these represent the size of a single contiguous buffer, so
I think that is why the log message mentioned that this is more
about type consistency than being able to handle larger data, as
I do not think anyone would reasonably feed a contiguous buffer
larger than 4 GB in one go in practice.  For that reason, two
details stood out to me:

 - zlib_buf_cap() still returns 'unsigned long', and
   zlib_pre_call() feeds these potentially wider values to it.
   Is it possible that we trigger truncation before 'avail_in'
   or 'avail_out' is compared with 'ZLIB_BUF_MAX' in
   zlib_buf_cap()?

 - unpack_object_header_buffer() still takes an 'unsigned long'
   length, while oe_get_size_slow() in 'builtin/pack-objects.c'
   passes a 'size_t' 'avail' to it.  This comes from use_pack(),
   so it is a relatively small value stored in a wider 'size_t',
   but I am unsure whether your static checker would flag this for
   potential truncation.

They are probably harmless in practice, but they are still a bit
concerning from the standpoint of type consistency.