From 702b0411a41359fd6cacd33fc2eb95195b66ca1a Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Thu, 16 Dec 2021 16:13:40 +0000 Subject: [PATCH 1/6] sparse-checkout: fix segfault on malformed patterns Then core.sparseCheckoutCone is enabled, the sparse-checkout patterns are used to populate two hashsets that accelerate pattern matching. If the user modifies the sparse-checkout file outside of the 'sparse-checkout' builtin, then strange patterns can happen, triggering some error checks. One of these error checks is possible to hit when some special characters exist in a line. A warning message is correctly written to stderr, but then there is additional logic that attempts to remove the line from the hashset and free the data. This leads to a segfault in the 'git sparse-checkout list' command because it iterates over the contents of the hashset, which is now invalid. The fix here is to stop trying to remove from the hashset. In addition, we disable cone mode sparse-checkout because of the malformed data. This results in the pattern-matching working with a possibly-slower algorithm, but using the patterns as they are in the sparse-checkout file. This also changes the behavior of commands such as 'git sparse-checkout list' because the output patterns will be the contents of the sparse-checkout file instead of the list of directories. This is an existing behavior for other types of bad patterns. Add a test that triggers the segfault without the code change. Reported-by: John Burnett Reviewed-by: Elijah Newren Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano --- dir.c | 4 +--- t/t1091-sparse-checkout-builtin.sh | 21 +++++++++++++++++++++ 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/dir.c b/dir.c index 5aa6fbad0b76ad..7e72958d51dabb 100644 --- a/dir.c +++ b/dir.c @@ -819,9 +819,7 @@ static void add_pattern_to_hashsets(struct pattern_list *pl, struct path_pattern /* we already included this at the parent level */ warning(_("your sparse-checkout file may have issues: pattern '%s' is repeated"), given->pattern); - hashmap_remove(&pl->parent_hashmap, &translated->ent, &data); - free(data); - free(translated); + goto clear_hashmaps; } return; diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh index 272ba1b566b3ea..3921ea80138bf5 100755 --- a/t/t1091-sparse-checkout-builtin.sh +++ b/t/t1091-sparse-checkout-builtin.sh @@ -708,4 +708,25 @@ test_expect_success 'cone mode clears ignored subdirectories' ' test_cmp expect out ' +test_expect_success 'malformed cone-mode patterns' ' + git -C repo sparse-checkout init --cone && + mkdir -p repo/foo/bar && + touch repo/foo/bar/x repo/foo/y && + cat >repo/.git/info/sparse-checkout <<-\EOF && + /* + !/*/ + /foo/ + !/foo/*/ + /foo/\*/ + EOF + + # Listing the patterns will notice the duplicate pattern and + # emit a warning. It will list the patterns directly instead + # of using the cone-mode translation to a set of directories. + git -C repo sparse-checkout list >actual 2>err && + test_cmp repo/.git/info/sparse-checkout actual && + grep "warning: your sparse-checkout file may have issues: pattern .* is repeated" err && + grep "warning: disabling cone pattern matching" err +' + test_done From e4003ef7eabfe806fed9e4e88b3a46de3ab6b6ba Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Thu, 16 Dec 2021 16:13:41 +0000 Subject: [PATCH 2/6] sparse-checkout: fix OOM error with mixed patterns Add a test to t1091-sparse-checkout-builtin.sh that would result in an infinite loop and out-of-memory error before this change. The issue relies on having non-cone-mode patterns while trying to modify the patterns in cone-mode. The fix is simple, allowing us to break from the loop when the input path does not contain a slash, as the "dir" pattern we added does not. This is only a fix to the critical out-of-memory error. A better response to such a strange state will follow in a later change. Reported-by: Calbabreaker Helped-by: Taylor Blau Reviewed-by: Elijah Newren Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano --- builtin/sparse-checkout.c | 2 +- t/t1091-sparse-checkout-builtin.sh | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c index d0f5c4702be69d..9ccdcde9832e93 100644 --- a/builtin/sparse-checkout.c +++ b/builtin/sparse-checkout.c @@ -483,7 +483,7 @@ static void insert_recursive_pattern(struct pattern_list *pl, struct strbuf *pat char *oldpattern = e->pattern; size_t newlen; - if (slash == e->pattern) + if (!slash || slash == e->pattern) break; newlen = slash - e->pattern; diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh index 3921ea80138bf5..1f877ced0c887c 100755 --- a/t/t1091-sparse-checkout-builtin.sh +++ b/t/t1091-sparse-checkout-builtin.sh @@ -103,6 +103,17 @@ test_expect_success 'clone --sparse' ' check_files clone a ' +test_expect_success 'switching to cone mode with non-cone mode patterns' ' + git init bad-patterns && + ( + cd bad-patterns && + git sparse-checkout init && + git sparse-checkout add dir && + git config core.sparseCheckoutCone true && + git sparse-checkout add dir + ) +' + test_expect_success 'interaction with clone --no-checkout (unborn index)' ' git clone --no-checkout "file://$(pwd)/repo" clone_no_checkout && git -C clone_no_checkout sparse-checkout init --cone && From f40d382072d7eebe17fbccc8d212af58958a5dce Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Thu, 16 Dec 2021 16:13:42 +0000 Subject: [PATCH 3/6] sparse-checkout: refuse to add to bad patterns When in cone mode sparse-checkout, it is unclear how 'git sparse-checkout add ...' should behave if the existing sparse-checkout file does not match the cone mode patterns. Change the behavior to fail with an error message about the existing patterns. Also, all cone mode patterns start with a '/' character, so add that restriction. This is necessary for our example test 'cone mode: warn on bad pattern', but also requires modifying the example sparse-checkout file we use to test the warnings related to recognizing cone mode patterns. This error checking would cause a failure further down the test script because of a test that adds non-cone mode patterns without cleaning them up. Perform that cleanup as part of the test now. Reviewed-by: Elijah Newren Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano --- builtin/sparse-checkout.c | 3 +++ dir.c | 2 +- t/t1091-sparse-checkout-builtin.sh | 7 +++++-- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c index 9ccdcde9832e93..6580075a63181d 100644 --- a/builtin/sparse-checkout.c +++ b/builtin/sparse-checkout.c @@ -598,6 +598,9 @@ static void add_patterns_cone_mode(int argc, const char **argv, die(_("unable to load existing sparse-checkout patterns")); free(sparse_filename); + if (!existing.use_cone_patterns) + die(_("existing sparse-checkout patterns do not use cone mode")); + hashmap_for_each_entry(&existing.recursive_hashmap, &iter, pe, ent) { if (!hashmap_contains_parent(&pl->recursive_hashmap, pe->pattern, &buffer) || diff --git a/dir.c b/dir.c index 7e72958d51dabb..eca75e89213ec8 100644 --- a/dir.c +++ b/dir.c @@ -727,7 +727,7 @@ static void add_pattern_to_hashsets(struct pattern_list *pl, struct path_pattern } if (given->patternlen < 2 || - *given->pattern == '*' || + *given->pattern != '/' || strstr(given->pattern, "**")) { /* Not a cone pattern. */ warning(_("unrecognized pattern: '%s'"), given->pattern); diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh index 1f877ced0c887c..34b97fd3ee0857 100755 --- a/t/t1091-sparse-checkout-builtin.sh +++ b/t/t1091-sparse-checkout-builtin.sh @@ -110,7 +110,8 @@ test_expect_success 'switching to cone mode with non-cone mode patterns' ' git sparse-checkout init && git sparse-checkout add dir && git config core.sparseCheckoutCone true && - git sparse-checkout add dir + test_must_fail git sparse-checkout add dir 2>err && + grep "existing sparse-checkout patterns do not use cone mode" err ) ' @@ -176,12 +177,14 @@ test_expect_success 'set sparse-checkout using --stdin' ' ' test_expect_success 'add to sparse-checkout' ' - cat repo/.git/info/sparse-checkout >expect && + cat repo/.git/info/sparse-checkout >old && + test_when_finished cp old repo/.git/info/sparse-checkout && cat >add <<-\EOF && pattern1 /folder1/ pattern2 EOF + cat old >expect && cat add >>expect && git -C repo sparse-checkout add --stdin actual && From d33146878e0ea21d68961cb79b35e904c96a40e9 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Sat, 25 Dec 2021 14:40:23 -0800 Subject: [PATCH 4/6] fixup! sparse-checkout: fix OOM error with mixed patterns --- t/t1091-sparse-checkout-builtin.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh index 34b97fd3ee0857..ef16c7a47536d9 100755 --- a/t/t1091-sparse-checkout-builtin.sh +++ b/t/t1091-sparse-checkout-builtin.sh @@ -109,7 +109,7 @@ test_expect_success 'switching to cone mode with non-cone mode patterns' ' cd bad-patterns && git sparse-checkout init && git sparse-checkout add dir && - git config core.sparseCheckoutCone true && + git config --worktree core.sparseCheckoutCone true && test_must_fail git sparse-checkout add dir 2>err && grep "existing sparse-checkout patterns do not use cone mode" err ) From 027d9e1b948e9bce598d670634bb2f662c709cef Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 30 Dec 2021 13:55:31 -0800 Subject: [PATCH 5/6] Revert "fixup! sparse-checkout: fix OOM error with mixed patterns" This reverts commit d33146878e0ea21d68961cb79b35e904c96a40e9, which will not be needed with the updated version of another topic that the test was having trouble with. --- t/t1091-sparse-checkout-builtin.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh index f4d5a0d6e70ac6..32a6328ddb69f5 100755 --- a/t/t1091-sparse-checkout-builtin.sh +++ b/t/t1091-sparse-checkout-builtin.sh @@ -117,7 +117,7 @@ test_expect_success 'switching to cone mode with non-cone mode patterns' ' cd bad-patterns && git sparse-checkout init && git sparse-checkout add dir && - git config --worktree core.sparseCheckoutCone true && + git config core.sparseCheckoutCone true && test_must_fail git sparse-checkout add dir 2>err && grep "existing sparse-checkout patterns do not use cone mode" err ) From 9f3ce1d8c68eb3aa1b8a00ed5cae6db5184310ad Mon Sep 17 00:00:00 2001 From: John Cai Date: Sat, 1 Jan 2022 17:10:21 -0800 Subject: [PATCH 6/6] add-patch.c: compare oid of rev and HEAD instead of string literals Instead of comparing to the literal "HEAD", compare the commit objects instead so that other ways of saying the same thing (such as "@") are also handled appropriately. Signed-off-by: "John Cai" --- add-patch.c | 24 +++++++++++++----------- t/t2016-checkout-patch.sh | 8 ++++++++ 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/add-patch.c b/add-patch.c index 573eef0cc4a866..bed14079da778a 100644 --- a/add-patch.c +++ b/add-patch.c @@ -8,6 +8,7 @@ #include "diff.h" #include "compat/terminal.h" #include "prompt.h" +#include "refs.h" enum prompt_mode_type { PROMPT_MODE_CHANGE = 0, PROMPT_DELETION, PROMPT_ADDITION, PROMPT_HUNK, @@ -1695,35 +1696,36 @@ int run_add_p(struct repository *r, enum add_p_mode mode, { r }, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT }; size_t i, binary_count = 0; + struct object_id rev_oid, head_oid; + int rev_equals_head = 0; init_add_i_state(&s.s, r); if (mode == ADD_P_STASH) s.mode = &patch_mode_stash; else if (mode == ADD_P_RESET) { - /* - * NEEDSWORK: Instead of comparing to the literal "HEAD", - * compare the commit objects instead so that other ways of - * saying the same thing (such as "@") are also handled - * appropriately. - * - * This applies to the cases below too. - */ - if (!revision || !strcmp(revision, "HEAD")) + if (!resolve_ref_unsafe("HEAD", RESOLVE_REF_READING, &head_oid, NULL)) + die("no such ref: HEAD"); + if (revision) { + if (!resolve_ref_unsafe(revision, RESOLVE_REF_READING, &rev_oid, NULL)) + die("no such ref: %s", revision); + rev_equals_head = strcmp(oid_to_hex(&rev_oid), oid_to_hex(&head_oid)); + } + if (!revision || !rev_equals_head) s.mode = &patch_mode_reset_head; else s.mode = &patch_mode_reset_nothead; } else if (mode == ADD_P_CHECKOUT) { if (!revision) s.mode = &patch_mode_checkout_index; - else if (!strcmp(revision, "HEAD")) + else if (!rev_equals_head) s.mode = &patch_mode_checkout_head; else s.mode = &patch_mode_checkout_nothead; } else if (mode == ADD_P_WORKTREE) { if (!revision) s.mode = &patch_mode_checkout_index; - else if (!strcmp(revision, "HEAD")) + else if (!rev_equals_head) s.mode = &patch_mode_worktree_head; else s.mode = &patch_mode_worktree_nothead; diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh index abfd586c32b017..b61c56f505ac32 100755 --- a/t/t2016-checkout-patch.sh +++ b/t/t2016-checkout-patch.sh @@ -63,6 +63,14 @@ test_expect_success PERL 'git checkout -p HEAD with change already staged' ' verify_state dir/foo head head ' +test_expect_success PERL 'git checkout -p @ with change already staged' ' + set_state dir/foo index index && + # the third n is to get out in case it mistakenly does not apply + test_write_lines n y n | git checkout -p @ && + verify_saved_state bar && + verify_state dir/foo head head +' + test_expect_success PERL 'git checkout -p HEAD^...' ' # the third n is to get out in case it mistakenly does not apply test_write_lines n y n | git checkout -p HEAD^... &&