diff mbox series

[v3,14/17] fsck: don't hard die on invalid object types

Message ID patch-14.17-260e9888a3-20210520T111610Z-avarab@gmail.com (mailing list archive)
State New, archived
Headers show
Series fsck: better "invalid object" error reporting | expand

Commit Message

Ævar Arnfjörð Bjarmason May 20, 2021, 11:23 a.m. UTC
Change the error fsck emits on invalid object types, such as:

    $ git hash-object --stdin -w -t garbage --literally </dev/null
    <OID>

From the very ungraceful error of:

    $ git fsck
    fatal: invalid object type
    $

To:

    $ git fsck
    error: hash mismatch for <OID_PATH> (expected <OID>)
    error: <OID>: object corrupt or missing: <OID_PATH>
    [ the rest of the fsck output here, i.e. it didn't hard die ]

We'll still exit with non-zero, but now we'll finish the rest of the
traversal. The tests that's being added here asserts that we'll still
complain about other fsck issues (e.g. an unrelated dangling blob).

To do this we need to pass down the "OBJECT_INFO_ALLOW_UNKNOWN_TYPE"
flag from read_loose_object() through to parse_loose_header(). Since
the read_loose_object() function is only used in builtin/fsck.c we can
simply change it. See f6371f92104 (sha1_file: add read_loose_object()
function, 2017-01-13) for the introduction of read_loose_object().

Why are we complaining about a "hash mismatch" for an object of a type
we don't know about? We shouldn't. This is the bare minimal change
needed to not make fsck hard die on a repository that's been corrupted
in this manner. In subsequent commits we'll teach fsck to recognize
this particular type of corruption and emit a better error message.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/fsck.c  |  3 ++-
 object-file.c   | 11 ++++++++---
 object-store.h  |  3 ++-
 t/t1450-fsck.sh | 14 +++++++-------
 4 files changed, 19 insertions(+), 12 deletions(-)

Comments

Jonathan Tan May 27, 2021, 6:18 p.m. UTC | #1
> diff --git a/object-file.c b/object-file.c
> index 0de699de98..0e8a024eb3 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -2522,7 +2522,8 @@ int read_loose_object(const char *path,
>  		      const struct object_id *expected_oid,
>  		      enum object_type *type,
>  		      unsigned long *size,
> -		      void **contents)
> +		      void **contents,
> +		      unsigned int oi_flags)
>  {
>  	int ret = -1;
>  	void *map = NULL;
> @@ -2530,6 +2531,7 @@ int read_loose_object(const char *path,
>  	git_zstream stream;
>  	char hdr[MAX_HEADER_LEN];
>  	struct object_info oi = OBJECT_INFO_INIT;
> +	int allow_unknown = oi_flags & OBJECT_INFO_ALLOW_UNKNOWN_TYPE;
>  	oi.typep = type;
>  	oi.sizep = size;
>  
> @@ -2552,8 +2554,11 @@ int read_loose_object(const char *path,
>  		git_inflate_end(&stream);
>  		goto out;
>  	}
> -	if (*type < 0)
> -		die(_("invalid object type"));
> +	if (!allow_unknown && *type < 0) {
> +		error(_("header for %s declares an unknown type"), path);
> +		git_inflate_end(&stream);
> +		goto out;
> +	}
>  
>  	if (*type == OBJ_BLOB && *size > big_file_threshold) {
>  		if (check_stream_oid(&stream, hdr, *size, path, expected_oid) < 0)

So instead of dying, we print an error and behave as if the object was
invalid for some other reason. Makes sense.
diff mbox series

Patch

diff --git a/builtin/fsck.c b/builtin/fsck.c
index 87a99b0108..38b515deb6 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -600,7 +600,8 @@  static int fsck_loose(const struct object_id *oid, const char *path, void *data)
 	void *contents;
 	int eaten;
 
-	if (read_loose_object(path, oid, &type, &size, &contents) < 0) {
+	if (read_loose_object(path, oid, &type, &size, &contents,
+			      OBJECT_INFO_ALLOW_UNKNOWN_TYPE) < 0) {
 		errors_found |= ERROR_OBJECT;
 		error(_("%s: object corrupt or missing: %s"),
 		      oid_to_hex(oid), path);
diff --git a/object-file.c b/object-file.c
index 0de699de98..0e8a024eb3 100644
--- a/object-file.c
+++ b/object-file.c
@@ -2522,7 +2522,8 @@  int read_loose_object(const char *path,
 		      const struct object_id *expected_oid,
 		      enum object_type *type,
 		      unsigned long *size,
-		      void **contents)
+		      void **contents,
+		      unsigned int oi_flags)
 {
 	int ret = -1;
 	void *map = NULL;
@@ -2530,6 +2531,7 @@  int read_loose_object(const char *path,
 	git_zstream stream;
 	char hdr[MAX_HEADER_LEN];
 	struct object_info oi = OBJECT_INFO_INIT;
+	int allow_unknown = oi_flags & OBJECT_INFO_ALLOW_UNKNOWN_TYPE;
 	oi.typep = type;
 	oi.sizep = size;
 
@@ -2552,8 +2554,11 @@  int read_loose_object(const char *path,
 		git_inflate_end(&stream);
 		goto out;
 	}
-	if (*type < 0)
-		die(_("invalid object type"));
+	if (!allow_unknown && *type < 0) {
+		error(_("header for %s declares an unknown type"), path);
+		git_inflate_end(&stream);
+		goto out;
+	}
 
 	if (*type == OBJ_BLOB && *size > big_file_threshold) {
 		if (check_stream_oid(&stream, hdr, *size, path, expected_oid) < 0)
diff --git a/object-store.h b/object-store.h
index 9accb614fc..790e8b1798 100644
--- a/object-store.h
+++ b/object-store.h
@@ -245,7 +245,8 @@  int read_loose_object(const char *path,
 		      const struct object_id *expected_oid,
 		      enum object_type *type,
 		      unsigned long *size,
-		      void **contents);
+		      void **contents,
+		      unsigned int oi_flags);
 
 /* Retry packed storage after checking packed and loose storage */
 #define HAS_OBJECT_RECHECK_PACKED 1
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index f36ec1e2f4..e7e8decebb 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -863,16 +863,16 @@  test_expect_success 'detect corrupt index file in fsck' '
 	test_i18ngrep "bad index file" errors
 '
 
-test_expect_success 'fsck hard errors on an invalid object type' '
+test_expect_success 'fsck error and recovery on invalid object type' '
 	test_create_repo garbage-type &&
 	empty_blob=$(git -C garbage-type hash-object --stdin -w -t blob </dev/null) &&
 	garbage_blob=$(git -C garbage-type hash-object --stdin -w -t garbage --literally </dev/null) &&
-	cat >err.expect <<-\EOF &&
-	fatal: invalid object type
-	EOF
-	test_must_fail git -C garbage-type fsck >out.actual 2>err.actual &&
-	test_cmp err.expect err.actual &&
-	test_must_be_empty out.actual
+	test_must_fail git -C garbage-type fsck >out 2>err &&
+	grep -e "^error" -e "^fatal" err >errors &&
+	test_line_count = 2 errors &&
+	grep "error: hash mismatch for" err &&
+	grep "$garbage_blob: object corrupt or missing:" err &&
+	grep "dangling blob $empty_blob" out
 '
 
 test_done