Initialize a few uninitialized variables

When I ran CodeQL on Git's source code, it said that that variables might be uninitialized in a few places.

Johannes Schindelin (4):
cat_one_file(): make it easy to see that the size variable is
initialized
fsck: avoid using an uninitialized variable load_revindex_from_disk(): avoid accessing uninitialized data load_pack_mtimes_file(): avoid accessing uninitialized data

 builtin/cat-file.c | 2 +-
 fsck.c             | 2 +-
 pack-mtimes.c      | 2 +-
 pack-revindex.c    | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1888%2Fdscho%2Funinitialized-variables-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1888/dscho/uninitialized-variables-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1888

gitgitgadget

From: Johannes Schindelin <[email protected]>

The large switch statement makes it a bit impractical to reason about the code.

One of the code paths can technically lead to using size without being
initialized: if the t case is taken and the type name is set to the empty string, we would actually leave size unintialized right until we use it.

Practically, this cannot happen because the do_oid_object_info_extended() function is expected to always populate the type_name if asked for. However, it is quite unnecessary to leave the code as unwieldy to reason about: Just initialize the variable to 0 and be done with it.

---
 builtin/cat-file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index b13561cf73b..128c901fa8e 100644
@@ -104,7 +104,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
 	struct object_id oid;
 	enum object_type type;
 	char *buf;
-	unsigned long size;
+	unsigned long size = 0;
 	struct object_context obj_context = {0};
 	struct object_info oi = OBJECT_INFO_INIT;
 	struct strbuf sb = STRBUF_INIT;

gitgitgadget

From: Johannes Schindelin <[email protected]>

In fsck_commit(), after counting the authors of a commit, we set the err variable either when there was no author, or when there were more than two authors recorded. Then we access the err variable to figure out whether we should return early. But if there was exactly one author, that variable is still uninitialized.

Let's just initialize the variable.

This issue was pointed out by CodeQL.

---
 fsck.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 9fc4c25ffd5..ad04b24ff13 100644
@@ -925,7 +925,7 @@ static int fsck_commit(const struct object_id *oid,
 {
 	struct object_id tree_oid, parent_oid;
 	unsigned author_count;
-	int err;
+	int err = 0;
 	const char *buffer_begin = buffer;
 	const char *buffer_end = buffer + size;
 	const char *p;

gitgitgadget

From: Johannes Schindelin <[email protected]>

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.

---
 pack-revindex.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index d3832478d99..3b007d771b3 100644
@@ -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))

gitgitgadget

From: Johannes Schindelin <[email protected]>

The mtimes_size variable is uninitialzed when the function errors out, yet its value is assigned to another variable. Let's just initialize it.

---
 pack-mtimes.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index cdf30b8d2b0..c1f531d45a0 100644
