@matheusbsbs / matheus-bagliano / commits / 4627c67fa6

object-file: fix a unpack_loose_header() regression in 3b6a8db3b03

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>

Ævar Arnfjörð Bjarmason committed May 12, 2022 at 15:32 UTC 4627c67fa68d5669be511962a6437a11c0db3c99
3 files changed +25 -6
object-file.c
+6 -2
@@ -2623,8 +2623,12 @@ int read_loose_object(const char *path,
2623 goto out;
2624 }
2625
2626 - if (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr),
2627 - NULL) < 0) {
2626 + switch (unpack_loose_header(&stream, map, mapsize, hdr, sizeof(hdr),
2627 + NULL)) {
2628 + case ULHR_OK:
2629 + break;
2630 + case ULHR_BAD:
2631 + case ULHR_TOO_LONG:
2632 error(_("unable to unpack header of %s"), path);
2633 goto out;
2634 }
t/t1006-cat-file.sh
+8 -2
@@ -681,7 +681,7 @@ test_expect_success 'cat-file -t and -s on corrupt loose object' '
681
682 # Setup and create the empty blob and its path
683 empty_path=$(git rev-parse --git-path objects/$(test_oid_to_path "$EMPTY_BLOB")) &&
684 - git hash-object -w --stdin </dev/null &&
684 + empty_blob=$(git hash-object -w --stdin </dev/null) &&
685
686 # Create another blob and its path
687 echo other >other.blob &&
@@ -722,7 +722,13 @@ test_expect_success 'cat-file -t and -s on corrupt loose object' '
722 # content out as-is. Try to make it zlib-invalid.
723 mv -f other.blob "$empty_path" &&
724 test_must_fail git fsck 2>err.fsck &&
725 - grep "^error: inflate: data stream error (" err.fsck
725 + cat >expect <<-EOF &&
726 + error: inflate: data stream error (incorrect header check)
727 + error: unable to unpack header of ./$empty_path
728 + error: $empty_blob: object corrupt or missing: ./$empty_path
729 + EOF
730 + grep "^error: " err.fsck >actual &&
731 + test_cmp expect actual
732 )
733 '
734
t/t1450-fsck.sh
+11 -2
@@ -774,10 +774,19 @@ test_expect_success 'fsck finds problems in duplicate loose objects' '
774 # no "-d" here, so we end up with duplicates
775 git repack &&
776 # now corrupt the loose copy
777 - file=$(sha1_file "$(git rev-parse HEAD)") &&
777 + oid="$(git rev-parse HEAD)" &&
778 + file=$(sha1_file "$oid") &&
779 rm "$file" &&
780 echo broken >"$file" &&
780 - test_must_fail git fsck
781 + test_must_fail git fsck 2>err &&
782 +
783 + cat >expect <<-EOF &&
784 + error: inflate: data stream error (incorrect header check)
785 + error: unable to unpack header of $file
786 + error: $oid: object corrupt or missing: $file
787 + EOF
788 + grep "^error: " err >actual &&
789 + test_cmp expect actual
790 )
791 '
792