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

From: Taylor Blau <me@ttaylorr.com>
Date: Mar 27, 2025, 14:23 UTC
Message-ID: <Z+VfTUrQe1x8DO+h@nand.local>
In-Reply-To: <b9901920de20ae29bd55bd68dab37a737867593b.1743079429.git.gitgitgadget@gmail.com>

On Thu, Mar 27, 2025 at 12:43:48PM +0000, Johannes Schindelin via GitGitGadget wrote:

Show 20 quoted lines

> 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