[1/2] fsck: verify checksums of all .bitmap files

Commit Message

Derrick Stolee May 1, 2023, 6:21 p.m. UTC

From: Derrick Stolee <[e-mail]>

If a filesystem-level corruption occurs in a .bitmap file, Git can react
poorly. This could take the form of a run-time error due to failing to
parse an EWAH bitmap or be more subtle such as returning the wrong set
of objects to a fetch or clone.

A natural first response to either of these kinds of errors is to run
'git fsck' to see if any files are corrupt. This currently ignores all
.bitmap files.

Add checks to 'git fsck' for all .bitmap files that are currently
associated with a multi-pack-index or pack file. Verify their checksums
using the hashfile API.

We iterate through all multi-pack-indexes and pack-files to be sure to
check all .bitmap files, not just the one that would be read by the
process. For example, a multi-pack-index bitmap overrules a pack-bitmap.
However, if the multi-pack-index is removed, the pack-bitmap may be
selected instead. Be thorough to include every file that could become
active in such a way. This includes checking files in alternates.

There is potential that we could extend this effort to check the
structure of the reachability bitmaps themselves, but it is very
expensive to do so. At minimum, it's as expensive as generating the
bitmaps in the first place, and that's assuming that we don't use the
trivial algorithm of verifying each bitmap individually. The trivial
algorithm will result in quadratic behavior (number of objects times
number of bitmapped commits) while the bitmap building operation
constructs a lattice of commits to build bitmaps incrementally and then
generate the final bitmaps from a subset of those commits.

If we were to extend 'git fsck' to check .bitmap file contents more
closely like this, then we would likely want to hide it behind an option
that signals the user is more willing to do expensive operations such as
this.

For testing, set up a repository with a pack-bitmap _and_ a
multi-pack-index bitmap. This requires some file movement to avoid
deleting the pack-bitmap during the repack that creates the
multi-pack-index bitmap. We can then verify that 'git fsck' is checking
all files, not just the "active" bitmap.

Signed-off-by: Derrick Stolee <[e-mail]>
---
 builtin/fsck.c                |  4 ++++
 pack-bitmap.c                 | 44 +++++++++++++++++++++++++++++++++++
 pack-bitmap.h                 |  2 ++
 t/t5326-multi-pack-bitmaps.sh | 38 ++++++++++++++++++++++++++++++
 4 files changed, 88 insertions(+)

Comments

Junio C Hamano May 1, 2023, 7:07 p.m. UTC | #1

"Derrick Stolee via GitGitGadget" <[e-mail]> writes:

> +static int verify_bitmap_file(const char *name)
> +{
> +	struct stat st;
> +	unsigned char *data;
> +	int fd = git_open(name);
> +	int res = 0;
> +
> +	/* A non-existent file is valid. */
> +	if (fstat(fd, &st)) {
> +		close(fd);
> +		return 0;
> +	}

git_open() may return a negative value to signal an error, and
fstat() would return negative to signal its own error, and feeding
the negative fd to close() would also be an error without molesting
other open file descriptors we would want to keep, and we do not
check the return value from close(), so all of the above may be
safe, but makes us feel a bit dirty.

	/* It is OK not to have the file */
	if (fd < 0 || fstat(fd, &st)) {
		if (0 <= fd)
			close(fd);
		return 0;
	}

is probably not overly too verbose and makes the result a bit more
palatable, perhaps.

> +	data = xmmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
> +	close(fd);
> +	if (!hashfile_checksum_valid(data, st.st_size))
> +		res = error(_("bitmap file '%s' has invalid checksum"),
> +			    name);
> +
> +	munmap(data, st.st_size);
> +	return res;
> +}

OK.

