{"thread":{"id":"63205","subject":"[PATCH 0/4] Initialize a few uninitialized variables","startedAt":"2025-03-27T12:43:51Z","lastAt":"2025-07-17T16:45:13Z","messageCount":10,"participants":["Johannes Schindelin via GitGitGadget","Taylor Blau","Jeff King","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"515197","messageId":"pull.1888.git.1743079429.gitgitgadget@gmail.com","threadId":"63205","inReplyTo":null,"subject":"[PATCH 0/4] Initialize a few uninitialized variables","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-27T12:43:45Z","receivedAt":"2025-03-27T12:43:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When I ran CodeQL on Git's source code, it said that that variables might be\nuninitialized in a few places.\n\nJohannes Schindelin (4):\n cat_one_file(): make it easy to see that the `size` variable is\n initialized\n fsck: avoid using an uninitialized variable\n load_revindex_from_disk(): avoid accessing uninitialized data\n load_pack_mtimes_file(): avoid accessing uninitialized data\n\n builtin/cat-file.c | 2 +-\n fsck.c | 2 +-\n pack-mtimes.c | 2 +-\n pack-revindex.c | 2 +-\n 4 files changed, 4 insertions(+), 4 deletions(-)\n\n\nbase-commit: 683c54c999c301c2cd6f715c411407c413b1d84e\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1888%2Fdscho%2Funinitialized-variables-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1888/dscho/uninitialized-variables-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1888\n-- \ngitgitgadget\n"},{"id":"515198","messageId":"b5e98d5e144f4b3a2771a421690b4b729dc1b7ae.1743079429.git.gitgitgadget@gmail.com","threadId":"63205","inReplyTo":"pull.1888.git.1743079429.gitgitgadget@gmail.com","subject":"[PATCH 1/4] cat_one_file(): make it easy to see that the `size` variable is initialized","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-27T12:43:46Z","receivedAt":"2025-03-27T12:43:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe large `switch` statement makes it a bit impractical to reason about\nthe code.\n\nOne of the code paths can technically lead to using `size` without being\ninitialized: if the `t` case is taken and the type name is set to the\nempty string, we would actually leave `size` unintialized right until we\nuse it.\n\nPractically, this cannot happen because the\n`do_oid_object_info_extended()` function is expected to always populate\nthe `type_name` if asked for. However, it is quite unnecessary to leave\nthe code as unwieldy to reason about: Just initialize the variable to 0\nand be done with it.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/cat-file.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex b13561cf73b..128c901fa8e 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -104,7 +104,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,\n \tstruct object_id oid;\n \tenum object_type type;\n \tchar *buf;\n-\tunsigned long size;\n+\tunsigned long size = 0;\n \tstruct object_context obj_context = {0};\n \tstruct object_info oi = OBJECT_INFO_INIT;\n \tstruct strbuf sb = STRBUF_INIT;\n-- \ngitgitgadget\n\n"},{"id":"515199","messageId":"575b4b2c2b8a5e800bb65b99f1fcdd6aaae63f94.1743079429.git.gitgitgadget@gmail.com","threadId":"63205","inReplyTo":"pull.1888.git.1743079429.gitgitgadget@gmail.com","subject":"[PATCH 2/4] fsck: avoid using an uninitialized variable","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-27T12:43:47Z","receivedAt":"2025-03-27T12:43:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn `fsck_commit()`, after counting the authors of a commit, we set the\n`err` variable either when there was no author, or when there were more\nthan two authors recorded. Then we access the `err` variable to figure\nout whether we should return early. But if there was exactly one author,\nthat variable is still uninitialized.\n\nLet's just initialize the variable.\n\nThis issue was pointed out by CodeQL.\n\nSigned-off-by: Johannes Schindelin \n---\n fsck.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 9fc4c25ffd5..ad04b24ff13 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -925,7 +925,7 @@ static int fsck_commit(const struct object_id *oid,\n {\n \tstruct object_id tree_oid, parent_oid;\n \tunsigned author_count;\n-\tint err;\n+\tint err = 0;\n \tconst char *buffer_begin = buffer;\n \tconst char *buffer_end = buffer + size;\n \tconst char *p;\n-- \ngitgitgadget\n\n"},{"id":"515200","messageId":"b9901920de20ae29bd55bd68dab37a737867593b.1743079429.git.gitgitgadget@gmail.com","threadId":"63205","inReplyTo":"pull.1888.git.1743079429.gitgitgadget@gmail.com","subject":"[PATCH 3/4] load_revindex_from_disk(): avoid accessing uninitialized data","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-27T12:43:48Z","receivedAt":"2025-03-27T12:43:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe `revindex_size` value is uninitialized in case the function is\nerroring out, but we want to assign its value. Let's just initialize it.\n\nSigned-off-by: Johannes Schindelin \n---\n pack-revindex.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/pack-revindex.c b/pack-revindex.c\nindex d3832478d99..3b007d771b3 100644\n--- a/pack-revindex.c\n+++ b/pack-revindex.c\n@@ -208,7 +208,7 @@ static int load_revindex_from_disk(char *revindex_name,\n \tint fd, ret = 0;\n \tstruct stat st;\n \tvoid *data = NULL;\n-\tsize_t revindex_size;\n+\tsize_t revindex_size = 0;\n \tstruct revindex_header *hdr;\n \n \tif (git_env_bool(GIT_TEST_REV_INDEX_DIE_ON_DISK, 0))\n-- \ngitgitgadget\n\n"},{"id":"515201","messageId":"d630e95b9672e6b4b3114c077f2995c5170a6a8f.1743079429.git.gitgitgadget@gmail.com","threadId":"63205","inReplyTo":"pull.1888.git.1743079429.gitgitgadget@gmail.com","subject":"[PATCH 4/4] load_pack_mtimes_file(): avoid accessing uninitialized data","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-27T12:43:49Z","receivedAt":"2025-03-27T12:43:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe `mtimes_size` variable is uninitialzed when the function errors out,\nyet its value is assigned to another variable. Let's just initialize it.\n\nSigned-off-by: Johannes Schindelin \n---\n pack-mtimes.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/pack-mtimes.c b/pack-mtimes.c\nindex cdf30b8d2b0..c1f531d45a0 100644\n--- a/pack-mtimes.c\n+++ b/pack-mtimes.c\n@@ -29,7 +29,7 @@ static int load_pack_mtimes_file(char *mtimes_file,\n \tint fd, ret = 0;\n \tstruct stat st;\n \tuint32_t *data = NULL;\n-\tsize_t mtimes_size, expected_size;\n+\tsize_t mtimes_size = 0, expected_size;\n \tstruct mtimes_header header;\n \n \tfd = git_open(mtimes_file);\n-- \ngitgitgadget\n"},{"id":"515202","messageId":"Z+VfTUrQe1x8DO+h@nand.local","threadId":"63205","inReplyTo":"b9901920de20ae29bd55bd68dab37a737867593b.1743079429.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/4] load_revindex_from_disk(): avoid accessing uninitialized data","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-27T14:23:09Z","receivedAt":"2025-03-27T14:23:11Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Mar 27, 2025 at 12:43:48PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n>\n> The `revindex_size` value is uninitialized in case the function is\n> erroring out, but we want to assign its value. Let's just initialize it.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> pack-revindex.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/pack-revindex.c b/pack-revindex.c\n> index d3832478d99..3b007d771b3 100644\n> --- a/pack-revindex.c\n> +++ b/pack-revindex.c\n> @@ -208,7 +208,7 @@ static int load_revindex_from_disk(char *revindex_name,\n> \tint fd, ret = 0;\n> \tstruct stat st;\n> \tvoid *data = NULL;\n> -\tsize_t revindex_size;\n> +\tsize_t revindex_size = 0;\n\nI'm certainly not opposed to initializing variables proactively, but in\nthis particular case I don't think it's necessary.\n\nWe assign 'revindex_size' out to 'len_p' when we enter the cleanup\nroutine label if 'ret' is zero. We'll use 'revindex_size' in the same\nlabel to munmap() when 'ret' is non-zero, but only if 'data' is also\ninitialized.\n\nSo there are two conditions where we'll enter the cleanup label before\nassigning 'revindex_size', when git_open() returns a negative value, or\nfstat()ing the descriptor that git_open() gave us returns a non-zero\nvalue. In both of those cases, ret is non-zero (it is assigned to 1 and\nthe return value of error_errno() in those cases, respectively). Since\n'data' is also NULL here, this function will terminate without using\nthe uninitialized 'revindex_size'.\n\nIf both of those work (i.e., we opened the file and fstat()ed it\nsuccessfully), then we'll have revindex_size initialized to st.st_size\n(really the result of calling xsize_t() on it). There are two sanity\nchecks on the size, both of which happen before we have mmap()ed the\nfile, and both sanity checks set 'ret' to a non-zero value upon failure.\n\nSo by the time we '*len_p = revindex_size' it is guaranteed to be\ninitialized and just junk bytes on the stack.\n\nDid this trigger a warning from a static analyzer or something? If so,\nI'm happy to take this patch to appease it. Perhaps that it what's going\non since I recall you mentioning that you were working on enabling\nCodeQL in Microsoft's fork of Git.\n\nBut if not I might suggest dropping this patch for the reasons above.\n\nThanks,\nTaylor\n"},{"id":"515203","messageId":"Z+VfmZ4RZ5OjU/ed@nand.local","threadId":"63205","inReplyTo":"d630e95b9672e6b4b3114c077f2995c5170a6a8f.1743079429.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/4] load_pack_mtimes_file(): avoid accessing uninitialized data","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-27T14:24:25Z","receivedAt":"2025-03-27T14:24:27Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Mar 27, 2025 at 12:43:49PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n>\n> The `mtimes_size` variable is uninitialzed when the function errors out,\n> yet its value is assigned to another variable. Let's just initialize it.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> pack-mtimes.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/pack-mtimes.c b/pack-mtimes.c\n> index cdf30b8d2b0..c1f531d45a0 100644\n> --- a/pack-mtimes.c\n> +++ b/pack-mtimes.c\n> @@ -29,7 +29,7 @@ static int load_pack_mtimes_file(char *mtimes_file,\n> \tint fd, ret = 0;\n> \tstruct stat st;\n> \tuint32_t *data = NULL;\n> -\tsize_t mtimes_size, expected_size;\n> +\tsize_t mtimes_size = 0, expected_size;\n\nHmm. This one follows an identical line of reasoning as in my previous\nresponse in the thread. So I think this one is likewise unnecessary\n(though not harmful, and certainly useful if it appeases static analysis\ntools, etc).\n\nThanks,\nTaylor\n"},{"id":"515212","messageId":"20250328034628.GA3067273@coredump.intra.peff.net","threadId":"63205","inReplyTo":"b5e98d5e144f4b3a2771a421690b4b729dc1b7ae.1743079429.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/4] cat_one_file(): make it easy to see that the `size` variable is initialized","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-03-28T03:46:28Z","receivedAt":"2025-03-28T03:46:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 27, 2025 at 12:43:46PM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin \n> \n> The large `switch` statement makes it a bit impractical to reason about\n> the code.\n> \n> One of the code paths can technically lead to using `size` without being\n> initialized: if the `t` case is taken and the type name is set to the\n> empty string, we would actually leave `size` unintialized right until we\n> use it.\n\nI don't think that's quite true. If we have an empty type name we leave\nthe switch and hit these lines:\n\n\tif (!buf)\n\t\tdie(\"git cat-file %s: bad file\", obj_name);\n\n\twrite_or_die(1, buf, size);\n\nSince we set buf to NULL before the switch and never touch it in the 't'\ncase, we'll always hit that die() call.\n\nSo this really is a false positive, regardless of what happens to the\ntype name buffer. I'm a little surprised that CodeQL would get this\nwrong, just because it is very easy to see that buf is not touched in\nthe 't' case at all (and thus must be NULL). But maybe I'm missing\nsomething.\n\nI do agree that the flow through the switch statement (where \"break\" is\ngood for some cases and a failure mode for others) makes this code\nrather hard to reason about. I'm sure it could be rewritten, but I'm not\nsure if it's worth spending time on.\n\n> Practically, this cannot happen because the\n> `do_oid_object_info_extended()` function is expected to always populate\n> the `type_name` if asked for. However, it is quite unnecessary to leave\n> the code as unwieldy to reason about: Just initialize the variable to 0\n> and be done with it.\n\nYou can trigger the path in question like this:\n\n oid=$(echo foo | git hash-object --literally --stdin -w -t '')\n git cat-file --allow-unknown -t $oid\n\nwhich hits the \"bad file\" message.\n\n(Obviously the above is horrible and arguably something we should\nconsider forbidding; I have some patches moving towards ripping out\nsupport for non-standard types entirely).\n\n> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n> index b13561cf73b..128c901fa8e 100644\n> --- a/builtin/cat-file.c\n> +++ b/builtin/cat-file.c\n> @@ -104,7 +104,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,\n> \tstruct object_id oid;\n> \tenum object_type type;\n> \tchar *buf;\n> -\tunsigned long size;\n> +\tunsigned long size = 0;\n\nSo even though I think your analysis above had a few wrong details, I do\nagree this is a false positive in CodeQL and is probably OK to fix as\nyou do here. Though it might make more sense to do it alongside the\nassignment to \"buf\" (or to move the initialization of \"buf\" up here).\n\n-Peff\n"},{"id":"515213","messageId":"20250328040745.GB3067273@coredump.intra.peff.net","threadId":"63205","inReplyTo":"575b4b2c2b8a5e800bb65b99f1fcdd6aaae63f94.1743079429.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/4] fsck: avoid using an uninitialized variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-03-28T04:07:45Z","receivedAt":"2025-03-28T04:07:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 27, 2025 at 12:43:47PM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin \n> \n> In `fsck_commit()`, after counting the authors of a commit, we set the\n> `err` variable either when there was no author, or when there were more\n> than two authors recorded. Then we access the `err` variable to figure\n> out whether we should return early. But if there was exactly one author,\n> that variable is still uninitialized.\n> \n> Let's just initialize the variable.\n> \n> This issue was pointed out by CodeQL.\n\nHmm, I'd think we would hit this case all the time, since commits\ngenerally have one author. But I think it's another false positive.\n\nThe code in question is this:\n\n author_count = 0;\n while (buffer < buffer_end && skip_prefix(buffer, \"author \", &buffer)) {\n author_count++;\n err = fsck_ident(&buffer, oid, OBJ_COMMIT, options);\n if (err)\n return err;\n }\n if (author_count < 1)\n err = report(options, oid, OBJ_COMMIT, FSCK_MSG_MISSING_AUTHOR, \"invalid format - expected 'author' line\");\n else if (author_count > 1)\n err = report(options, oid, OBJ_COMMIT, FSCK_MSG_MULTIPLE_AUTHORS, \"invalid format - multiple 'author' lines\");\n if (err)\n return err;\n\nSo we set \"err\" as soon as we find _any_ author (when we check whether\nit is properly formatted via fsck_ident). And author_count will not be\nincremented if we did not find one. So either we must have assigned\nthe result of fsck_ident(), or we will hit the \"author_count < 1\" case\nand assign there.\n\nIt's certainly confusing, though, since \"err\" gets used in so many\nspots. I think the whole thing would be easier to understand if we had\ntighter-scoped single use variables like this:\n\ndiff --git a/fsck.c b/fsck.c\nindex 9fc4c25ffd..ea72b3247d 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -925,7 +925,6 @@ static int fsck_commit(const struct object_id *oid,\n {\n \tstruct object_id tree_oid, parent_oid;\n \tunsigned author_count;\n-\tint err;\n \tconst char *buffer_begin = buffer;\n \tconst char *buffer_end = buffer + size;\n \tconst char *p;\n@@ -941,39 +940,44 @@ static int fsck_commit(const struct object_id *oid,\n \tif (buffer >= buffer_end || !skip_prefix(buffer, \"tree \", &buffer))\n \t\treturn report(options, oid, OBJ_COMMIT, FSCK_MSG_MISSING_TREE, \"invalid format - expected 'tree' line\");\n \tif (parse_oid_hex(buffer, &tree_oid, &p) || *p != '\\n') {\n-\t\terr = report(options, oid, OBJ_COMMIT, FSCK_MSG_BAD_TREE_SHA1, \"invalid 'tree' line format - bad sha1\");\n+\t\tint err = report(options, oid, OBJ_COMMIT, FSCK_MSG_BAD_TREE_SHA1, \"invalid 'tree' line format - bad sha1\");\n \t\tif (err)\n \t\t\treturn err;\n \t}\n \tbuffer = p + 1;\n \twhile (buffer < buffer_end && skip_prefix(buffer, \"parent \", &buffer)) {\n \t\tif (parse_oid_hex(buffer, &parent_oid, &p) || *p != '\\n') {\n-\t\t\terr = report(options, oid, OBJ_COMMIT, FSCK_MSG_BAD_PARENT_SHA1, \"invalid 'parent' line format - bad sha1\");\n+\t\t\tint err = report(options, oid, OBJ_COMMIT, FSCK_MSG_BAD_PARENT_SHA1, \"invalid 'parent' line format - bad sha1\");\n \t\t\tif (err)\n \t\t\t\treturn err;\n \t\t}\n \t\tbuffer = p + 1;\n \t}\n \tauthor_count = 0;\n \twhile (buffer < buffer_end && skip_prefix(buffer, \"author \", &buffer)) {\n+\t\tint err = fsck_ident(&buffer, oid, OBJ_COMMIT, options);\n+\t\tif (err)\n+\t\t\treturn err;\n \t\tauthor_count++;\n-\t\terr = fsck_ident(&buffer, oid, OBJ_COMMIT, options);\n+\t}\n+\tif (author_count < 1) {\n+\t\tint err = report(options, oid, OBJ_COMMIT, FSCK_MSG_MISSING_AUTHOR, \"invalid format - expected 'author' line\");\n+\t\tif (err)\n+\t\t\treturn err;\n+\t} else if (author_count > 1) {\n+\t\tint err = report(options, oid, OBJ_COMMIT, FSCK_MSG_MULTIPLE_AUTHORS, \"invalid format - multiple 'author' lines\");\n \t\tif (err)\n \t\t\treturn err;\n \t}\n-\tif (author_count < 1)\n-\t\terr = report(options, oid, OBJ_COMMIT, FSCK_MSG_MISSING_AUTHOR, \"invalid format - expected 'author' line\");\n-\telse if (author_count > 1)\n-\t\terr = report(options, oid, OBJ_COMMIT, FSCK_MSG_MULTIPLE_AUTHORS, \"invalid format - multiple 'author' lines\");\n-\tif (err)\n-\t\treturn err;\n \tif (buffer >= buffer_end || !skip_prefix(buffer, \"committer \", &buffer))\n \t\treturn report(options, oid, OBJ_COMMIT, FSCK_MSG_MISSING_COMMITTER, \"invalid format - expected 'committer' line\");\n-\terr = fsck_ident(&buffer, oid, OBJ_COMMIT, options);\n-\tif (err)\n-\t\treturn err;\n+\telse {\n+\t\tint err = fsck_ident(&buffer, oid, OBJ_COMMIT, options);\n+\t\tif (err)\n+\t\t\treturn err;\n+\t}\n \tif (memchr(buffer_begin, '\\0', size)) {\n-\t\terr = report(options, oid, OBJ_COMMIT, FSCK_MSG_NUL_IN_COMMIT,\n+\t\tint err = report(options, oid, OBJ_COMMIT, FSCK_MSG_NUL_IN_COMMIT,\n \t\t\t \"NUL byte in the commit object body\");\n \t\tif (err)\n \t\t\treturn err;\n\nAnd then it is obvious that the general pattern is to propagate \"err\"\nfrom individual calls (and the ones that do not stick out like sore\nthumbs; are those bugs where we should keep going if the user set those\nmessage types to warn/ignore?).\n\nYou could even wrap the pattern in a macro, though perhaps that is\ngetting too magical. The resulting logic is easier to follow, though, if\nyou can look past the macro:\n\ndiff --git a/fsck.c b/fsck.c\nindex ea72b3247d..8c7ac3c448 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -919,6 +919,12 @@ static int fsck_ident(const char **ident,\n \treturn 0;\n }\n \n+#define MAYBE_RETURN(x) do { \\\n+\tint err = (x); \\\n+\tif (err) \\\n+\t\treturn err; \\\n+} while (0)\n+\n static int fsck_commit(const struct object_id *oid,\n \t\t const char *buffer, unsigned long size,\n \t\t struct fsck_options *options)\n@@ -939,49 +945,30 @@ static int fsck_commit(const struct object_id *oid,\n \n \tif (buffer >= buffer_end || !skip_prefix(buffer, \"tree \", &buffer))\n \t\treturn report(options, oid, OBJ_COMMIT, FSCK_MSG_MISSING_TREE, \"invalid format - expected 'tree' line\");\n-\tif (parse_oid_hex(buffer, &tree_oid, &p) || *p != '\\n') {\n-\t\tint err = report(options, oid, OBJ_COMMIT, FSCK_MSG_BAD_TREE_SHA1, \"invalid 'tree' line format - bad sha1\");\n-\t\tif (err)\n-\t\t\treturn err;\n-\t}\n+\tif (parse_oid_hex(buffer, &tree_oid, &p) || *p != '\\n')\n+\t\tMAYBE_RETURN(report(options, oid, OBJ_COMMIT, FSCK_MSG_BAD_TREE_SHA1, \"invalid 'tree' line format - bad sha1\"));\n \tbuffer = p + 1;\n \twhile (buffer < buffer_end && skip_prefix(buffer, \"parent \", &buffer)) {\n-\t\tif (parse_oid_hex(buffer, &parent_oid, &p) || *p != '\\n') {\n-\t\t\tint err = report(options, oid, OBJ_COMMIT, FSCK_MSG_BAD_PARENT_SHA1, \"invalid 'parent' line format - bad sha1\");\n-\t\t\tif (err)\n-\t\t\t\treturn err;\n-\t\t}\n+\t\tif (parse_oid_hex(buffer, &parent_oid, &p) || *p != '\\n')\n+\t\t\tMAYBE_RETURN(report(options, oid, OBJ_COMMIT, FSCK_MSG_BAD_PARENT_SHA1, \"invalid 'parent' line format - bad sha1\"));\n \t\tbuffer = p + 1;\n \t}\n \tauthor_count = 0;\n \twhile (buffer < buffer_end && skip_prefix(buffer, \"author \", &buffer)) {\n-\t\tint err = fsck_ident(&buffer, oid, OBJ_COMMIT, options);\n-\t\tif (err)\n-\t\t\treturn err;\n+\t\tMAYBE_RETURN(fsck_ident(&buffer, oid, OBJ_COMMIT, options));\n \t\tauthor_count++;\n \t}\n-\tif (author_count < 1) {\n-\t\tint err = report(options, oid, OBJ_COMMIT, FSCK_MSG_MISSING_AUTHOR, \"invalid format - expected 'author' line\");\n-\t\tif (err)\n-\t\t\treturn err;\n-\t} else if (author_count > 1) {\n-\t\tint err = report(options, oid, OBJ_COMMIT, FSCK_MSG_MULTIPLE_AUTHORS, \"invalid format - multiple 'author' lines\");\n-\t\tif (err)\n-\t\t\treturn err;\n-\t}\n+\tif (author_count < 1)\n+\t\tMAYBE_RETURN(report(options, oid, OBJ_COMMIT, FSCK_MSG_MISSING_AUTHOR, \"invalid format - expected 'author' line\"));\n+\telse if (author_count > 1)\n+\t\tMAYBE_RETURN(report(options, oid, OBJ_COMMIT, FSCK_MSG_MULTIPLE_AUTHORS, \"invalid format - multiple 'author' lines\"));\n \tif (buffer >= buffer_end || !skip_prefix(buffer, \"committer \", &buffer))\n \t\treturn report(options, oid, OBJ_COMMIT, FSCK_MSG_MISSING_COMMITTER, \"invalid format - expected 'committer' line\");\n-\telse {\n-\t\tint err = fsck_ident(&buffer, oid, OBJ_COMMIT, options);\n-\t\tif (err)\n-\t\t\treturn err;\n-\t}\n-\tif (memchr(buffer_begin, '\\0', size)) {\n-\t\tint err = report(options, oid, OBJ_COMMIT, FSCK_MSG_NUL_IN_COMMIT,\n-\t\t\t \"NUL byte in the commit object body\");\n-\t\tif (err)\n-\t\t\treturn err;\n-\t}\n+\telse\n+\t\tMAYBE_RETURN(fsck_ident(&buffer, oid, OBJ_COMMIT, options));\n+\n+\tif (memchr(buffer_begin, '\\0', size))\n+\t\tMAYBE_RETURN(report(options, oid, OBJ_COMMIT, FSCK_MSG_NUL_IN_COMMIT, \"NUL byte in the commit object body\"));\n \treturn 0;\n }\n \n\nI'd suspect that just the first patch above would fix the CodeQL issue.\nIt's certainly a larger diff, but IMHO the result is less confusing for\nhumans, too.\n\n-Peff\n"},{"id":"522215","messageId":"819d0360-5e99-b2ac-5bc3-50bf9bf95662@gmx.de","threadId":"63205","inReplyTo":"pull.1888.git.1743079429.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/4] Initialize a few uninitialized variables","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2025-07-17T16:45:09Z","receivedAt":"2025-07-17T16:45:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 27 Mar 2025, Johannes Schindelin via GitGitGadget wrote:\n\n> When I ran CodeQL on Git's source code, it said that that variables might be\n> uninitialized in a few places.\n\nFor the record, I am abandoning my efforts to upstream this. I was never\ninterested in the actual changes, I was interested in getting CodeQL to\nrun on Git's code base without reporting false positives, so that I could\ncompare the quality of the reports against Coverity's. Therefore, I was\nnot really prepared to polish these patches as if they were something\nimportant: They are not, and I cannot justify spending any more time on\nthese patches. I will carry them as-are in microsoft/git, under the label\n'uninitialized-variables', as long as they apply without major merge\nconflict headaches.\n\nCiao,\nJohannes\n"}]}