@matheusbsbs / matheus-bagliano / commits / 6e37c8ed3c

read-cache.c: fix writing "link" index ext with null base oid

Since commit 7db118303a (unpack_trees: fix breakage when o->src_index != o->dst_index - 2018-04-23) and changes in merge code to use separate index_state for source and destination, when doing a merge with split index activated, we may run into this line in unpack_trees(): o->result.split_index = init_split_index(&o->result); This is by itself not wrong. But this split index information is not fully populated (and it's only so when move_cache_to_base_index() is called, aka force splitting the index, or loading index_state from a file). Both "base_oid" and "base" in this case remain null. So when writing the main index down, we link to this index with null oid (default value after init_split_index()), which also means "no split index" internally. This triggers an incorrect base index refresh: warning: could not freshen shared index '.../sharedindex.0{40}' This patch makes sure we will not refresh null base_oid (because the file is never there). It also makes sure not to write "link" extension with null base_oid in the first place (no point having it at all). Read code already has protection against null base_oid. There is also another side fix in remove_split_index() that causes a crash when doing "git update-index --no-split-index" when base_oid in the index file is null. In this case we will not load istate->split_index->base but we dereference it anyway and are rewarded with a segfault. This should not happen anymore, but it's still wrong to dereference a potential NULL pointer, especially when we do check for NULL pointer in the next code. Reported-by: Luke Diamand <luke@diamand.org> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Nguyễn Thái Ngọc Duy committed Feb 13, 2019 at 16:51 UTC 6e37c8ed3c899385651f5beac1f1588fe3c1f5fb
3 files changed +39 -18
read-cache.c
+3 -2
@@ -2520,7 +2520,8 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
2520 return err;
2521
2522 /* Write extension data here */
2523 - if (!strip_extensions && istate->split_index) {
2523 + if (!strip_extensions && istate->split_index &&
2524 + !is_null_oid(&istate->split_index->base_oid)) {
2525 struct strbuf sb = STRBUF_INIT;
2526
2527 err = write_link_extension(&sb, istate) < 0 ||
@@ -2794,7 +2795,7 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,
2795 ret = write_split_index(istate, lock, flags);
2796
2797 /* Freshen the shared index only if the split-index was written */
2797 - if (!ret && !new_shared_index) {
2798 + if (!ret && !new_shared_index && !is_null_oid(&si->base_oid)) {
2799 const char *shared_index = git_path("sharedindex.%s",
2800 oid_to_hex(&si->base_oid));
2801 freshen_shared_index(shared_index, 1);
split-index.c
+18 -16
@@ -440,24 +440,26 @@ void add_split_index(struct index_state *istate)
440 void remove_split_index(struct index_state *istate)
441 {
442 if (istate->split_index) {
443 - /*
444 - * When removing the split index, we need to move
445 - * ownership of the mem_pool associated with the
446 - * base index to the main index. There may be cache entries
447 - * allocated from the base's memory pool that are shared with
448 - * the_index.cache[].
449 - */
450 - mem_pool_combine(istate->ce_mem_pool, istate->split_index->base->ce_mem_pool);
443 + if (istate->split_index->base) {
444 + /*
445 + * When removing the split index, we need to move
446 + * ownership of the mem_pool associated with the
447 + * base index to the main index. There may be cache entries
448 + * allocated from the base's memory pool that are shared with
449 + * the_index.cache[].
450 + */
451 + mem_pool_combine(istate->ce_mem_pool,
452 + istate->split_index->base->ce_mem_pool);
453
452 - /*
453 - * The split index no longer owns the mem_pool backing
454 - * its cache array. As we are discarding this index,
455 - * mark the index as having no cache entries, so it
456 - * will not attempt to clean up the cache entries or
457 - * validate them.
458 - */
459 - if (istate->split_index->base)
454 + /*
455 + * The split index no longer owns the mem_pool backing
456 + * its cache array. As we are discarding this index,
457 + * mark the index as having no cache entries, so it
458 + * will not attempt to clean up the cache entries or
459 + * validate them.
460 + */
461 istate->split_index->base->cache_nr = 0;
462 + }
463
464 /*
465 * We can discard the split index because its
t/t1700-split-index.sh
+18
@@ -447,4 +447,22 @@ test_expect_success 'writing split index with null sha1 does not write cache tre
447 test_line_count = 0 cache-tree.out
448 '
449
450 +test_expect_success 'do not refresh null base index' '
451 + test_create_repo merge &&
452 + (
453 + cd merge &&
454 + test_commit initial &&
455 + git checkout -b side-branch &&
456 + test_commit extra &&
457 + git checkout master &&
458 + git update-index --split-index &&
459 + test_commit more &&
460 + # must not write a new shareindex, or we wont catch the problem
461 + git -c splitIndex.maxPercentChange=100 merge --no-edit side-branch 2>err &&
462 + # i.e. do not expect warnings like
463 + # could not freshen shared index .../shareindex.00000...
464 + test_must_be_empty err
465 + )
466 +'
467 +
468 test_done