> +int verify_bitmap_files(struct repository *r)
> +{
> +	int res = 0;
> +
> +	for (struct multi_pack_index *m = get_multi_pack_index(r);
> +	     m; m = m->next) {
> +		char *midx_bitmap_name = midx_bitmap_filename(m);
> +		res |= verify_bitmap_file(midx_bitmap_name);
> +		free(midx_bitmap_name);
> +	}
> +
> +	for (struct packed_git *p = get_all_packs(r);
> +	     p; p = p->next) {
> +		char *pack_bitmap_name = pack_bitmap_filename(p);
> +		res |= verify_bitmap_file(pack_bitmap_name);
> +		free(pack_bitmap_name);
> +	}
> +
> +	return res;
> +}

OK.

> diff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh
> index 0882cbb6e4a..d7353d8d443 100755
> --- a/t/t5326-multi-pack-bitmaps.sh
> +++ b/t/t5326-multi-pack-bitmaps.sh
> @@ -434,4 +434,42 @@ test_expect_success 'tagged commits are selected for bitmapping' '
>  	)
>  '
>  
> +corrupt_file () {
> +	chmod a+w "$1" &&
> +	printf "bogus" | dd of="$1" bs=1 seek="12" conv=notrunc
> +}
> +
> +test_expect_success 'git fsck correctly identifies good and bad bitmaps' '
> +	git init valid &&
> +	test_when_finished rm -rf valid &&
> +
> +	test_commit_bulk 20 &&
> +	git repack -adbf &&
> +
> +	# Move pack-bitmap aside so it is not deleted
> +	# in next repack.
> +	packbitmap=$(ls .git/objects/pack/pack-*.bitmap) &&
> +	mv "$packbitmap" "$packbitmap.bak" &&
> +
> +	test_commit_bulk 10 &&
> +	git repack -b --write-midx &&
> +	midxbitmap=$(ls .git/objects/pack/multi-pack-index-*.bitmap) &&
> +
> +	# Copy MIDX bitmap to backup. Copy pack bitmap from backup.
> +	cp "$midxbitmap" "$midxbitmap.bak" &&
> +	cp "$packbitmap.bak" "$packbitmap" &&

So, at this point, we have only one pack, and we could enable the
midx bitmap and/or pack bitmap from their .bak.  We first enable
only the pack bitmap while leaving midx bitmap disabled.

> +	# fsck works at first
> +	git fsck &&

OK.

> +	corrupt_file "$packbitmap" &&
> +	test_must_fail git fsck 2>err &&
> +	grep "bitmap file '\''$packbitmap'\'' has invalid checksum" err &&

And when the only thing enabled, i.e. pack bitmap, is broken, we
notice the breakage.  OK.

> +	cp "$packbitmap.bak" "$packbitmap" &&
> +	corrupt_file "$midxbitmap" &&

Now we repair the pack bitmap and break midx bitmap.  Both are
enabled in this case.

> +	test_must_fail git fsck 2>err &&
> +	grep "bitmap file '\''$midxbitmap'\'' has invalid checksum" err

And we notice midx bitmap is corrupt.

Is it worth checking at this point if we detect breakages in both
bitmaps, if we have pack bitmap and midx bitmap, both broken?

Other than these tiny nits, looking very good.

Thanks.

Patch

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 2cd461b84c1..75b30d1d00c 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -27,6 +27,7 @@ 
 #include "run-command.h"
 #include "worktree.h"
 #include "pack-revindex.h"
+#include "pack-bitmap.h"
 
 #define REACHABLE 0x0001
 #define SEEN      0x0002
@@ -57,6 +58,7 @@  static int name_objects;
 #define ERROR_COMMIT_GRAPH 020
 #define ERROR_MULTI_PACK_INDEX 040
 #define ERROR_PACK_REV_INDEX 0100
+#define ERROR_BITMAP 0200
 
 static const char *describe_object(const struct object_id *oid)
 {
@@ -1056,6 +1058,8 @@  int cmd_fsck(int argc, const char **argv, const char *prefix)
 	}
 
 	errors_found |= check_pack_rev_indexes(the_repository, show_progress);
+	if (verify_bitmap_files(the_repository))
+		errors_found |= ERROR_BITMAP;
 
 	check_connectivity();
 
