Re: [PATCH 3/4] load_revindex_from_disk(): avoid accessing uninitialized data
From: Taylor Blau <hidden>
Date: 2025-03-27 14:23:11
On Thu, Mar 27, 2025 at 12:43:48PM +0000, Johannes Schindelin via GitGitGadget wrote:
quoted hunk ↗ jump to hunk
From: Johannes Schindelin <redacted> 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 <redacted> --- 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