refs: don't store peeled object IDs for invalid tags

Both the "files" and "reftable" backend store peeled object IDs for
references that point to tags:

  - The "files" backend stores the value when packing refs, where each
    peeled object ID is prefixed with "^".

  - The "reftable" backend stores the value whenever writing a new
    reference that points to a tag via a special ref record type.

Both of these backends use `peel_object()` to find the peeled object ID.
But as explained in the preceding commit, that function does not detect
the case where the tag's tagged object and its claimed type mismatch.

The consequence of storing these bogus peeled object IDs is that we're
less likely to detect such corruption in other parts of Git.
git-for-each-ref(1) for example does not notice anymore that the tag is
broken when using "--format=%(*objectname)" to dereference tags.

One could claim that this is good, because it still allows us to mostly
use the tag as intended. But the biggest problem here is that we now
have different behaviour for such a broken tag depending on whether or
not we have its peeled value in the refdb.

Fix the issue by verifying the object type when peeling the object. If
that verification fails we simply skip storing the peeled value in
either of the reference formats.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 4752d3f3981fe3de645e6bd6bce95e487639b481..1ab0c5039301647d133b9672898a8262aa8a1568 100644 (file)
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1528,7 +1528,7 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re
                } else {
                        struct object_id peeled;
                        int peel_error = peel_object(refs->base.repo, &update->new_oid,
-                                                    &peeled, 0);
+                                                    &peeled, PEEL_OBJECT_VERIFY_OBJECT_TYPE);
 
                        if (write_packed_entry(out, update->refname,
                                               &update->new_oid,

diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 9febb2322c3b24ba024a85a8fda5ac5af7c24047..6bbfd5618dac16693c735994d29550da73403bf7 100644 (file)
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -1632,7 +1632,8 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data
                        ref.refname = (char *)u->refname;
                        ref.update_index = ts;
 
-                       peel_error = peel_object(arg->refs->base.repo, &u->new_oid, &peeled, 0);
+                       peel_error = peel_object(arg->refs->base.repo, &u->new_oid, &peeled,
+                                                PEEL_OBJECT_VERIFY_OBJECT_TYPE);
                        if (!peel_error) {
                                ref.value_type = REFTABLE_REF_VAL2;
                                memcpy(ref.value.val2.target_value, peeled.hash, GIT_MAX_RAWSZ);

diff --git a/t/pack-refs-tests.sh b/t/pack-refs-tests.sh
index 3dbcc01718e157cec951736b333aa16506bd11f8..095823d915fd637e6212497566d3a76544d96caf 100644 (file)
--- a/t/pack-refs-tests.sh
+++ b/t/pack-refs-tests.sh
@@ -428,4 +428,36 @@ do
        '
 done
 
+test_expect_success 'pack-refs does not store invalid peeled tag value' '
+       test_when_finished rm -rf repo &&
+       git init repo &&
+       (
+               cd repo &&
+               git commit --allow-empty --message initial &&
+
+               echo garbage >blob-content &&
+               blob_id=$(git hash-object -w -t blob blob-content) &&
+
+               # Write an invalid tag into the object database. The tag itself
+               # is well-formed, but the tagged object is a blob while we
+               # claim that it is a commit.
+               cat >tag-content <<-EOF &&
+               object $blob_id
+               type commit
+               tag bad-tag
+               tagger C O Mitter <committer@example.com> 1112354055 +0200
+
+               annotated
+               EOF
+               tag_id=$(git hash-object -w -t tag tag-content) &&
+               git update-ref refs/tags/bad-tag "$tag_id" &&
+
+               # The packed-refs file should not contain the peeled object ID.
+               # If it did this would cause commands that use the peeled value
+               # to not notice this corrupted tag.
+               git pack-refs --all &&
+               test_grep ! "^\^" .git/packed-refs
+       )
+'
+
 test_done

diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
index 3ea5d51532a8b88ac8e36d79613812ed4668234a..6575528f212716df5a47bf3e973a269cb0e8bf9a 100755 (executable)
--- a/t/t0610-reftable-basics.sh
+++ b/t/t0610-reftable-basics.sh
@@ -1135,4 +1135,32 @@ test_expect_success 'fetch: accessing FETCH_HEAD special ref works' '
        test_cmp expect actual
 '
 
+test_expect_success 'writes do not persist peeled value for invalid tags' '
+       test_when_finished rm -rf repo &&
+       git init repo &&
+       (
+               cd repo &&
+               git commit --allow-empty --message initial &&
+
+               # We cannot easily verify that the peeled value is not stored
+               # in the tables. Instead, we test this indirectly: we create
+               # two tags that both point to the same object, but they claim
+               # different object types. If we parse both tags we notice that
+               # the parsed tagged object has a mismatch between the two tags
+               # and bail out.
+               #
+               # If we instead use the persisted peeled value we would not
+               # even parse the tags. As such, we would not notice the
+               # discrepancy either and thus listing these tags would succeed.
+               git tag tag-1 -m "tag 1" &&
+               git cat-file tag tag-1 >raw-tag &&
+               sed "s/^type commit$/type blob/" <raw-tag >broken-tag &&
+               broken_tag_id=$(git hash-object -w -t tag broken-tag) &&
+               git update-ref refs/tags/tag-2 $broken_tag_id &&
+
+               test_must_fail git for-each-ref --format="%(*objectname)" refs/tags/ 2>err &&
+               test_grep "bad tag pointer" err
+       )
+'
+
 test_done