fsck: verify .bitmap checksums, cleanup

PATCH Superseded Derrick Stolee via GitGitGadget 2 participants 8 messages May 1, 2023 – May 2, 2023

lore

Share & tools

lore

Derrick Stolee via GitGitGadget

author

2023-05-01 18:21 UTC #0 lore raw

cover fsck: verify .bitmap checksums, cleanup

Similar to the recent work on rev-indexes [1], add checks for .bitmap files to 'git fsck'.

Patch 1 includes the full details for .bitmap files.

Patch 2 includes a cleanup of ds/fsck-pack-revindex that I noticed while preparing this series.

Thanks, Stolee

[1]
https://lore.kernel.org/git/[email protected]/

Derrick Stolee (2):
fsck: verify checksums of all .bitmap files
fsck: use local repository

 builtin/fsck.c                | 10 +++++---
 pack-bitmap.c                 | 44 +++++++++++++++++++++++++++++++++++
 pack-bitmap.h                 |  2 ++
 t/t5326-multi-pack-bitmaps.sh | 38 ++++++++++++++++++++++++++++++
 4 files changed, 91 insertions(+), 3 deletions(-)

base-commit: 48d89b51b3bb8a60580c36731b96a7206ce1e5b9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1526%2Fderrickstolee%2Ffsck-bitmap-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1526/derrickstolee/fsck-bitmap-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1526

Signature · 1 line

gitgitgadget

Derrick Stolee via GitGitGadget

author

2023-05-01 18:21 UTC #3 lore raw

PATCH 2/2 fsck: use local repository

From: Derrick Stolee <[email protected]>

In 0d30feef3c5 (fsck: create scaffolding for rev-index checks, 2023-04-17) and later 5a6072f631d (fsck: validate .rev file header, 2023-04-17), the check_pack_rev_indexes() method was created with a 'struct repository *r' parameter. However, this parameter was unused and instead 'the_repository' was used in its place.

Fix this situation with the obvious replacement.

Signed-off-by Derrick Stolee <[email protected]>

---
 builtin/fsck.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

builtin/fsck.c +3 -3

index 75b30d1d00c..dcc165bf0c5 100644
@@ -869,20 +869,20 @@ static int check_pack_rev_indexes(struct repository *r, int show_progress)
 	int res = 0;
 
 	if (show_progress) {
-		for (struct packed_git *p = get_all_packs(the_repository); p; p = p->next)
+		for (struct packed_git *p = get_all_packs(r); p; p = p->next)
 			pack_count++;
 		progress = start_delayed_progress("Verifying reverse pack-indexes", pack_count);
 		pack_count = 0;
 	}
 
-	for (struct packed_git *p = get_all_packs(the_repository); p; p = p->next) {
+	for (struct packed_git *p = get_all_packs(r); p; p = p->next) {
 		int load_error = load_pack_revindex_from_disk(p);
 
 		if (load_error < 0) {
 			error(_("unable to load rev-index for pack '%s'"), p->pack_name);
 			res = ERROR_PACK_REV_INDEX;
 		} else if (!load_error &&
-			   !load_pack_revindex(the_repository, p) &&
+			   !load_pack_revindex(r, p) &&
 			   verify_pack_revindex(p)) {
 			error(_("invalid rev-index for pack '%s'"), p->pack_name);
 			res = ERROR_PACK_REV_INDEX;

Signature · 1 line

gitgitgadget

Derrick Stolee via GitGitGadget

author

2023-05-01 18:21 UTC #1 lore raw

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

In reply to Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

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 <[email protected]>

---
 builtin/fsck.c                |  4 ++++
 pack-bitmap.c                 | 44 +++++++++++++++++++++++++++++++++++
 pack-bitmap.h                 |  2 ++
 t/t5326-multi-pack-bitmaps.sh | 38 ++++++++++++++++++++++++++++++
 4 files changed, 88 insertions(+)

builtin/fsck.c +4 -0

index 2cd461b84c1..75b30d1d00c 100644
@@ -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();

pack-bitmap.c +44 -0

47 lines not on this page · raw message

pack-bitmap.h +2 -0

index f0180b5276b..84591f041bf 100644
@@ -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

t/t5326-multi-pack-bitmaps.sh +38 -0

index 0882cbb6e4a..d7353d8d443 100755
@@ -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

Signature · 1 line

gitgitgadget

Junio C Hamano

2023-05-01 19:07 UTC #2 lore raw

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

pack-bitmap.c int bitmap_is_preferred_refname(struct repository *r, const char *refname) · line 2361

Quoted 12 lines · Derrick Stolee via GitGitGadget

pack-bitmap.c +12 -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;
+	}

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.

pack-bitmap.c int bitmap_is_preferred_refname(struct repository *r, const char *refname) · line 2371

Quoted 9 lines

pack-bitmap.c +9 -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;
+}

OK.

Quoted 10 lines

pack-bitmap.c +10 -0

+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);
+	}

