Skip to content

Commit

Permalink
sparse index: fix use-after-free bug in cache_tree_verify()
Browse files Browse the repository at this point in the history 
In a sparse index it is possible for the tree that is being verified
to be freed while it is being verified. This happens when the index is
sparse but the cache tree is not and index_name_pos() looks up a path
from the cache tree that is a descendant of a sparse index entry. That
triggers a call to ensure_full_index() which frees the cache tree that
is being verified.  Carrying on trying to verify the tree after this
results in a use-after-free bug. Instead restart the verification if a
sparse index is converted to a full index. This bug is triggered by a
call to reset_head() in "git rebase --apply". Thanks to René Scharfe
and Derrick Stolee for their help analyzing the problem.

==74345==ERROR: AddressSanitizer: heap-use-after-free on address 0x606000001b20 at pc 0x557cbe82d3a2 bp 0x7ffdfee08090 sp 0x7ffdfee08080
READ of size 4 at 0x606000001b20 thread T0
    #0 0x557cbe82d3a1 in verify_one /home/phil/src/git/cache-tree.c:863
    #1 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840
    #2 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840
    #3 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840
    #4 0x557cbe830a2b in cache_tree_verify /home/phil/src/git/cache-tree.c:910
    #5 0x557cbea53741 in write_locked_index /home/phil/src/git/read-cache.c:3250
    #6 0x557cbeab7fdd in reset_head /home/phil/src/git/reset.c:87
    #7 0x557cbe72147f in cmd_rebase builtin/rebase.c:2074
    #8 0x557cbe5bd151 in run_builtin /home/phil/src/git/git.c:461
    #9 0x557cbe5bd151 in handle_builtin /home/phil/src/git/git.c:714
    #10 0x557cbe5c0503 in run_argv /home/phil/src/git/git.c:781
    #11 0x557cbe5c0503 in cmd_main /home/phil/src/git/git.c:912
    #12 0x557cbe5bad28 in main /home/phil/src/git/common-main.c:52
    #13 0x7fdd4b82eb24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24)
    #14 0x557cbe5bcb8d in _start (/home/phil/src/git/git+0x1b9b8d)

0x606000001b20 is located 0 bytes inside of 56-byte region [0x606000001b20,0x606000001b58)
freed by thread T0 here:
    #0 0x7fdd4bacff19 in __interceptor_free /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:127
    #1 0x557cbe82af60 in cache_tree_free /home/phil/src/git/cache-tree.c:35
    #2 0x557cbe82aee5 in cache_tree_free /home/phil/src/git/cache-tree.c:31
    #3 0x557cbe82aee5 in cache_tree_free /home/phil/src/git/cache-tree.c:31
    #4 0x557cbe82aee5 in cache_tree_free /home/phil/src/git/cache-tree.c:31
    #5 0x557cbeb2557a in ensure_full_index /home/phil/src/git/sparse-index.c:310
    #6 0x557cbea45c4a in index_name_stage_pos /home/phil/src/git/read-cache.c:588
    #7 0x557cbe82ce37 in verify_one /home/phil/src/git/cache-tree.c:850
    #8 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840
    #9 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840
    #10 0x557cbe82ca9d in verify_one /home/phil/src/git/cache-tree.c:840
    #11 0x557cbe830a2b in cache_tree_verify /home/phil/src/git/cache-tree.c:910
    #12 0x557cbea53741 in write_locked_index /home/phil/src/git/read-cache.c:3250
    #13 0x557cbeab7fdd in reset_head /home/phil/src/git/reset.c:87
    #14 0x557cbe72147f in cmd_rebase builtin/rebase.c:2074
    #15 0x557cbe5bd151 in run_builtin /home/phil/src/git/git.c:461
    #16 0x557cbe5bd151 in handle_builtin /home/phil/src/git/git.c:714
    #17 0x557cbe5c0503 in run_argv /home/phil/src/git/git.c:781
    #18 0x557cbe5c0503 in cmd_main /home/phil/src/git/git.c:912
    #19 0x557cbe5bad28 in main /home/phil/src/git/common-main.c:52
    #20 0x7fdd4b82eb24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24)

