Re: [PATCH 3/4] load_revindex_from_disk(): avoid accessing uninitialized data

[Taylor Blau <me@ttaylorr.com>]

On Thu, Mar 27, 2025 at 12:43:48PM +0000, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> The `revindex_size` value is uninitialized in case the function is
> erroring out, but we want to assign its value. Let's just initialize it.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  pack-revindex.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/pack-revindex.c b/pack-revindex.c
> index d3832478d99..3b007d771b3 100644
> --- a/pack-revindex.c
> +++ b/pack-revindex.c
> @@ -208,7 +208,7 @@ static int load_revindex_from_disk(char *revindex_name,
>  	int fd, ret = 0;
>  	struct stat st;
>  	void *data = NULL;
> -	size_t revindex_size;
> +	size_t revindex_size = 0;

I'm certainly not opposed to initializing variables proactively, but in
this particular case I don't think it's necessary.

We assign 'revindex_size' out to 'len_p' when we enter the cleanup
routine label if 'ret' is zero. We'll use 'revindex_size' in the same
label to munmap() when 'ret' is non-zero, but only if 'data' is also
initialized.

So there are two conditions where we'll enter the cleanup label before
assigning 'revindex_size', when git_open() returns a negative value, or
fstat()ing the descriptor that git_open() gave us returns a non-zero
value. In both of those cases, ret is non-zero (it is assigned to 1 and
the return value of error_errno() in those cases, respectively). Since
'data' is also NULL here, this function will terminate without using
the uninitialized 'revindex_size'.

If both of those work (i.e., we opened the file and fstat()ed it
successfully), then we'll have revindex_size initialized to st.st_size
(really the result of calling xsize_t() on it). There are two sanity
checks on the size, both of which happen before we have mmap()ed the
file, and both sanity checks set 'ret' to a non-zero value upon failure.

So by the time we '*len_p = revindex_size' it is guaranteed to be
initialized and just junk bytes on the stack.

Did this trigger a warning from a static analyzer or something? If so,
I'm happy to take this patch to appease it. Perhaps that it what's going
on since I recall you mentioning that you were working on enabling
CodeQL in Microsoft's fork of Git.

But if not I might suggest dropping this patch for the reasons above.

Thanks,
Taylor