pack-bitmap.c int bitmap_is_preferred_refname(struct repository *r, const char *refname) · line 2392

Quoted 10 lines

pack-bitmap.c +10 -0

+
+	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.

Quoted 22 lines

t/t5326-multi-pack-bitmaps.sh +14 -0

index 0882cbb6e4a..d7353d8d443 100755
@@ -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.

t/t5326-multi-pack-bitmaps.sh test_expect_success 'tagged commits are selected for bitmapping' ' · line 460

Quoted 10 lines

t/t5326-multi-pack-bitmaps.sh +10 -0

+	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.

t/t5326-multi-pack-bitmaps.sh test_expect_success 'tagged commits are selected for bitmapping' ' · line 463

Quoted 2 lines

t/t5326-multi-pack-bitmaps.sh +2 -0
+	# fsck works at first
+	git fsck &&

OK.

t/t5326-multi-pack-bitmaps.sh test_expect_success 'tagged commits are selected for bitmapping' ' · line 467

Quoted 3 lines

t/t5326-multi-pack-bitmaps.sh +3 -0

+	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.

t/t5326-multi-pack-bitmaps.sh test_expect_success 'tagged commits are selected for bitmapping' ' · line 467

Quoted 2 lines

t/t5326-multi-pack-bitmaps.sh +2 -0
+	cp "$packbitmap.bak" "$packbitmap" &&
+	corrupt_file "$midxbitmap" &&

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

t/t5326-multi-pack-bitmaps.sh test_expect_success 'tagged commits are selected for bitmapping' ' · line 469

Quoted 2 lines

t/t5326-multi-pack-bitmaps.sh +2 -0

+	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.

Junio C Hamano

2023-05-01 19:10 UTC #4 lore raw

Re: [PATCH 2/2] fsck: use local repository

In reply to Derrick Stolee via GitGitGadget

Quoted 9 lines · Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>
In 0d30feef3c5 (fsck: create scaffolding for rev-index checks, 2023-04-17) and later 5a6072f631d (fsck: validate .rev file header, 2023-04-17), the check_pack_rev_indexes() method was created with a 'struct repository *r' parameter. However, this parameter was unused and instead 'the_repository' was used in its place.
Fix this situation with the obvious replacement.

Indeed obvious it is. Thanks, will queue.

Quoted 34 lines

Signed-off-by Derrick Stolee <[email protected]>

---
 builtin/fsck.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

builtin/fsck.c +3 -3

index 75b30d1d00c..dcc165bf0c5 100644
@@ -869,20 +869,20 @@ static int check_pack_rev_indexes(struct repository *r, int show_progress)
 	int res = 0;
 
 	if (show_progress) {
-		for (struct packed_git *p = get_all_packs(the_repository); p; p = p->next)
+		for (struct packed_git *p = get_all_packs(r); p; p = p->next)
 			pack_count++;
 		progress = start_delayed_progress("Verifying reverse pack-indexes", pack_count);
 		pack_count = 0;
 	}
 