previously allocated by thread T0 here:
    #0 0x7fdd4bad0459 in __interceptor_calloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:154
    #1 0x557cbebc1807 in xcalloc /home/phil/src/git/wrapper.c:140
    #2 0x557cbe82b7d8 in cache_tree /home/phil/src/git/cache-tree.c:17
    #3 0x557cbe82b7d8 in prime_cache_tree_rec /home/phil/src/git/cache-tree.c:763
    #4 0x557cbe82b837 in prime_cache_tree_rec /home/phil/src/git/cache-tree.c:764
    #5 0x557cbe82b837 in prime_cache_tree_rec /home/phil/src/git/cache-tree.c:764
    #6 0x557cbe8304e1 in prime_cache_tree /home/phil/src/git/cache-tree.c:779
    #7 0x557cbeab7fa7 in reset_head /home/phil/src/git/reset.c:85
    #8 0x557cbe72147f in cmd_rebase builtin/rebase.c:2074
    #9 0x557cbe5bd151 in run_builtin /home/phil/src/git/git.c:461
    #10 0x557cbe5bd151 in handle_builtin /home/phil/src/git/git.c:714
    #11 0x557cbe5c0503 in run_argv /home/phil/src/git/git.c:781
    #12 0x557cbe5c0503 in cmd_main /home/phil/src/git/git.c:912
    #13 0x557cbe5bad28 in main /home/phil/src/git/common-main.c:52
    #14 0x7fdd4b82eb24 in __libc_start_main (/usr/lib/libc.so.6+0x27b24)

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
  • Loading branch information
@phillipwood @derrickstolee
phillipwood authored and derrickstolee committed Oct 7, 2021
1 parent 6df9339 commit e9317e5
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 9 deletions.
37 changes: 29 additions & 8 deletions cache-tree.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -923,32 +923,48 @@ static void verify_one_sparse(struct repository *r,
path->buf);
}

static void verify_one(struct repository *r,
struct index_state *istate,
struct cache_tree *it,
struct strbuf *path)
/*
* Returns:
* 0 - Verification completed.
* 1 - Restart verification - a call to ensure_full_index() freed the cache
* tree that is being verified and verification needs to be restarted from
* the new toplevel cache tree.
*/
static int verify_one(struct repository *r,
struct index_state *istate,
struct cache_tree *it,
struct strbuf *path)
{
int i, pos, len = path->len;
struct strbuf tree_buf = STRBUF_INIT;
struct object_id new_oid;

for (i = 0; i < it->subtree_nr; i++) {
strbuf_addf(path, "%s/", it->down[i]->name);
verify_one(r, istate, it->down[i]->cache_tree, path);
if (verify_one(r, istate, it->down[i]->cache_tree, path))
return 1;
strbuf_setlen(path, len);
}

if (it->entry_count < 0 ||
/* no verification on tests (t7003) that replace trees */
lookup_replace_object(r, &it->oid) != &it->oid)
return;
return 0;

if (path->len) {
/*
* If the index is sparse and the cache tree is not
* index_name_pos() may trigger ensure_full_index() which will
* free the tree that is being verified.
*/
int is_sparse = istate->sparse_index;
pos = index_name_pos(istate, path->buf, path->len);
if (is_sparse && !istate->sparse_index)
return 1;

if (pos >= 0) {
verify_one_sparse(r, istate, it, path, pos);
return;
return 0;
}

pos = -pos - 1;
Expand Down Expand Up @@ -996,6 +1012,7 @@ static void verify_one(struct repository *r,
oid_to_hex(&new_oid), oid_to_hex(&it->oid));
strbuf_setlen(path, len);
strbuf_release(&tree_buf);
return 0;
}

void cache_tree_verify(struct repository *r, struct index_state *istate)
Expand All @@ -1004,6 +1021,10 @@ void cache_tree_verify(struct repository *r, struct index_state *istate)

if (!istate->cache_tree)
return;
verify_one(r, istate, istate->cache_tree, &path);
if (verify_one(r, istate, istate->cache_tree, &path)) {
strbuf_reset(&path);
if (verify_one(r, istate, istate->cache_tree, &path))
BUG("ensure_full_index() called twice while verifying cache tree");
}
strbuf_release(&path);
}
2 changes: 1 addition & 1 deletion t/t1092-sparse-checkout-compatibility.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -890,7 +890,7 @@ test_expect_success 'read-tree --merge with directory-file conflicts' '
test_expect_success 'merge, cherry-pick, and rebase' '
init_repos &&
for OPERATION in "merge -m merge" cherry-pick rebase
for OPERATION in "merge -m merge" cherry-pick "rebase -q --apply" "rebase --merge"
do
test_all_match git checkout -B temp update-deep &&
test_all_match git $OPERATION update-folder1 &&
Expand Down

0 comments on commit e9317e5

Please sign in to comment.