[PATCH v3 00/17] fsck: better "invalid object" error reporting
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- May 20, 2021, 11:22 UTC
- Message-ID
- <cover-00.17-0000000000-20210520T111610Z-avarab@gmail.com>
- In-Reply-To
- <cover-0.6-00000000000-20210328T025618Z-avarab@gmail.com>
A re-roll of v2's 6 patch series[1], has turned into 17 now. Less scary than one might think though, it's mostly added test coverage + splitting existing commits into more incremental chunks, but see the range-diff. This should address all the feedback on v2 + more.
A brief recap summary of what this is about: We now gracefully recover instead of dying when fsck encounters types that aren't blob/commit/tree/tag. Those types don't exist in the wild, but you can manually create them with "git hash-object -t garbage --literally".
So in some senes this matters to nobody, but I'm doing this as part of general changes I've been pushing to make fsck/gc error reporting more graceful, and errors more recoverable. We now have a few more places in object-file.c where we don't just die(), but properly return API-like return codes/data to the caller instead.
This does not contain any changes to how --allow-unknown-type hash-object's --literally etc. work, as I suggested we could do in [2]. Any such changes will need the API changes here, but these are just the narrow fsck fixes.
1. https://lore.kernel.org/git/cover-0.6-00000000000-20210413T093734Z-avarab@gmail.com 2. https://lore.kernel.org/git/87r1i4qf4h.fsf@evledraar.gmail.com/
Ævar Arnfjörð Bjarmason (17): fsck tests: refactor one test to use a sub-repo fsck tests: add test for fsck-ing an unknown type cat-file tests: test for missing object with -t and -s cat-file tests: test that --allow-unknown-type isn't on by default rev-list tests: test for behavior with invalid object types cat-file tests: add corrupt loose object test cat-file tests: test for current --allow-unknown-type behavior cache.h: move object functions to object-store.h object-file.c: make parse_loose_header_extended() public object-file.c: add missing braces to loose_object_info() object-file.c: stop dying in parse_loose_header() object-file.c: return -2 on "header too long" in unpack_loose_header() object-file.c: return -1, not "status" from unpack_loose_header() fsck: don't hard die on invalid object types object-store.h: move read_loose_object() below 'struct object_info' fsck: report invalid types recorded in objects fsck: report invalid object type-path combinations
builtin/fast-export.c | 2 +- builtin/fsck.c | 28 ++++++- builtin/index-pack.c | 2 +- builtin/mktag.c | 3 +- cache.h | 10 --- object-file.c | 156 ++++++++++++++++++------------------- object-store.h | 60 +++++++++++---- object.c | 4 +- pack-check.c | 3 +- streaming.c | 10 ++- t/t1006-cat-file.sh | 169 +++++++++++++++++++++++++++++++++++++++++ t/t1450-fsck.sh | 64 +++++++++++----- t/t6115-rev-list-du.sh | 11 +++ 13 files changed, 387 insertions(+), 135 deletions(-)
Range-diff against v2:
2: 5a2cd6cca9 = 1: aa38b2bf9e fsck tests: refactor one test to use a sub-repo
-: ---------- > 2: 82b64abd25 fsck tests: add test for fsck-ing an unknown type
-: ---------- > 3: 7c3c2fe25d cat-file tests: test for missing object with -t and -s
-: ---------- > 4: 871b820003 cat-file tests: test that --allow-unknown-type isn't on by default
-: ---------- > 5: b98da9cc89 rev-list tests: test for behavior with invalid object types
-: ---------- > 6: 04cc1d20f6 cat-file tests: add corrupt loose object test
-: ---------- > 7: 9217320888 cat-file tests: test for current --allow-unknown-type behavior
1: 37c323a241 = 8: 12dd453879 cache.h: move object functions to object-store.h
3: d0d9cb3331 ! 9: 6a5b78dcad fsck: don't hard die on invalid object types
@@ Metadata
Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
## Commit message ##
- fsck: don't hard die on invalid object types
+ object-file.c: make parse_loose_header_extended() public
- Change builtin/fsck.c to pass down a
- OBJECT_INFO_ALLOW_UNKNOWN_TYPE. This changes this very ungraceful
- error:
+ Make the parse_loose_header_extended() function public and remove the
+ parse_loose_header() wrapper. The only direct user of it outside of
+ object-file.c itself was in streaming.c, that caller can simply pass
+ the required "struct object-info *" instead.
- $ git hash-object --stdin -w -t garbage --literally </dev/null
- <OID>
- $ git fsck
- fatal: invalid object type
- $
-
- Into:
-
- $ 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).
-
- But 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.
-
- The parse_loose_header() function being changed here is only used in
- builtin/fsck.c, see f6371f92104 (sha1_file: add read_loose_object()
- function, 2017-01-13) for its introduction.
+ This change is being done in preparation for teaching
+ read_loose_object() to accept a flag to pass to
+ parse_loose_header(). It isn't strictly necessary for that change, we
+ could simply use parse_loose_header_extended() there, but will leave
+ the API in a better end state.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- ## builtin/fsck.c ##
-@@ builtin/fsck.c: 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);
-
## object-file.c ##
@@ object-file.c: static void *unpack_loose_rest(git_zstream *stream,
* too permissive for what we want to check. So do an anal
@@ object-file.c: static int loose_object_info(struct repository *r,
if (status >= 0 && oi->contentp) {
@@ object-file.c: 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;
unsigned long mapsize;
git_zstream stream;
char hdr[MAX_HEADER_LEN];
@@ object-file.c: int read_loose_object(const char *path,
}
- *type = parse_loose_header(hdr, size);
-+ *type = parse_loose_header(hdr, &oi, oi_flags);
++ *type = parse_loose_header(hdr, &oi, 0);
if (*type < 0) {
error(_("unable to parse header of %s"), path);
git_inflate_end(&stream);
## object-store.h ##
-@@ object-store.h: 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
@@ object-store.h: int for_each_packed_object(each_packed_object_fn, void *,
int unpack_loose_header(git_zstream *stream, unsigned char *map,
unsigned long mapsize, void *buffer,
@@ object-store.h: int for_each_packed_object(each_packed_object_fn, void *,
int finalize_object_file(const char *tmpfile, const char *filename);
## streaming.c ##
-@@ streaming.c: static struct stream_vtbl loose_vtbl = {
-
- static open_method_decl(loose)
+@@ streaming.c: static int open_istream_loose(struct git_istream *st, struct repository *r,
+ const struct object_id *oid,
+ enum object_type *type)
{
-+ struct object_info oi2 = OBJECT_INFO_INIT;
-+ oi2.sizep = &st->size;
++ struct object_info oi = OBJECT_INFO_INIT;
++ oi.sizep = &st->size;
+
st->u.loose.mapped = map_loose_object(r, oid, &st->u.loose.mapsize);
if (!st->u.loose.mapped)
return -1;
-@@ streaming.c: static open_method_decl(loose)
+@@ streaming.c: static int open_istream_loose(struct git_istream *st, struct repository *r,
st->u.loose.mapsize,
st->u.loose.hdr,
sizeof(st->u.loose.hdr)) < 0) ||
- (parse_loose_header(st->u.loose.hdr, &st->size) < 0)) {
-+ (parse_loose_header(st->u.loose.hdr, &oi2, 0) < 0)) {
++ (parse_loose_header(st->u.loose.hdr, &oi, 0) < 0)) {
git_inflate_end(&st->z);
munmap(st->u.loose.mapped, st->u.loose.mapsize);
return -1;
-
- ## t/t1450-fsck.sh ##
-@@ t/t1450-fsck.sh: test_expect_success 'detect corrupt index file in fsck' '
- test_i18ngrep "bad index file" errors
- '
-
-+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) &&
-+ test_must_fail git -C garbage-type fsck >out 2>err &&
-+ grep "$garbage_blob: object corrupt or missing:" err &&
-+ grep "dangling blob $empty_blob" out
-+'
-+
- test_done
-: ---------- > 10: 5d31d7e1a5 object-file.c: add missing braces to loose_object_info()
-: ---------- > 11: ee28089219 object-file.c: stop dying in parse_loose_header()
-: ---------- > 12: 77f2cd439c object-file.c: return -2 on "header too long" in unpack_loose_header()
-: ---------- > 13: d22d5b8b85 object-file.c: return -1, not "status" from unpack_loose_header()
-: ---------- > 14: 260e9888a3 fsck: don't hard die on invalid object types
4: 81fffefcf9 ! 15: e2afb813b2 object-store.h: move read_loose_object() below 'struct object_info'
@@ Metadata
## Commit message ##
object-store.h: move read_loose_object() below 'struct object_info'
- Move the definition of read_loose_object() below "struct
+ Move the declaration of read_loose_object() below "struct
object_info". In the next commit we'll add a "struct object_info *"
parameter to it, moving it will avoid a forward declaration of the
struct.
5: 5fb6ac4fae ! 16: 328f05c51b fsck: report invalid types recorded in objects
@@ Commit message
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- ## builtin/cat-file.c ##
-@@ builtin/cat-file.c: static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
- struct strbuf sb = STRBUF_INIT;
- unsigned flags = OBJECT_INFO_LOOKUP_REPLACE;
- const char *path = force_path;
-+ int ret;
-
- if (unknown_type)
- flags |= OBJECT_INFO_ALLOW_UNKNOWN_TYPE;
-@@ builtin/cat-file.c: static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
- switch (opt) {
- case 't':
- oi.type_name = &sb;
-- if (oid_object_info_extended(the_repository, &oid, &oi, flags) < 0)
-+ ret = oid_object_info_extended(the_repository, &oid, &oi, flags);
-+ if (!unknown_type && ret < 0)
- die("git cat-file: could not get object info");
- if (sb.len) {
- printf("%s\n", sb.buf);
-@@ builtin/cat-file.c: static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
-
- case 's':
- oi.sizep = &size;
-- if (oid_object_info_extended(the_repository, &oid, &oi, flags) < 0)
-+ ret = oid_object_info_extended(the_repository, &oid, &oi, flags);
-+ if (!unknown_type && ret < 0)
- die("git cat-file: could not get object info");
- printf("%"PRIuMAX"\n", (uintmax_t)size);
- return 0;
-
## builtin/fsck.c ##
@@ builtin/fsck.c: static int fsck_loose(const struct object_id *oid, const char *path, void *data)
unsigned long size;
@@ builtin/fsck.c: static int fsck_loose(const struct object_id *oid, const char *p
## object-file.c ##
-@@ object-file.c: int parse_loose_header(const char *hdr,
- * we're obtaining the type using '--allow-unknown-type'
- * option.
- */
-- if ((flags & OBJECT_INFO_ALLOW_UNKNOWN_TYPE) && (type < 0))
-- type = 0;
-- else if (type < 0)
-+ if (type < 0 && !(flags & OBJECT_INFO_ALLOW_UNKNOWN_TYPE))
- die(_("invalid object type"));
- if (oi->typep)
- *oi->typep = type;
-@@ object-file.c: static int loose_object_info(struct repository *r,
- } else if (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr)) < 0)
- status = error(_("unable to unpack %s header"),
- oid_to_hex(oid));
-- if (status < 0)
-+ if (status < 0) {
- ; /* Do nothing */
-- else if (hdrbuf.len) {
-+ } else if (hdrbuf.len) {
- if ((status = parse_loose_header(hdrbuf.buf, oi, flags)) < 0)
- status = error(_("unable to parse %s header with --allow-unknown-type"),
- oid_to_hex(oid));
-- } else if ((status = parse_loose_header(hdr, oi, flags)) < 0)
-- status = error(_("unable to parse %s header"), oid_to_hex(oid));
-+ } else {
-+ status = parse_loose_header(hdr, oi, flags);
-+ if (status < 0 && !(flags & OBJECT_INFO_ALLOW_UNKNOWN_TYPE))
-+ error(_("unable to parse %s header"), oid_to_hex(oid));
-+ }
-
- if (status >= 0 && oi->contentp) {
- *oi->contentp = unpack_loose_rest(&stream, hdr,
@@ object-file.c: static int check_stream_oid(git_zstream *stream,
int read_loose_object(const char *path,
@@ object-file.c: 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;
+ enum object_type *type = oi->typep;
+ unsigned long *size = oi->sizep;
@@ object-file.c: int read_loose_object(const char *path,
goto out;
}
-- *type = parse_loose_header(hdr, &oi, oi_flags);
-- if (*type < 0) {
-- error(_("unable to parse header of %s"), path);
-+ *type = parse_loose_header(hdr, oi, oi_flags);
-+ if (*type < 0 && !(oi_flags & OBJECT_INFO_ALLOW_UNKNOWN_TYPE)) {
-+ error(_("unable to parse header %s"), path);
+- if (parse_loose_header(hdr, &oi) < 0) {
++ if (parse_loose_header(hdr, oi) < 0) {
+ error(_("unable to parse header of %s"), path);
git_inflate_end(&stream);
goto out;
- }
@@ object-file.c: int read_loose_object(const char *path,
goto out;
}
@@ t/t1450-fsck.sh: test_expect_success 'object with hash mismatch' '
git rev-parse HEAD^{tree} >.git/refs/heads/invalid &&
test_when_finished "git update-ref -d refs/heads/invalid" &&
@@ t/t1450-fsck.sh: test_expect_success 'fsck error and recovery on invalid object 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) &&
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 &&
++ test_line_count = 1 errors &&
+ grep "$garbage_blob: object is of unknown type '"'"'garbage'"'"':" err &&
-+ grep error: err >err.errors &&
-+ test_line_count = 1 err.errors &&
grep "dangling blob $empty_blob" out
'
6: 226d2031bc ! 17: c5e6686765 fsck: report invalid object type-path combinations
@@ Metadata
## Commit message ##
fsck: report invalid object type-path combinations
- fsck: improve error on loose object hash mismatch
-
Improve the error that's emitted in cases where we find a loose object
we parse, but which isn't at the location we expect it to be.
@@ builtin/fsck.c: static int fsck_loose(const struct object_id *oid, const char *p
struct strbuf sb = STRBUF_INIT;
unsigned int oi_flags = OBJECT_INFO_ALLOW_UNKNOWN_TYPE;
struct object_info oi;
-+ struct object_id real_oid = null_oid;
++ struct object_id real_oid = *null_oid();
int found = 0;
oi.type_name = &sb;
oi.sizep = &size;
@@ object-file.c: int check_object_signature(struct repository *r, const struct obj
break;
r->hash_algo->update_fn(&c, buf, readlen);
}
-- r->hash_algo->final_fn(real_oid.hash, &c);
-+ r->hash_algo->final_fn(real_oid->hash, &c);
+- r->hash_algo->final_oid_fn(&real_oid, &c);
++ r->hash_algo->final_oid_fn(real_oid, &c);
close_istream(st);
- return !oideq(oid, &real_oid) ? -1 : 0;
+ return !oideq(oid, real_oid) ? -1 : 0;
@@ object-file.c: int read_loose_object(const char *path,
- error(_("hash mismatch for %s (expected %s)"), path,
- oid_to_hex(expected_oid));
+ *contents, *size, oi->type_name->buf, real_oid)) {
-+ if (oideq(real_oid, &null_oid))
++ if (oideq(real_oid, null_oid()))
+ BUG("should only get OID mismatch errors with mapped contents");
free(*contents);
goto out;
@@ object-store.h: int oid_object_info_extended(struct repository *r,
struct object_info *oi,
unsigned int oi_flags);
@@ object-store.h: int unpack_loose_header(git_zstream *stream, unsigned char *map,
- int parse_loose_header(const char *hdr, struct object_info *oi,
- unsigned int flags);
+ int parse_loose_header(const char *hdr, struct object_info *oi);
+
int check_object_signature(struct repository *r, const struct object_id *oid,
- void *buf, unsigned long size, const char *type);
+ void *buf, unsigned long size, const char *type,
@@ pack-check.c: static int verify_packfile(struct repository *r,
oid_to_hex(&oid), p->pack_name);
else if (fn) {
+ ## t/t1006-cat-file.sh ##
+@@ t/t1006-cat-file.sh: test_expect_success 'cat-file -t and -s on corrupt loose object' '
+ # Swap the two to corrupt the repository
+ mv -v "$other_path" "$empty_path" &&
+ test_must_fail git fsck 2>err.fsck &&
+- grep "hash mismatch" err.fsck &&
++ grep "hash-path mismatch" err.fsck &&
+
+ # confirm that cat-file is reading the new swapped-in
+ # blob...
+
## t/t1450-fsck.sh ##
@@ t/t1450-fsck.sh: test_expect_success 'object with hash mismatch' '
(-- 2.32.0.rc0.406.g73369325f8d