cache-tree: reject entries with null sha1
authorJeff King <peff@peff.net>
Fri, 21 Apr 2017 18:46:17 +0000 (14:46 -0400)
committerJunio C Hamano <gitster@pobox.com>
Mon, 24 Apr 2017 01:21:59 +0000 (18:21 -0700)
We generally disallow null sha1s from entering the index,
due to 4337b5856 (do not write null sha1s to on-disk index,
2012-07-28). However, we loosened that in 83bd7437c
(write_index: optionally allow broken null sha1s,
2013-08-27) so that tools like filter-branch could be used
to repair broken history.

However, we should make sure that these broken entries do
not get propagated into new trees. For most entries, we'd
catch them with the missing-object check (since presumably
the null sha1 does not exist in our object database). But
gitlink entries do not need reachability, so we may blindly
copy the entry into a bogus tree.

This patch rejects all null sha1s (with the same "invalid
entry" message that missing objects get) when building trees
from the index. It does so even for non-gitlinks, and even
when "write-tree" is given the --missing-ok flag. The null
sha1 is a special sentinel value that is already rejected in
trees by fsck; whether the object exists or not, it is an
error to put it in a tree.

Note that for this to work, we must also avoid reusing an
existing cache-tree that contains the null sha1. This patch
does so by just refusing to write out any cache tree when
the index contains a null sha1. This is blunter than we need
to be; we could just reject the subtree that contains the
offending entry. But it's not worth the complexity. The
behavior is unchanged unless you have a broken index entry,
and even then we'd refuse the whole index write unless the
emergency GIT_ALLOW_NULL_SHA1 is in use. And even then the
end result is only a performance drop (any write-tree will
have to generate the whole cache-tree from scratch).

The tests bear some explanation.

The existing test in t7009 doesn't catch this problem,
because our index-filter runs "git rm --cached", which will
try to rewrite the updated index and barf on the bogus
entry. So we never even make it to write-tree. The new test
there adds a noop index-filter, which does show the problem.

The new tests in t1601 are slightly redundant with what
filter-branch is doing under the hood in t7009. But as
they're much more direct, they're easier to reason about.
And should filter-branch ever change or go away, we'd want
to make sure that these plumbing commands behave sanely.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
cache-tree.c
read-cache.c
t/t1601-index-bogus.sh [new file with mode: 0755]
t/t7009-filter-branch-null-sha1.sh
index 345ea359638fbdfec874c55d387f9da2399338d2..34baa6d85a1e156a5d01c0e5f9c5f5d2709049ab 100644 (file)
@@ -354,7 +354,9 @@ static int update_one(struct cache_tree *it,
                        entlen = pathlen - baselen;
                        i++;
                }
-               if (mode != S_IFGITLINK && !missing_ok && !has_sha1_file(sha1)) {
+
+               if (is_null_sha1(sha1) ||
+                   (mode != S_IFGITLINK && !missing_ok && !has_sha1_file(sha1))) {
                        strbuf_release(&buffer);
                        if (expected_missing)
                                return -1;
index 9054369dd0c6ddfd35794fc63f0e0296ec0c9c91..2ca2cae6179b897285c406b94d3d26bc3968cfc8 100644 (file)
@@ -2021,6 +2021,7 @@ static int do_write_index(struct index_state *istate, int newfd,
        int entries = istate->cache_nr;
        struct stat st;
        struct strbuf previous_name_buf = STRBUF_INIT, *previous_name;
+       int drop_cache_tree = 0;
 
        for (i = removed = extended = 0; i < entries; i++) {
                if (cache[i]->ce_flags & CE_REMOVE)
@@ -2071,6 +2072,8 @@ static int do_write_index(struct index_state *istate, int newfd,
                                warning(msg, ce->name);
                        else
                                return error(msg, ce->name);
+
+                       drop_cache_tree = 1;
                }
                if (ce_write_entry(&c, newfd, ce, previous_name) < 0)
                        return -1;
@@ -2089,7 +2092,7 @@ static int do_write_index(struct index_state *istate, int newfd,
                if (err)
                        return -1;
        }
-       if (!strip_extensions && istate->cache_tree) {
+       if (!strip_extensions && !drop_cache_tree && istate->cache_tree) {
                struct strbuf sb = STRBUF_INIT;
 
                cache_tree_write(&sb, istate->cache_tree);
diff --git a/t/t1601-index-bogus.sh b/t/t1601-index-bogus.sh
new file mode 100755 (executable)
index 0000000..73cc932
--- /dev/null
@@ -0,0 +1,22 @@
+#!/bin/sh
+
+test_description='test handling of bogus index entries'
+. ./test-lib.sh
+
+test_expect_success 'create tree with null sha1' '
+       tree=$(printf "160000 commit $_z40\\tbroken\\n" | git mktree)
+'
+
+test_expect_success 'read-tree refuses to read null sha1' '
+       test_must_fail git read-tree $tree
+'
+
+test_expect_success 'GIT_ALLOW_NULL_SHA1 overrides refusal' '
+       GIT_ALLOW_NULL_SHA1=1 git read-tree $tree
+'
+
+test_expect_success 'git write-tree refuses to write null sha1' '
+       test_must_fail git write-tree
+'
+
+test_done
index c27f90f285faf0529f08fae7262a40b605bd7375..a8d9ec498778b5a597fd5751f811c8bf0b0ab926 100755 (executable)
@@ -31,6 +31,12 @@ test_expect_success 'setup: bring HEAD and index in sync' '
        git commit -a -m "back to normal"
 '
 
+test_expect_success 'noop filter-branch complains' '
+       test_must_fail git filter-branch \
+               --force --prune-empty \
+               --index-filter "true"
+'
+
 test_expect_success 'filter commands are still checked' '
        test_must_fail git filter-branch \
                --force --prune-empty \