@matheusbsbs / matheus-bagliano / commits / a96d3cc3f6

cache-tree: reject entries with null sha1

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>

Jeff King committed Apr 21, 2017 at 14:46 UTC a96d3cc3f65285f7d51c30fb3f630202b5347a57

4 files changed +35 -2

cache-tree.c

+3 -1

@@ -354,7 +354,9 @@ static int update_one(struct cache_tree *it,

354 entlen = pathlen - baselen;

355 i++;

356 }

357 - if (mode != S_IFGITLINK && !missing_ok && !has_sha1_file(sha1)) {

357 +

358 + if (is_null_sha1(sha1) ||

359 + (mode != S_IFGITLINK && !missing_ok && !has_sha1_file(sha1))) {

360 strbuf_release(&buffer);

361 if (expected_missing)

362 return -1;

read-cache.c

+4 -1

@@ -2021,6 +2021,7 @@ static int do_write_index(struct index_state *istate, int newfd,

2021 int entries = istate->cache_nr;

2022 struct stat st;

2023 struct strbuf previous_name_buf = STRBUF_INIT, *previous_name;

2024 + int drop_cache_tree = 0;

2025

2026 for (i = removed = extended = 0; i < entries; i++) {

2027 if (cache[i]->ce_flags & CE_REMOVE)

@@ -2071,6 +2072,8 @@ static int do_write_index(struct index_state *istate, int newfd,

2072 warning(msg, ce->name);

2073 else

2074 return error(msg, ce->name);

2075 +

2076 + drop_cache_tree = 1;

2077 }

2078 if (ce_write_entry(&c, newfd, ce, previous_name) < 0)

2079 return -1;

@@ -2089,7 +2092,7 @@ static int do_write_index(struct index_state *istate, int newfd,

2092 if (err)

2093 return -1;

2094 }

2092 - if (!strip_extensions && istate->cache_tree) {

2095 + if (!strip_extensions && !drop_cache_tree && istate->cache_tree) {

2096 struct strbuf sb = STRBUF_INIT;

2097

2098 cache_tree_write(&sb, istate->cache_tree);

t/t1601-index-bogus.sh new

+22

@@ -0,0 +1,22 @@

1 +#!/bin/sh

2 +

3 +test_description='test handling of bogus index entries'

4 +. ./test-lib.sh

5 +

6 +test_expect_success 'create tree with null sha1' '

7 + tree=$(printf "160000 commit $_z40\\tbroken\\n" | git mktree)

8 +'

9 +

10 +test_expect_success 'read-tree refuses to read null sha1' '

11 + test_must_fail git read-tree $tree

12 +'

13 +

14 +test_expect_success 'GIT_ALLOW_NULL_SHA1 overrides refusal' '

15 + GIT_ALLOW_NULL_SHA1=1 git read-tree $tree

16 +'

17 +

18 +test_expect_success 'git write-tree refuses to write null sha1' '

19 + test_must_fail git write-tree

20 +'

21 +

22 +test_done

t/t7009-filter-branch-null-sha1.sh

@@ -31,6 +31,12 @@ test_expect_success 'setup: bring HEAD and index in sync' '

31 git commit -a -m "back to normal"

32 '

34 +test_expect_success 'noop filter-branch complains' '

35 + test_must_fail git filter-branch \

36 + --force --prune-empty \

37 + --index-filter "true"

38 +'

39 +

40 test_expect_success 'filter commands are still checked' '

41 test_must_fail git filter-branch \

42 --force --prune-empty \