From: Junio C Hamano <gitster@pobox.com> To: git@vger.kernel.org Cc: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com> Subject: [PATCH v2 4/4] object-file: fix a unpack_loose_header() regression in 3b6a8db3b03 Date: Thu, 12 May 2022 15:32:18 -0700 [thread overview] Message-ID: <20220512223218.237544-5-gitster@pobox.com> (raw) In-Reply-To: <20220512223218.237544-1-gitster@pobox.com> From: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Fix a regression in my 3b6a8db3b03 (object-file.c: use "enum" return type for unpack_loose_header(), 2021-10-01) revealed both by running the test suite with --valgrind, and with the amended "git fsck" test. In practice this regression in v2.34.0 caused us to claim that we couldn't parse the header, as opposed to not being able to unpack it. Before the change in the C code the test_cmp added here would emit: -error: unable to unpack header of ./objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391 +error: unable to parse header of ./objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391 I.e. we'd proceed to call parse_loose_header() on the uninitialized "hdr" value, and it would have been very unlikely for that uninitialized memory to be a valid git object. The other callers of unpack_loose_header() were already checking the enum values exhaustively. See 3b6a8db3b03 and 5848fb11acd (object-file.c: return ULHR_TOO_LONG on "header too long", 2021-10-01). Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- object-file.c | 8 ++++++-- t/t1006-cat-file.sh | 10 ++++++++-- t/t1450-fsck.sh | 13 +++++++++++-- 3 files changed, 25 insertions(+), 6 deletions(-) diff --git a/object-file.c b/object-file.c index 5ffbf3d4fd..b5d1d12b68 100644 --- a/object-file.c +++ b/object-file.c @@ -2623,8 +2623,12 @@ int read_loose_object(const char *path, goto out; } - if (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr), - NULL) < 0) { + switch (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr), + NULL)) { + case ULHR_OK: + break; + case ULHR_BAD: + case ULHR_TOO_LONG: error(_("unable to unpack header of %s"), path); goto out; } diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh index 1b85207694..dadf3b1458 100755 --- a/t/t1006-cat-file.sh +++ b/t/t1006-cat-file.sh @@ -681,7 +681,7 @@ test_expect_success 'cat-file -t and -s on corrupt loose object' ' # Setup and create the empty blob and its path empty_path=$(git rev-parse --git-path objects/$(test_oid_to_path "$EMPTY_BLOB")) && - git hash-object -w --stdin </dev/null && + empty_blob=$(git hash-object -w --stdin </dev/null) && # Create another blob and its path echo other >other.blob && @@ -722,7 +722,13 @@ test_expect_success 'cat-file -t and -s on corrupt loose object' ' # content out as-is. Try to make it zlib-invalid. mv -f other.blob "$empty_path" && test_must_fail git fsck 2>err.fsck && - grep "^error: inflate: data stream error (" err.fsck + cat >expect <<-EOF && + error: inflate: data stream error (incorrect header check) + error: unable to unpack header of ./$empty_path + error: $empty_blob: object corrupt or missing: ./$empty_path + EOF + grep "^error: " err.fsck >actual && + test_cmp expect actual ) ' diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh index de50c0ea01..ab7f31f1dc 100755 --- a/t/t1450-fsck.sh +++ b/t/t1450-fsck.sh @@ -774,10 +774,19 @@ test_expect_success 'fsck finds problems in duplicate loose objects' ' # no "-d" here, so we end up with duplicates git repack && # now corrupt the loose copy - file=$(sha1_file "$(git rev-parse HEAD)") && + oid="$(git rev-parse HEAD)" && + file=$(sha1_file "$oid") && rm "$file" && echo broken >"$file" && - test_must_fail git fsck + test_must_fail git fsck 2>err && + + cat >expect <<-EOF && + error: inflate: data stream error (incorrect header check) + error: unable to unpack header of $file + error: $oid: object corrupt or missing: $file + EOF + grep "^error: " err >actual && + test_cmp expect actual ) ' -- 2.36.1-338-g1c7f76a54c
next prev parent reply other threads:[~2022-05-12 22:36 UTC|newest] Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-04-21 20:14 [PATCH 0/4] Fix issues and a regression noted by valgrind Ævar Arnfjörð Bjarmason 2022-04-21 20:14 ` [PATCH 1/4] tests: make RUNTIME_PREFIX compatible with --valgrind Ævar Arnfjörð Bjarmason 2022-04-21 22:22 ` Junio C Hamano 2022-04-21 20:14 ` [PATCH 2/4] log test: skip a failing mkstemp() test under valgrind Ævar Arnfjörð Bjarmason 2022-04-21 20:14 ` [PATCH 3/4] commit-graph.c: don't assume that stat() succeeds Ævar Arnfjörð Bjarmason 2022-04-21 22:29 ` Junio C Hamano 2022-04-21 20:14 ` [PATCH 4/4] object-file: fix a unpack_loose_header() regression in 3b6a8db3b03 Ævar Arnfjörð Bjarmason 2022-04-21 22:39 ` Junio C Hamano 2022-04-22 8:21 ` Ævar Arnfjörð Bjarmason 2022-05-12 22:32 ` [PATCH v2 0/4] test fixes around valgrind Junio C Hamano 2022-05-12 22:32 ` [PATCH v2 1/4] tests: using custom GIT_EXEC_PATH breaks --valgrind tests Junio C Hamano 2022-05-12 22:32 ` [PATCH v2 2/4] log test: skip a failing mkstemp() test under valgrind Junio C Hamano 2022-05-12 22:32 ` [PATCH v2 3/4] commit-graph.c: don't assume that stat() succeeds Junio C Hamano 2022-05-12 22:32 ` Junio C Hamano [this message] 2022-05-12 23:39 ` [PATCH v2 4/4] object-file: fix a unpack_loose_header() regression in 3b6a8db3b03 Junio C Hamano 2022-05-16 14:59 ` Derrick Stolee 2022-05-19 20:09 ` [RFC PATCH 0/2] Alternate ab/valgrind-fixes fix-up Ævar Arnfjörð Bjarmason 2022-05-19 20:09 ` [RFC PATCH 1/2] object-file API: fix obscure unpack_loose_header() return Ævar Arnfjörð Bjarmason 2022-05-19 20:09 ` [RFC PATCH 2/2] object-file API: have unpack_loose_header() return "int" again Ævar Arnfjörð Bjarmason 2022-05-20 4:27 ` Junio C Hamano
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style List information: http://vger.kernel.org/majordomo-info.html * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20220512223218.237544-5-gitster@pobox.com \ --to=gitster@pobox.com \ --cc=avarab@gmail.com \ --cc=git@vger.kernel.org \ --subject='Re: [PATCH v2 4/4] object-file: fix a unpack_loose_header() regression in 3b6a8db3b03' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
Code repositories for project(s) associated with this inbox: https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).