@@ -29,7 +29,7 @@ static int load_pack_mtimes_file(char *mtimes_file,
 	int fd, ret = 0;
 	struct stat st;
 	uint32_t *data = NULL;
-	size_t mtimes_size, expected_size;
+	size_t mtimes_size = 0, expected_size;
 	struct mtimes_header header;
 
 	fd = git_open(mtimes_file);

gitgitgadget

I don't think that's quite true. If we have an empty type name we leave the switch and hit these lines:

	if (!buf)
		die("git cat-file %s: bad file", obj_name);

	write_or_die(1, buf, size);

Since we set buf to NULL before the switch and never touch it in the 't' case, we'll always hit that die() call.

So this really is a false positive, regardless of what happens to the type name buffer. I'm a little surprised that CodeQL would get this wrong, just because it is very easy to see that buf is not touched in the 't' case at all (and thus must be NULL). But maybe I'm missing something.

I do agree that the flow through the switch statement (where "break" is good for some cases and a failure mode for others) makes this code rather hard to reason about. I'm sure it could be rewritten, but I'm not sure if it's worth spending time on.

You can trigger the path in question like this:

  oid=$(echo foo | git hash-object --literally --stdin -w -t '')
  git cat-file --allow-unknown -t $oid

which hits the "bad file" message.

(Obviously the above is horrible and arguably something we should consider forbidding; I have some patches moving towards ripping out support for non-standard types entirely).

Hmm, I'd think we would hit this case all the time, since commits generally have one author. But I think it's another false positive.

The code in question is this:

          author_count = 0;
          while (buffer < buffer_end && skip_prefix(buffer, "author ", &buffer)) {
                  author_count++;
                  err = fsck_ident(&buffer, oid, OBJ_COMMIT, options);
                  if (err)
                          return err;
          }
          if (author_count < 1)
                  err = report(options, oid, OBJ_COMMIT, FSCK_MSG_MISSING_AUTHOR, "invalid format - expected 'author' line");
          else if (author_count > 1)
                  err = report(options, oid, OBJ_COMMIT, FSCK_MSG_MULTIPLE_AUTHORS, "invalid format - multiple 'author' lines");
          if (err)
                  return err;

So we set "err" as soon as we find _any_ author (when we check whether it is properly formatted via fsck_ident). And author_count will not be incremented if we did not find one. So either we must have assigned the result of fsck_ident(), or we will hit the "author_count < 1" case and assign there.

It's certainly confusing, though, since "err" gets used in so many spots. I think the whole thing would be easier to understand if we had tighter-scoped single use variables like this:

index 9fc4c25ffd..ea72b3247d 100644
@@ -925,7 +925,6 @@ static int fsck_commit(const struct object_id *oid,
 {
 	struct object_id tree_oid, parent_oid;
 	unsigned author_count;
-	int err;
 	const char *buffer_begin = buffer;
 	const char *buffer_end = buffer + size;
 	const char *p;

@@ -941,39 +940,44 @@ static int fsck_commit(const struct object_id *oid,
 	if (buffer >= buffer_end || !skip_prefix(buffer, "tree ", &buffer))
 		return report(options, oid, OBJ_COMMIT, FSCK_MSG_MISSING_TREE, "invalid format - expected 'tree' line");
 	if (parse_oid_hex(buffer, &tree_oid, &p) || *p != '\n') {
-		err = report(options, oid, OBJ_COMMIT, FSCK_MSG_BAD_TREE_SHA1, "invalid 'tree' line format - bad sha1");
+		int err = report(options, oid, OBJ_COMMIT, FSCK_MSG_BAD_TREE_SHA1, "invalid 'tree' line format - bad sha1");
 		if (err)
 			return err;
 	}
 	buffer = p + 1;
 	while (buffer < buffer_end && skip_prefix(buffer, "parent ", &buffer)) {
 		if (parse_oid_hex(buffer, &parent_oid, &p) || *p != '\n') {
-			err = report(options, oid, OBJ_COMMIT, FSCK_MSG_BAD_PARENT_SHA1, "invalid 'parent' line format - bad sha1");
+			int err = report(options, oid, OBJ_COMMIT, FSCK_MSG_BAD_PARENT_SHA1, "invalid 'parent' line format - bad sha1");
 			if (err)
 				return err;
 		}
 		buffer = p + 1;
 	}
 	author_count = 0;
 	while (buffer < buffer_end && skip_prefix(buffer, "author ", &buffer)) {
+		int err = fsck_ident(&buffer, oid, OBJ_COMMIT, options);
+		if (err)
+			return err;
 		author_count++;
-		err = fsck_ident(&buffer, oid, OBJ_COMMIT, options);
+	}
+	if (author_count < 1) {
+		int err = report(options, oid, OBJ_COMMIT, FSCK_MSG_MISSING_AUTHOR, "invalid format - expected 'author' line");
+		if (err)
+			return err;
+	} else if (author_count > 1) {
+		int err = report(options, oid, OBJ_COMMIT, FSCK_MSG_MULTIPLE_AUTHORS, "invalid format - multiple 'author' lines");
 		if (err)
 			return err;
 	}
-	if (author_count < 1)
-		err = report(options, oid, OBJ_COMMIT, FSCK_MSG_MISSING_AUTHOR, "invalid format - expected 'author' line");
-	else if (author_count > 1)
-		err = report(options, oid, OBJ_COMMIT, FSCK_MSG_MULTIPLE_AUTHORS, "invalid format - multiple 'author' lines");
-	if (err)
-		return err;
 	if (buffer >= buffer_end || !skip_prefix(buffer, "committer ", &buffer))
 		return report(options, oid, OBJ_COMMIT, FSCK_MSG_MISSING_COMMITTER, "invalid format - expected 'committer' line");
-	err = fsck_ident(&buffer, oid, OBJ_COMMIT, options);
-	if (err)
-		return err;
+	else {
+		int err = fsck_ident(&buffer, oid, OBJ_COMMIT, options);
+		if (err)
+			return err;
+	}
 	if (memchr(buffer_begin, '\0', size)) {
-		err = report(options, oid, OBJ_COMMIT, FSCK_MSG_NUL_IN_COMMIT,
+		int err = report(options, oid, OBJ_COMMIT, FSCK_MSG_NUL_IN_COMMIT,
 			     "NUL byte in the commit object body");
 		if (err)
 			return err;

And then it is obvious that the general pattern is to propagate "err" from individual calls (and the ones that do not stick out like sore thumbs; are those bugs where we should keep going if the user set those message types to warn/ignore?).

You could even wrap the pattern in a macro, though perhaps that is getting too magical. The resulting logic is easier to follow, though, if you can look past the macro:

I'd suspect that just the first patch above would fix the CodeQL issue. It's certainly a larger diff, but IMHO the result is less confusing for humans, too.

-Peff

Hi,

For the record, I am abandoning my efforts to upstream this. I was never interested in the actual changes, I was interested in getting CodeQL to run on Git's code base without reporting false positives, so that I could compare the quality of the reports against Coverity's. Therefore, I was not really prepared to polish these patches as if they were something
important: They are not, and I cannot justify spending any more time on these patches. I will carry them as-are in microsoft/git, under the label 'uninitialized-variables', as long as they apply without major merge conflict headaches.

Ciao,
Johannes