| Message ID | b9901920de20ae29bd55bd68dab37a737867593b.1743079429.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Initialize a few uninitialized variables | expand |
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
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; struct revindex_header *hdr; if (git_env_bool(GIT_TEST_REV_INDEX_DIE_ON_DISK, 0))