-	for (struct packed_git *p = get_all_packs(the_repository); p; p = p->next) {
+	for (struct packed_git *p = get_all_packs(r); p; p = p->next) {
 		int load_error = load_pack_revindex_from_disk(p);
 
 		if (load_error < 0) {
 			error(_("unable to load rev-index for pack '%s'"), p->pack_name);
 			res = ERROR_PACK_REV_INDEX;
 		} else if (!load_error &&
-			   !load_pack_revindex(the_repository, p) &&
+			   !load_pack_revindex(r, p) &&
 			   verify_pack_revindex(p)) {
 			error(_("invalid rev-index for pack '%s'"), p->pack_name);
 			res = ERROR_PACK_REV_INDEX;

Derrick Stolee via GitGitGadget

author

2023-05-02 13:27 UTC #5 lore raw

v2 cover fsck: verify .bitmap checksums, cleanup

In reply to Derrick Stolee via GitGitGadget

Similar to the recent work on rev-indexes [1], add checks for .bitmap files to 'git fsck'.

Patch 1 includes the full details for .bitmap files.

Patch 2 includes a cleanup of ds/fsck-pack-revindex that I noticed while preparing this series.

Updates since v2
================

* The code for verify_bitmap_file() added checks for a negative file descriptor, as well as some other cleanup.
* The test in t5326 is expanded to include the "both are corrupt" case, as well as a reorganization because the test was not confined to an inner repo.

Thanks, Stolee

[1]
https://lore.kernel.org/git/[email protected]/

Derrick Stolee (2):
fsck: verify checksums of all .bitmap files
fsck: use local repository

 builtin/fsck.c                | 10 +++++---
 pack-bitmap.c                 | 45 +++++++++++++++++++++++++++++++++++
 pack-bitmap.h                 |  2 ++
 t/t5326-multi-pack-bitmaps.sh | 44 ++++++++++++++++++++++++++++++++++
 4 files changed, 98 insertions(+), 3 deletions(-)

base-commit: 48d89b51b3bb8a60580c36731b96a7206ce1e5b9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1526%2Fderrickstolee%2Ffsck-bitmap-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1526/derrickstolee/fsck-bitmap-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1526

Range-diff vs v1:

1: d608d2faa86 ! 1: 5d6d6658633 fsck: verify checksums of all .bitmap files

     @@ pack-bitmap.c: int bitmap_is_preferred_refname(struct repository *r, const char
      +	int fd = git_open(name);
      +	int res = 0;
      +
     -+	/* A non-existent file is valid. */
     -+	if (fstat(fd, &st)) {
     -+		close(fd);
     ++	/* It is OK to not have the file. */
     ++	if (fd < 0 || fstat(fd, &st)) {
     ++		if (fd >= 0)
     ++			close(fd);
      +		return 0;
      +	}
      +
     @@ t/t5326-multi-pack-bitmaps.sh: test_expect_success 'tagged commits are selected
      +	cp "$packbitmap.bak" "$packbitmap" &&
      +
      +	# fsck works at first
     -+	git fsck &&
     ++	git fsck 2>err &&
     ++	test_must_be_empty err &&
      +
      +	corrupt_file "$packbitmap" &&
      +	test_must_fail git fsck 2>err &&
     @@ t/t5326-multi-pack-bitmaps.sh: test_expect_success 'tagged commits are selected
      +	cp "$packbitmap.bak" "$packbitmap" &&
      +	corrupt_file "$midxbitmap" &&
      +	test_must_fail git fsck 2>err &&
     -+	grep "bitmap file '\''$midxbitmap'\'' has invalid checksum" err
     ++	grep "bitmap file '\''$midxbitmap'\'' has invalid checksum" err &&
     ++
     ++	corrupt_file "$packbitmap" &&
     ++	test_must_fail git fsck 2>err &&
     ++	grep "bitmap file '\''$midxbitmap'\'' has invalid checksum" err &&
     ++	grep "bitmap file '\''$packbitmap'\'' has invalid checksum" err
      +'
      +
       test_done

2: 2162a352a46 = 2: 00789bbc558 fsck: use local repository

Signature · 1 line

gitgitgadget

Derrick Stolee via GitGitGadget

author

2023-05-02 13:27 UTC #6 lore raw

PATCH v2 1/2 fsck: verify checksums of all .bitmap files

From: Derrick Stolee <[email protected]>

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.

Signed-off-by Derrick Stolee <[email protected]>

---
 builtin/fsck.c                |  4 ++++
 pack-bitmap.c                 | 45 +++++++++++++++++++++++++++++++++++
 pack-bitmap.h                 |  2 ++
 t/t5326-multi-pack-bitmaps.sh | 44 ++++++++++++++++++++++++++++++++++
 4 files changed, 95 insertions(+)

builtin/fsck.c +4 -0

index 2cd461b84c1..75b30d1d00c 100644
@@ -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();

pack-bitmap.c +45 -0

48 lines not on this page · raw message

pack-bitmap.h +2 -0

index f0180b5276b..84591f041bf 100644
@@ -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

t/t5326-multi-pack-bitmaps.sh +44 -0

48 lines not on this page · raw message

Signature · 1 line

gitgitgadget

Derrick Stolee via GitGitGadget

author

2023-05-02 13:27 UTC #7 lore raw

PATCH v2 2/2 fsck: use local repository

In reply to Derrick Stolee via GitGitGadget

From: Derrick Stolee <[email protected]>

Fix this situation with the obvious replacement.

Signed-off-by Derrick Stolee <[email protected]>

---
 builtin/fsck.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

builtin/fsck.c +3 -3

index 75b30d1d00c..dcc165bf0c5 100644
@@ -869,20 +869,20 @@ static int check_pack_rev_indexes(struct repository *r, int show_progress)
 	int res = 0;
 
 	if (show_progress) {
-		for (struct packed_git *p = get_all_packs(the_repository); p; p = p->next)
+		for (struct packed_git *p = get_all_packs(r); p; p = p->next)
 			pack_count++;
 		progress = start_delayed_progress("Verifying reverse pack-indexes", pack_count);
 		pack_count = 0;
 	}
 
-	for (struct packed_git *p = get_all_packs(the_repository); p; p = p->next) {
+	for (struct packed_git *p = get_all_packs(r); p; p = p->next) {
 		int load_error = load_pack_revindex_from_disk(p);
 
 		if (load_error < 0) {
 			error(_("unable to load rev-index for pack '%s'"), p->pack_name);
 			res = ERROR_PACK_REV_INDEX;
 		} else if (!load_error &&
-			   !load_pack_revindex(the_repository, p) &&
+			   !load_pack_revindex(r, p) &&
 			   verify_pack_revindex(p)) {
 			error(_("invalid rev-index for pack '%s'"), p->pack_name);
 			res = ERROR_PACK_REV_INDEX;

Signature · 1 line

gitgitgadget