diff --git a/pack-bitmap.c b/pack-bitmap.c
index e0fad723bf3..ded58929a81 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -2346,3 +2346,47 @@  int bitmap_is_preferred_refname(struct repository *r, const char *refname)
 
 	return 0;
 }
+
+static int verify_bitmap_file(const char *name)
+{
+	struct stat st;
+	unsigned char *data;
+	int fd = git_open(name);
+	int res = 0;
+
+	/* A non-existent file is valid. */
+	if (fstat(fd, &st)) {
+		close(fd);
+		return 0;
+	}
+
+	data = xmmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
+	close(fd);
+	if (!hashfile_checksum_valid(data, st.st_size))
+		res = error(_("bitmap file '%s' has invalid checksum"),
+			    name);
+
+	munmap(data, st.st_size);
+	return res;
+}
+
+int verify_bitmap_files(struct repository *r)
+{
+	int res = 0;
+
+	for (struct multi_pack_index *m = get_multi_pack_index(r);
+	     m; m = m->next) {
+		char *midx_bitmap_name = midx_bitmap_filename(m);
+		res |= verify_bitmap_file(midx_bitmap_name);
+		free(midx_bitmap_name);
+	}
+
+	for (struct packed_git *p = get_all_packs(r);
+	     p; p = p->next) {
+		char *pack_bitmap_name = pack_bitmap_filename(p);
+		res |= verify_bitmap_file(pack_bitmap_name);
+		free(pack_bitmap_name);
+	}
+
+	return res;
+}
diff --git a/pack-bitmap.h b/pack-bitmap.h
index f0180b5276b..84591f041bf 100644
--- a/pack-bitmap.h
+++ b/pack-bitmap.h
@@ -111,4 +111,6 @@  int bitmap_is_midx(struct bitmap_index *bitmap_git);
 const struct string_list *bitmap_preferred_tips(struct repository *r);
 int bitmap_is_preferred_refname(struct repository *r, const char *refname);
 
+int verify_bitmap_files(struct repository *r);
+
 #endif
diff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh
index 0882cbb6e4a..d7353d8d443 100755
--- a/t/t5326-multi-pack-bitmaps.sh
+++ b/t/t5326-multi-pack-bitmaps.sh
@@ -434,4 +434,42 @@  test_expect_success 'tagged commits are selected for bitmapping' '
 	)
 '
 
+corrupt_file () {
+	chmod a+w "$1" &&
+	printf "bogus" | dd of="$1" bs=1 seek="12" conv=notrunc
+}
+
+test_expect_success 'git fsck correctly identifies good and bad bitmaps' '
+	git init valid &&
+	test_when_finished rm -rf valid &&
+
+	test_commit_bulk 20 &&
+	git repack -adbf &&
+
+	# Move pack-bitmap aside so it is not deleted
+	# in next repack.
+	packbitmap=$(ls .git/objects/pack/pack-*.bitmap) &&
+	mv "$packbitmap" "$packbitmap.bak" &&
+
+	test_commit_bulk 10 &&
+	git repack -b --write-midx &&
+	midxbitmap=$(ls .git/objects/pack/multi-pack-index-*.bitmap) &&
+
+	# Copy MIDX bitmap to backup. Copy pack bitmap from backup.
+	cp "$midxbitmap" "$midxbitmap.bak" &&
+	cp "$packbitmap.bak" "$packbitmap" &&
+
+	# fsck works at first
+	git fsck &&
+
+	corrupt_file "$packbitmap" &&
+	test_must_fail git fsck 2>err &&
+	grep "bitmap file '\''$packbitmap'\'' has invalid checksum" err &&
+
+	cp "$packbitmap.bak" "$packbitmap" &&
+	corrupt_file "$midxbitmap" &&
+	test_must_fail git fsck 2>err &&
+	grep "bitmap file '\''$midxbitmap'\'' has invalid checksum" err
+'
+
 test_done