* [PATCH 0/3] sparse-checkout: fix segfault on malformed patterns @ 2021-12-07 20:02 Derrick Stolee via GitGitGadget 2021-12-07 20:02 ` [PATCH 1/3] " Derrick Stolee via GitGitGadget ` (4 more replies) 0 siblings, 5 replies; 26+ messages in thread From: Derrick Stolee via GitGitGadget @ 2021-12-07 20:02 UTC (permalink / raw) To: git; +Cc: me, newren, vdye, Derrick Stolee This series fixes some issues with parsing sparse-checkout patterns when core.sparseCheckoutCone is enabled but the sparse-checkout file itself contains patterns that don't match the cone mode format. The first patch fixes a segfault first reported in [1]. The other two patches are from an earlier submission [2] that never got picked up and I lost track of. There was another patch involving 'git sparse-checkout init --cone' that isn't necessary, especially with Elijah doing some work in that space right now. [1] https://github.com/git-for-windows/git/issues/3498 [2] https://lore.kernel.org/git/pull.1043.git.1632160658.gitgitgadget@gmail.com Thanks, -Stolee Derrick Stolee (3): sparse-checkout: fix segfault on malformed patterns sparse-checkout: fix OOM error with mixed patterns sparse-checkout: refuse to add to bad patterns builtin/sparse-checkout.c | 5 ++++- dir.c | 5 +---- t/t1091-sparse-checkout-builtin.sh | 31 +++++++++++++++++++++++++++++- 3 files changed, 35 insertions(+), 6 deletions(-) base-commit: abe6bb3905392d5eb6b01fa6e54d7e784e0522aa Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1069%2Fderrickstolee%2Fsparse-checkout%2Finput-bug-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1069/derrickstolee/sparse-checkout/input-bug-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/1069 -- gitgitgadget ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/3] sparse-checkout: fix segfault on malformed patterns 2021-12-07 20:02 [PATCH 0/3] sparse-checkout: fix segfault on malformed patterns Derrick Stolee via GitGitGadget @ 2021-12-07 20:02 ` Derrick Stolee via GitGitGadget 2021-12-07 20:22 ` Elijah Newren 2021-12-07 20:02 ` [PATCH 2/3] sparse-checkout: fix OOM error with mixed patterns Derrick Stolee via GitGitGadget ` (3 subsequent siblings) 4 siblings, 1 reply; 26+ messages in thread From: Derrick Stolee via GitGitGadget @ 2021-12-07 20:02 UTC (permalink / raw) To: git; +Cc: me, newren, vdye, Derrick Stolee, Derrick Stolee From: Derrick Stolee <dstolee@microsoft.com> 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 no invalid. The fix here is to stop trying to remove from the hashset. Better to leave bad data in the sparse-checkout matching logic (with a warning) than to segfault. If we are in this state, then we are already traversing into undefined behavior, so this change to keep the entry in the hashset is no worse than removing it. Add a test that triggers the segfault without the code change. Reported-by: John Burnett <johnburnett@johnburnett.com> Signed-off-by: Derrick Stolee <dstolee@microsoft.com> --- dir.c | 3 --- t/t1091-sparse-checkout-builtin.sh | 15 +++++++++++++++ 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/dir.c b/dir.c index 5aa6fbad0b7..0693c7cb3ee 100644 --- a/dir.c +++ b/dir.c @@ -819,9 +819,6 @@ 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); } return; diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh index 272ba1b566b..c72b8ee2e7b 100755 --- a/t/t1091-sparse-checkout-builtin.sh +++ b/t/t1091-sparse-checkout-builtin.sh @@ -708,4 +708,19 @@ 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 + cat repo/.git/info/sparse-checkout && + git -C repo sparse-checkout list +' + test_done -- gitgitgadget ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 1/3] sparse-checkout: fix segfault on malformed patterns 2021-12-07 20:02 ` [PATCH 1/3] " Derrick Stolee via GitGitGadget @ 2021-12-07 20:22 ` Elijah Newren 0 siblings, 0 replies; 26+ messages in thread From: Elijah Newren @ 2021-12-07 20:22 UTC (permalink / raw) To: Derrick Stolee via GitGitGadget Cc: Git Mailing List, Taylor Blau, Victoria Dye, Derrick Stolee, Derrick Stolee On Tue, Dec 7, 2021 at 12:02 PM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: Derrick Stolee <dstolee@microsoft.com> > > 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 > no invalid. s/no invalid/now invalid/ ? > > The fix here is to stop trying to remove from the hashset. Better to leave > bad data in the sparse-checkout matching logic (with a warning) than to > segfault. If we are in this state, then we are already traversing into > undefined behavior, so this change to keep the entry in the hashset is no > worse than removing it. > > Add a test that triggers the segfault without the code change. > > Reported-by: John Burnett <johnburnett@johnburnett.com> > Signed-off-by: Derrick Stolee <dstolee@microsoft.com> > --- > dir.c | 3 --- > t/t1091-sparse-checkout-builtin.sh | 15 +++++++++++++++ > 2 files changed, 15 insertions(+), 3 deletions(-) > > diff --git a/dir.c b/dir.c > index 5aa6fbad0b7..0693c7cb3ee 100644 > --- a/dir.c > +++ b/dir.c > @@ -819,9 +819,6 @@ 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); > } > > return; > diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh > index 272ba1b566b..c72b8ee2e7b 100755 > --- a/t/t1091-sparse-checkout-builtin.sh > +++ b/t/t1091-sparse-checkout-builtin.sh > @@ -708,4 +708,19 @@ 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 > + cat repo/.git/info/sparse-checkout && > + git -C repo sparse-checkout list > +' > + > test_done > -- > gitgitgadget > ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 2/3] sparse-checkout: fix OOM error with mixed patterns 2021-12-07 20:02 [PATCH 0/3] sparse-checkout: fix segfault on malformed patterns Derrick Stolee via GitGitGadget 2021-12-07 20:02 ` [PATCH 1/3] " Derrick Stolee via GitGitGadget @ 2021-12-07 20:02 ` Derrick Stolee via GitGitGadget 2021-12-07 20:02 ` [PATCH 3/3] sparse-checkout: refuse to add to bad patterns Derrick Stolee via GitGitGadget ` (2 subsequent siblings) 4 siblings, 0 replies; 26+ messages in thread From: Derrick Stolee via GitGitGadget @ 2021-12-07 20:02 UTC (permalink / raw) To: git; +Cc: me, newren, vdye, Derrick Stolee, Derrick Stolee From: Derrick Stolee <dstolee@microsoft.com> 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 <calbabreaker@gmail.com> Helped-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Derrick Stolee <dstolee@microsoft.com> --- 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 d0f5c4702be..9ccdcde9832 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 c72b8ee2e7b..67ce24c9c83 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 && -- gitgitgadget ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 3/3] sparse-checkout: refuse to add to bad patterns 2021-12-07 20:02 [PATCH 0/3] sparse-checkout: fix segfault on malformed patterns Derrick Stolee via GitGitGadget 2021-12-07 20:02 ` [PATCH 1/3] " Derrick Stolee via GitGitGadget 2021-12-07 20:02 ` [PATCH 2/3] sparse-checkout: fix OOM error with mixed patterns Derrick Stolee via GitGitGadget @ 2021-12-07 20:02 ` Derrick Stolee via GitGitGadget 2021-12-07 21:51 ` [PATCH 0/3] sparse-checkout: fix segfault on malformed patterns Elijah Newren 2021-12-10 15:18 ` [PATCH v2 0/4] " Derrick Stolee via GitGitGadget 4 siblings, 0 replies; 26+ messages in thread From: Derrick Stolee via GitGitGadget @ 2021-12-07 20:02 UTC (permalink / raw) To: git; +Cc: me, newren, vdye, Derrick Stolee, Derrick Stolee From: Derrick Stolee <dstolee@microsoft.com> When in cone mode sparse-checkout, it is unclear how 'git sparse-checkout add <dir1> ...' 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. Signed-off-by: Derrick Stolee <dstolee@microsoft.com> --- 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 9ccdcde9832..6580075a631 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 0693c7cb3ee..a5dddafa16d 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 67ce24c9c83..2ed247d75a5 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 <add && git -C repo sparse-checkout list >actual && -- gitgitgadget ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 0/3] sparse-checkout: fix segfault on malformed patterns 2021-12-07 20:02 [PATCH 0/3] sparse-checkout: fix segfault on malformed patterns Derrick Stolee via GitGitGadget ` (2 preceding siblings ...) 2021-12-07 20:02 ` [PATCH 3/3] sparse-checkout: refuse to add to bad patterns Derrick Stolee via GitGitGadget @ 2021-12-07 21:51 ` Elijah Newren 2021-12-08 14:23 ` Derrick Stolee 2021-12-10 15:18 ` [PATCH v2 0/4] " Derrick Stolee via GitGitGadget 4 siblings, 1 reply; 26+ messages in thread From: Elijah Newren @ 2021-12-07 21:51 UTC (permalink / raw) To: Derrick Stolee via GitGitGadget Cc: Git Mailing List, Taylor Blau, Victoria Dye, Derrick Stolee On Tue, Dec 7, 2021 at 12:02 PM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote: > > This series fixes some issues with parsing sparse-checkout patterns when > core.sparseCheckoutCone is enabled but the sparse-checkout file itself > contains patterns that don't match the cone mode format. I was only able to find what I think is a small typo in one of the commits. Everything else looks good to me. > The first patch fixes a segfault first reported in [1]. The other two > patches are from an earlier submission [2] that never got picked up and I > lost track of. There was another patch involving 'git sparse-checkout init Sorry for missing that series earlier. Glad we've got some of them now. > --cone' that isn't necessary, especially with Elijah doing some work in that > space right now. > > [1] https://github.com/git-for-windows/git/issues/3498 [2] > https://lore.kernel.org/git/pull.1043.git.1632160658.gitgitgadget@gmail.com > > Thanks, -Stolee > > Derrick Stolee (3): > sparse-checkout: fix segfault on malformed patterns > sparse-checkout: fix OOM error with mixed patterns > sparse-checkout: refuse to add to bad patterns > > builtin/sparse-checkout.c | 5 ++++- > dir.c | 5 +---- > t/t1091-sparse-checkout-builtin.sh | 31 +++++++++++++++++++++++++++++- > 3 files changed, 35 insertions(+), 6 deletions(-) > > > base-commit: abe6bb3905392d5eb6b01fa6e54d7e784e0522aa > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1069%2Fderrickstolee%2Fsparse-checkout%2Finput-bug-v1 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1069/derrickstolee/sparse-checkout/input-bug-v1 > Pull-Request: https://github.com/gitgitgadget/git/pull/1069 > -- > gitgitgadget ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 0/3] sparse-checkout: fix segfault on malformed patterns 2021-12-07 21:51 ` [PATCH 0/3] sparse-checkout: fix segfault on malformed patterns Elijah Newren @ 2021-12-08 14:23 ` Derrick Stolee 0 siblings, 0 replies; 26+ messages in thread From: Derrick Stolee @ 2021-12-08 14:23 UTC (permalink / raw) To: Elijah Newren, Derrick Stolee via GitGitGadget Cc: Git Mailing List, Taylor Blau, Victoria Dye, Derrick Stolee On 12/7/2021 4:51 PM, Elijah Newren wrote: > On Tue, Dec 7, 2021 at 12:02 PM Derrick Stolee via GitGitGadget > <gitgitgadget@gmail.com> wrote: >> >> This series fixes some issues with parsing sparse-checkout patterns when >> core.sparseCheckoutCone is enabled but the sparse-checkout file itself >> contains patterns that don't match the cone mode format. > > I was only able to find what I think is a small typo in one of the > commits. Everything else looks good to me. Thanks. I'll give this a few more days for more feedback before sending a v2 with that fix. >> The first patch fixes a segfault first reported in [1]. The other two >> patches are from an earlier submission [2] that never got picked up and I >> lost track of. There was another patch involving 'git sparse-checkout init > > Sorry for missing that series earlier. Glad we've got some of them now. The fault was my own for dropping this during a particularly busy time. Thanks, -Stolee ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 0/4] sparse-checkout: fix segfault on malformed patterns 2021-12-07 20:02 [PATCH 0/3] sparse-checkout: fix segfault on malformed patterns Derrick Stolee via GitGitGadget ` (3 preceding siblings ...) 2021-12-07 21:51 ` [PATCH 0/3] sparse-checkout: fix segfault on malformed patterns Elijah Newren @ 2021-12-10 15:18 ` Derrick Stolee via GitGitGadget 2021-12-10 15:18 ` [PATCH v2 1/4] " Derrick Stolee via GitGitGadget ` (3 more replies) 4 siblings, 4 replies; 26+ messages in thread From: Derrick Stolee via GitGitGadget @ 2021-12-10 15:18 UTC (permalink / raw) To: git; +Cc: me, newren, vdye, Derrick Stolee, Derrick Stolee This series fixes some issues with parsing sparse-checkout patterns when core.sparseCheckoutCone is enabled but the sparse-checkout file itself contains patterns that don't match the cone mode format. The first patch fixes a segfault first reported in [1]. The other two patches are from an earlier submission [2] that never got picked up and I lost track of. There was another patch involving 'git sparse-checkout init --cone' that isn't necessary, especially with Elijah doing some work in that space right now. [1] https://github.com/git-for-windows/git/issues/3498 [2] https://lore.kernel.org/git/pull.1043.git.1632160658.gitgitgadget@gmail.com Thanks, -Stolee Derrick Stolee (4): sparse-checkout: fix segfault on malformed patterns sparse-checkout: fix OOM error with mixed patterns sparse-checkout: refuse to add to bad patterns amend! sparse-checkout: fix segfault on malformed patterns builtin/sparse-checkout.c | 5 ++++- dir.c | 5 +---- t/t1091-sparse-checkout-builtin.sh | 31 +++++++++++++++++++++++++++++- 3 files changed, 35 insertions(+), 6 deletions(-) base-commit: abe6bb3905392d5eb6b01fa6e54d7e784e0522aa Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1069%2Fderrickstolee%2Fsparse-checkout%2Finput-bug-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1069/derrickstolee/sparse-checkout/input-bug-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/1069 Range-diff vs v1: 1: becbee16d2e ! 1: a0e3dd335c9 sparse-checkout: fix segfault on malformed patterns @@ Commit message Add a test that triggers the segfault without the code change. Reported-by: John Burnett <johnburnett@johnburnett.com> + Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Derrick Stolee <dstolee@microsoft.com> ## dir.c ## 2: 239bf23eacb ! 2: 86fbf130c03 sparse-checkout: fix OOM error with mixed patterns @@ Commit message Reported-by: Calbabreaker <calbabreaker@gmail.com> Helped-by: Taylor Blau <me@ttaylorr.com> + Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Derrick Stolee <dstolee@microsoft.com> ## builtin/sparse-checkout.c ## 3: cc52fb2b0b7 ! 3: 5d096e380a4 sparse-checkout: refuse to add to bad patterns @@ Commit message 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 <newren@gmail.com> Signed-off-by: Derrick Stolee <dstolee@microsoft.com> ## builtin/sparse-checkout.c ## -: ----------- > 4: 7bacb3760f3 amend! sparse-checkout: fix segfault on malformed patterns -- gitgitgadget ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 1/4] sparse-checkout: fix segfault on malformed patterns 2021-12-10 15:18 ` [PATCH v2 0/4] " Derrick Stolee via GitGitGadget @ 2021-12-10 15:18 ` Derrick Stolee via GitGitGadget 2021-12-10 15:18 ` [PATCH v2 2/4] sparse-checkout: fix OOM error with mixed patterns Derrick Stolee via GitGitGadget ` (2 subsequent siblings) 3 siblings, 0 replies; 26+ messages in thread From: Derrick Stolee via GitGitGadget @ 2021-12-10 15:18 UTC (permalink / raw) To: git; +Cc: me, newren, vdye, Derrick Stolee, Derrick Stolee, Derrick Stolee From: Derrick Stolee <dstolee@microsoft.com> 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 no invalid. The fix here is to stop trying to remove from the hashset. Better to leave bad data in the sparse-checkout matching logic (with a warning) than to segfault. If we are in this state, then we are already traversing into undefined behavior, so this change to keep the entry in the hashset is no worse than removing it. Add a test that triggers the segfault without the code change. Reported-by: John Burnett <johnburnett@johnburnett.com> Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Derrick Stolee <dstolee@microsoft.com> --- dir.c | 3 --- t/t1091-sparse-checkout-builtin.sh | 15 +++++++++++++++ 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/dir.c b/dir.c index 5aa6fbad0b7..0693c7cb3ee 100644 --- a/dir.c +++ b/dir.c @@ -819,9 +819,6 @@ 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); } return; diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh index 272ba1b566b..c72b8ee2e7b 100755 --- a/t/t1091-sparse-checkout-builtin.sh +++ b/t/t1091-sparse-checkout-builtin.sh @@ -708,4 +708,19 @@ 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 + cat repo/.git/info/sparse-checkout && + git -C repo sparse-checkout list +' + test_done -- gitgitgadget ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 2/4] sparse-checkout: fix OOM error with mixed patterns 2021-12-10 15:18 ` [PATCH v2 0/4] " Derrick Stolee via GitGitGadget 2021-12-10 15:18 ` [PATCH v2 1/4] " Derrick Stolee via GitGitGadget @ 2021-12-10 15:18 ` Derrick Stolee via GitGitGadget 2021-12-10 15:18 ` [PATCH v2 3/4] sparse-checkout: refuse to add to bad patterns Derrick Stolee via GitGitGadget 2021-12-15 13:46 ` [PATCH v3 0/3] sparse-checkout: fix segfault on malformed patterns Derrick Stolee via GitGitGadget 3 siblings, 0 replies; 26+ messages in thread From: Derrick Stolee via GitGitGadget @ 2021-12-10 15:18 UTC (permalink / raw) To: git; +Cc: me, newren, vdye, Derrick Stolee, Derrick Stolee, Derrick Stolee From: Derrick Stolee <dstolee@microsoft.com> 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 <calbabreaker@gmail.com> Helped-by: Taylor Blau <me@ttaylorr.com> Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Derrick Stolee <dstolee@microsoft.com> --- 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 d0f5c4702be..9ccdcde9832 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 c72b8ee2e7b..67ce24c9c83 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 && -- gitgitgadget ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 3/4] sparse-checkout: refuse to add to bad patterns 2021-12-10 15:18 ` [PATCH v2 0/4] " Derrick Stolee via GitGitGadget 2021-12-10 15:18 ` [PATCH v2 1/4] " Derrick Stolee via GitGitGadget 2021-12-10 15:18 ` [PATCH v2 2/4] sparse-checkout: fix OOM error with mixed patterns Derrick Stolee via GitGitGadget @ 2021-12-10 15:18 ` Derrick Stolee via GitGitGadget 2021-12-15 13:46 ` [PATCH v3 0/3] sparse-checkout: fix segfault on malformed patterns Derrick Stolee via GitGitGadget 3 siblings, 0 replies; 26+ messages in thread From: Derrick Stolee via GitGitGadget @ 2021-12-10 15:18 UTC (permalink / raw) To: git; +Cc: me, newren, vdye, Derrick Stolee, Derrick Stolee, Derrick Stolee From: Derrick Stolee <dstolee@microsoft.com> When in cone mode sparse-checkout, it is unclear how 'git sparse-checkout add <dir1> ...' 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 <newren@gmail.com> Signed-off-by: Derrick Stolee <dstolee@microsoft.com> --- 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 9ccdcde9832..6580075a631 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 0693c7cb3ee..a5dddafa16d 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 67ce24c9c83..2ed247d75a5 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 <add && git -C repo sparse-checkout list >actual && -- gitgitgadget ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v3 0/3] sparse-checkout: fix segfault on malformed patterns 2021-12-10 15:18 ` [PATCH v2 0/4] " Derrick Stolee via GitGitGadget ` (2 preceding siblings ...) 2021-12-10 15:18 ` [PATCH v2 3/4] sparse-checkout: refuse to add to bad patterns Derrick Stolee via GitGitGadget @ 2021-12-15 13:46 ` Derrick Stolee via GitGitGadget 2021-12-15 13:46 ` [PATCH v3 1/3] " Derrick Stolee via GitGitGadget ` (4 more replies) 3 siblings, 5 replies; 26+ messages in thread From: Derrick Stolee via GitGitGadget @ 2021-12-15 13:46 UTC (permalink / raw) To: git; +Cc: me, newren, vdye, Derrick Stolee, Derrick Stolee This series fixes some issues with parsing sparse-checkout patterns when core.sparseCheckoutCone is enabled but the sparse-checkout file itself contains patterns that don't match the cone mode format. The first patch fixes a segfault first reported in [1]. The other two patches are from an earlier submission [2] that never got picked up and I lost track of. There was another patch involving 'git sparse-checkout init --cone' that isn't necessary, especially with Elijah doing some work in that space right now. [1] https://github.com/git-for-windows/git/issues/3498 [2] https://lore.kernel.org/git/pull.1043.git.1632160658.gitgitgadget@gmail.com Thanks, -Stolee Updates in v2 and v3 ==================== * I intended to fix a typo in a patch, but accidentally sent the amend! commit in v2 * v3 has the typo fix properly squashed in. * Added Elijah's review. Derrick Stolee (3): sparse-checkout: fix segfault on malformed patterns sparse-checkout: fix OOM error with mixed patterns sparse-checkout: refuse to add to bad patterns builtin/sparse-checkout.c | 5 ++++- dir.c | 5 +---- t/t1091-sparse-checkout-builtin.sh | 31 +++++++++++++++++++++++++++++- 3 files changed, 35 insertions(+), 6 deletions(-) base-commit: abe6bb3905392d5eb6b01fa6e54d7e784e0522aa Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1069%2Fderrickstolee%2Fsparse-checkout%2Finput-bug-v3 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1069/derrickstolee/sparse-checkout/input-bug-v3 Pull-Request: https://github.com/gitgitgadget/git/pull/1069 Range-diff vs v2: 1: a0e3dd335c9 ! 1: 1744a26845f sparse-checkout: fix segfault on malformed patterns @@ Commit message 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 - no invalid. + now invalid. The fix here is to stop trying to remove from the hashset. Better to leave bad data in the sparse-checkout matching logic (with a warning) than to 2: 86fbf130c03 = 2: a2fe867222e sparse-checkout: fix OOM error with mixed patterns 3: 5d096e380a4 = 3: a0e5a942ae0 sparse-checkout: refuse to add to bad patterns 4: 7bacb3760f3 < -: ----------- amend! sparse-checkout: fix segfault on malformed patterns -- gitgitgadget ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v3 1/3] sparse-checkout: fix segfault on malformed patterns 2021-12-15 13:46 ` [PATCH v3 0/3] sparse-checkout: fix segfault on malformed patterns Derrick Stolee via GitGitGadget @ 2021-12-15 13:46 ` Derrick Stolee via GitGitGadget 2021-12-15 20:56 ` Junio C Hamano 2021-12-15 13:46 ` [PATCH v3 2/3] sparse-checkout: fix OOM error with mixed patterns Derrick Stolee via GitGitGadget ` (3 subsequent siblings) 4 siblings, 1 reply; 26+ messages in thread From: Derrick Stolee via GitGitGadget @ 2021-12-15 13:46 UTC (permalink / raw) To: git; +Cc: me, newren, vdye, Derrick Stolee, Derrick Stolee, Derrick Stolee From: Derrick Stolee <dstolee@microsoft.com> 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. Better to leave bad data in the sparse-checkout matching logic (with a warning) than to segfault. If we are in this state, then we are already traversing into undefined behavior, so this change to keep the entry in the hashset is no worse than removing it. Add a test that triggers the segfault without the code change. Reported-by: John Burnett <johnburnett@johnburnett.com> Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Derrick Stolee <dstolee@microsoft.com> --- dir.c | 3 --- t/t1091-sparse-checkout-builtin.sh | 15 +++++++++++++++ 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/dir.c b/dir.c index 5aa6fbad0b7..0693c7cb3ee 100644 --- a/dir.c +++ b/dir.c @@ -819,9 +819,6 @@ 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); } return; diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh index 272ba1b566b..c72b8ee2e7b 100755 --- a/t/t1091-sparse-checkout-builtin.sh +++ b/t/t1091-sparse-checkout-builtin.sh @@ -708,4 +708,19 @@ 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 + cat repo/.git/info/sparse-checkout && + git -C repo sparse-checkout list +' + test_done -- gitgitgadget ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v3 1/3] sparse-checkout: fix segfault on malformed patterns 2021-12-15 13:46 ` [PATCH v3 1/3] " Derrick Stolee via GitGitGadget @ 2021-12-15 20:56 ` Junio C Hamano 2021-12-16 14:23 ` Derrick Stolee 0 siblings, 1 reply; 26+ messages in thread From: Junio C Hamano @ 2021-12-15 20:56 UTC (permalink / raw) To: Derrick Stolee via GitGitGadget Cc: git, me, newren, vdye, Derrick Stolee, Derrick Stolee, Derrick Stolee "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Derrick Stolee <dstolee@microsoft.com> > > 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. Makes sense. > 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. Understandable. > The fix here is to stop trying to remove from the hashset. Better to leave > bad data in the sparse-checkout matching logic (with a warning) than to > segfault. True, as long as it won't make the situation worse by depending on that bad data to further damage working tree data or in-repo data when damaged working tree data gets committed. And "list segfaults with freed/NULLed data---so leave the bad ones in to print these bad ones" feels OK-ish. As long as the user is not transporting the listed output to another repository, which may fall into "making the situation worse" category by spreading an existing breakage, that is. In other words, this may paper over the segfault, and it may be safe only for "sparse-checkout list", but is it safe for other operations that actually use this bad data to further affect other things in the repository? If not, I wonder if we want to hard die to lock the repository down before the issue is fixed to avoid spreading the damage? > diff --git a/dir.c b/dir.c > index 5aa6fbad0b7..0693c7cb3ee 100644 > --- a/dir.c > +++ b/dir.c > @@ -819,9 +819,6 @@ 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); > } > > return; > diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh > index 272ba1b566b..c72b8ee2e7b 100755 > --- a/t/t1091-sparse-checkout-builtin.sh > +++ b/t/t1091-sparse-checkout-builtin.sh > @@ -708,4 +708,19 @@ 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 > + cat repo/.git/info/sparse-checkout && Stray debugging output? > + git -C repo sparse-checkout list And we are happy as long as the command does not segfault, and we do not care what the output is. > +' > + > test_done Will queue, but not convinced yet. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 1/3] sparse-checkout: fix segfault on malformed patterns 2021-12-15 20:56 ` Junio C Hamano @ 2021-12-16 14:23 ` Derrick Stolee 0 siblings, 0 replies; 26+ messages in thread From: Derrick Stolee @ 2021-12-16 14:23 UTC (permalink / raw) To: Junio C Hamano, Derrick Stolee via GitGitGadget Cc: git, me, newren, vdye, Derrick Stolee, Derrick Stolee On 12/15/2021 3:56 PM, Junio C Hamano wrote: > "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> From: Derrick Stolee <dstolee@microsoft.com> >> >> 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. > > Makes sense. > >> 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. > > Understandable. > >> The fix here is to stop trying to remove from the hashset. Better to leave >> bad data in the sparse-checkout matching logic (with a warning) than to >> segfault. > > True, as long as it won't make the situation worse by depending on > that bad data to further damage working tree data or in-repo data > when damaged working tree data gets committed. And "list segfaults > with freed/NULLed data---so leave the bad ones in to print these bad > ones" feels OK-ish. > > As long as the user is not transporting the listed output to another > repository, which may fall into "making the situation worse" > category by spreading an existing breakage, that is. > > In other words, this may paper over the segfault, and it may be safe > only for "sparse-checkout list", but is it safe for other operations > that actually use this bad data to further affect other things in > the repository? If not, I wonder if we want to hard die to lock the > repository down before the issue is fixed to avoid spreading the > damage? You're right. I should do what we do elsewhere in this method and disable cone mode pattern matching, reverting to the old matching algorithms. This will clear the hashsets and avoid using them anywhere else. >> +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 >> + cat repo/.git/info/sparse-checkout && > > Stray debugging output? Thanks. >> + git -C repo sparse-checkout list > > And we are happy as long as the command does not segfault, and we do > not care what the output is. And I'll be able to test that the 'list' subcommand changes behavior when cone mode is disabled, in addition to checking stderr for the correct warnings. Thanks, -Stolee ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v3 2/3] sparse-checkout: fix OOM error with mixed patterns 2021-12-15 13:46 ` [PATCH v3 0/3] sparse-checkout: fix segfault on malformed patterns Derrick Stolee via GitGitGadget 2021-12-15 13:46 ` [PATCH v3 1/3] " Derrick Stolee via GitGitGadget @ 2021-12-15 13:46 ` Derrick Stolee via GitGitGadget 2021-12-15 13:46 ` [PATCH v3 3/3] sparse-checkout: refuse to add to bad patterns Derrick Stolee via GitGitGadget ` (2 subsequent siblings) 4 siblings, 0 replies; 26+ messages in thread From: Derrick Stolee via GitGitGadget @ 2021-12-15 13:46 UTC (permalink / raw) To: git; +Cc: me, newren, vdye, Derrick Stolee, Derrick Stolee, Derrick Stolee From: Derrick Stolee <dstolee@microsoft.com> 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 <calbabreaker@gmail.com> Helped-by: Taylor Blau <me@ttaylorr.com> Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Derrick Stolee <dstolee@microsoft.com> --- 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 d0f5c4702be..9ccdcde9832 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 c72b8ee2e7b..67ce24c9c83 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 && -- gitgitgadget ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v3 3/3] sparse-checkout: refuse to add to bad patterns 2021-12-15 13:46 ` [PATCH v3 0/3] sparse-checkout: fix segfault on malformed patterns Derrick Stolee via GitGitGadget 2021-12-15 13:46 ` [PATCH v3 1/3] " Derrick Stolee via GitGitGadget 2021-12-15 13:46 ` [PATCH v3 2/3] sparse-checkout: fix OOM error with mixed patterns Derrick Stolee via GitGitGadget @ 2021-12-15 13:46 ` Derrick Stolee via GitGitGadget 2021-12-15 20:43 ` [PATCH v3 0/3] sparse-checkout: fix segfault on malformed patterns Junio C Hamano 2021-12-16 16:13 ` [PATCH v4 " Derrick Stolee via GitGitGadget 4 siblings, 0 replies; 26+ messages in thread From: Derrick Stolee via GitGitGadget @ 2021-12-15 13:46 UTC (permalink / raw) To: git; +Cc: me, newren, vdye, Derrick Stolee, Derrick Stolee, Derrick Stolee From: Derrick Stolee <dstolee@microsoft.com> When in cone mode sparse-checkout, it is unclear how 'git sparse-checkout add <dir1> ...' 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 <newren@gmail.com> Signed-off-by: Derrick Stolee <dstolee@microsoft.com> --- 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 9ccdcde9832..6580075a631 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 0693c7cb3ee..a5dddafa16d 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 67ce24c9c83..2ed247d75a5 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 <add && git -C repo sparse-checkout list >actual && -- gitgitgadget ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v3 0/3] sparse-checkout: fix segfault on malformed patterns 2021-12-15 13:46 ` [PATCH v3 0/3] sparse-checkout: fix segfault on malformed patterns Derrick Stolee via GitGitGadget ` (2 preceding siblings ...) 2021-12-15 13:46 ` [PATCH v3 3/3] sparse-checkout: refuse to add to bad patterns Derrick Stolee via GitGitGadget @ 2021-12-15 20:43 ` Junio C Hamano 2021-12-16 14:24 ` Derrick Stolee 2021-12-16 16:13 ` [PATCH v4 " Derrick Stolee via GitGitGadget 4 siblings, 1 reply; 26+ messages in thread From: Junio C Hamano @ 2021-12-15 20:43 UTC (permalink / raw) To: Derrick Stolee via GitGitGadget Cc: git, me, newren, vdye, Derrick Stolee, Derrick Stolee "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > This series fixes some issues with parsing sparse-checkout patterns when > core.sparseCheckoutCone is enabled but the sparse-checkout file itself > contains patterns that don't match the cone mode format. > > The first patch fixes a segfault first reported in [1]. The other two > patches are from an earlier submission [2] that never got picked up and I > lost track of. There was another patch involving 'git sparse-checkout init > --cone' that isn't necessary, especially with Elijah doing some work in that > space right now. Thanks. The segfault fix matters even more with Elijah's "we can flip between modes" feature, right? > [1] https://github.com/git-for-windows/git/issues/3498 [2] > https://lore.kernel.org/git/pull.1043.git.1632160658.gitgitgadget@gmail.com > > Thanks, -Stolee > > > Updates in v2 and v3 > ==================== > > * I intended to fix a typo in a patch, but accidentally sent the amend! > commit in v2 > * v3 has the typo fix properly squashed in. > * Added Elijah's review. > > Derrick Stolee (3): > sparse-checkout: fix segfault on malformed patterns > sparse-checkout: fix OOM error with mixed patterns > sparse-checkout: refuse to add to bad patterns > > builtin/sparse-checkout.c | 5 ++++- > dir.c | 5 +---- > t/t1091-sparse-checkout-builtin.sh | 31 +++++++++++++++++++++++++++++- > 3 files changed, 35 insertions(+), 6 deletions(-) > > > base-commit: abe6bb3905392d5eb6b01fa6e54d7e784e0522aa > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1069%2Fderrickstolee%2Fsparse-checkout%2Finput-bug-v3 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1069/derrickstolee/sparse-checkout/input-bug-v3 > Pull-Request: https://github.com/gitgitgadget/git/pull/1069 > > Range-diff vs v2: > > 1: a0e3dd335c9 ! 1: 1744a26845f sparse-checkout: fix segfault on malformed patterns > @@ Commit message > 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 > - no invalid. > + now invalid. > > The fix here is to stop trying to remove from the hashset. Better to leave > bad data in the sparse-checkout matching logic (with a warning) than to > 2: 86fbf130c03 = 2: a2fe867222e sparse-checkout: fix OOM error with mixed patterns > 3: 5d096e380a4 = 3: a0e5a942ae0 sparse-checkout: refuse to add to bad patterns > 4: 7bacb3760f3 < -: ----------- amend! sparse-checkout: fix segfault on malformed patterns ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 0/3] sparse-checkout: fix segfault on malformed patterns 2021-12-15 20:43 ` [PATCH v3 0/3] sparse-checkout: fix segfault on malformed patterns Junio C Hamano @ 2021-12-16 14:24 ` Derrick Stolee 2021-12-16 19:16 ` Junio C Hamano 0 siblings, 1 reply; 26+ messages in thread From: Derrick Stolee @ 2021-12-16 14:24 UTC (permalink / raw) To: Junio C Hamano, Derrick Stolee via GitGitGadget Cc: git, me, newren, vdye, Derrick Stolee On 12/15/2021 3:43 PM, Junio C Hamano wrote: > "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> This series fixes some issues with parsing sparse-checkout patterns when >> core.sparseCheckoutCone is enabled but the sparse-checkout file itself >> contains patterns that don't match the cone mode format. >> >> The first patch fixes a segfault first reported in [1]. The other two >> patches are from an earlier submission [2] that never got picked up and I >> lost track of. There was another patch involving 'git sparse-checkout init >> --cone' that isn't necessary, especially with Elijah doing some work in that >> space right now. > > Thanks. The segfault fix matters even more with Elijah's "we can > flip between modes" feature, right? Generally, I think segfaults are a higher priority, but this one is hard to find in the wild, I think. I'll send a v4 today and hopefully it makes it an easy decision for you. Thanks, -Stolee ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 0/3] sparse-checkout: fix segfault on malformed patterns 2021-12-16 14:24 ` Derrick Stolee @ 2021-12-16 19:16 ` Junio C Hamano 0 siblings, 0 replies; 26+ messages in thread From: Junio C Hamano @ 2021-12-16 19:16 UTC (permalink / raw) To: Derrick Stolee Cc: Derrick Stolee via GitGitGadget, git, me, newren, vdye, Derrick Stolee Derrick Stolee <stolee@gmail.com> writes: >> Thanks. The segfault fix matters even more with Elijah's "we can >> flip between modes" feature, right? > > Generally, I think segfaults are a higher priority, but this one is hard > to find in the wild, I think. I'll send a v4 today and hopefully it makes > it an easy decision for you. I agree about the priority and I was assuming that the topics do not have semantic conflicts we cannot reconcile, but I do not offhand recall what the other topic did when there patterns unsuitable in the cone mode when transitioning to the cone mode (or vice-versa for that matter). ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v4 0/3] sparse-checkout: fix segfault on malformed patterns 2021-12-15 13:46 ` [PATCH v3 0/3] sparse-checkout: fix segfault on malformed patterns Derrick Stolee via GitGitGadget ` (3 preceding siblings ...) 2021-12-15 20:43 ` [PATCH v3 0/3] sparse-checkout: fix segfault on malformed patterns Junio C Hamano @ 2021-12-16 16:13 ` Derrick Stolee via GitGitGadget 2021-12-16 16:13 ` [PATCH v4 1/3] " Derrick Stolee via GitGitGadget ` (2 more replies) 4 siblings, 3 replies; 26+ messages in thread From: Derrick Stolee via GitGitGadget @ 2021-12-16 16:13 UTC (permalink / raw) To: git; +Cc: me, newren, vdye, Derrick Stolee, Derrick Stolee This series fixes some issues with parsing sparse-checkout patterns when core.sparseCheckoutCone is enabled but the sparse-checkout file itself contains patterns that don't match the cone mode format. The first patch fixes a segfault first reported in [1]. The other two patches are from an earlier submission [2] that never got picked up and I lost track of. There was another patch involving 'git sparse-checkout init --cone' that isn't necessary, especially with Elijah doing some work in that space right now. [1] https://github.com/git-for-windows/git/issues/3498 [2] https://lore.kernel.org/git/pull.1043.git.1632160658.gitgitgadget@gmail.com Thanks, -Stolee Update in v4 ============ * For added precaution, this kind of unexpected duplicate pattern will disable cone mode matching. * Tests are updated to verify this new behavior. Updates in v2 and v3 ==================== * I intended to fix a typo in a patch, but accidentally sent the amend! commit in v2 * v3 has the typo fix properly squashed in. * Added Elijah's review. Derrick Stolee (3): sparse-checkout: fix segfault on malformed patterns sparse-checkout: fix OOM error with mixed patterns sparse-checkout: refuse to add to bad patterns builtin/sparse-checkout.c | 5 +++- dir.c | 6 ++--- t/t1091-sparse-checkout-builtin.sh | 37 +++++++++++++++++++++++++++++- 3 files changed, 42 insertions(+), 6 deletions(-) base-commit: abe6bb3905392d5eb6b01fa6e54d7e784e0522aa Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1069%2Fderrickstolee%2Fsparse-checkout%2Finput-bug-v4 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1069/derrickstolee/sparse-checkout/input-bug-v4 Pull-Request: https://github.com/gitgitgadget/git/pull/1069 Range-diff vs v3: 1: 1744a26845f ! 1: 5353c541d9f sparse-checkout: fix segfault on malformed patterns @@ Commit message 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. Better to leave - bad data in the sparse-checkout matching logic (with a warning) than to - segfault. If we are in this state, then we are already traversing into - undefined behavior, so this change to keep the entry in the hashset is no - worse than removing it. + 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. @@ dir.c: static void add_pattern_to_hashsets(struct pattern_list *pl, struct path_ - hashmap_remove(&pl->parent_hashmap, &translated->ent, &data); - free(data); - free(translated); ++ goto clear_hashmaps; } return; @@ t/t1091-sparse-checkout-builtin.sh: test_expect_success 'cone mode clears ignore + !/foo/*/ + /foo/\*/ + EOF -+ cat repo/.git/info/sparse-checkout && -+ git -C repo sparse-checkout list ++ ++ # 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 2: a2fe867222e = 2: 3fd625290a3 sparse-checkout: fix OOM error with mixed patterns 3: a0e5a942ae0 = 3: f5f7b8b8e04 sparse-checkout: refuse to add to bad patterns -- gitgitgadget ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v4 1/3] sparse-checkout: fix segfault on malformed patterns 2021-12-16 16:13 ` [PATCH v4 " Derrick Stolee via GitGitGadget @ 2021-12-16 16:13 ` Derrick Stolee via GitGitGadget 2021-12-16 16:13 ` [PATCH v4 2/3] sparse-checkout: fix OOM error with mixed patterns Derrick Stolee via GitGitGadget 2021-12-16 16:13 ` [PATCH v4 3/3] sparse-checkout: refuse to add to bad patterns Derrick Stolee via GitGitGadget 2 siblings, 0 replies; 26+ messages in thread From: Derrick Stolee via GitGitGadget @ 2021-12-16 16:13 UTC (permalink / raw) To: git; +Cc: me, newren, vdye, Derrick Stolee, Derrick Stolee, Derrick Stolee From: Derrick Stolee <dstolee@microsoft.com> 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 <johnburnett@johnburnett.com> Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Derrick Stolee <dstolee@microsoft.com> --- 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 5aa6fbad0b7..7e72958d51d 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 272ba1b566b..3921ea80138 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 -- gitgitgadget ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 2/3] sparse-checkout: fix OOM error with mixed patterns 2021-12-16 16:13 ` [PATCH v4 " Derrick Stolee via GitGitGadget 2021-12-16 16:13 ` [PATCH v4 1/3] " Derrick Stolee via GitGitGadget @ 2021-12-16 16:13 ` Derrick Stolee via GitGitGadget 2021-12-16 16:13 ` [PATCH v4 3/3] sparse-checkout: refuse to add to bad patterns Derrick Stolee via GitGitGadget 2 siblings, 0 replies; 26+ messages in thread From: Derrick Stolee via GitGitGadget @ 2021-12-16 16:13 UTC (permalink / raw) To: git; +Cc: me, newren, vdye, Derrick Stolee, Derrick Stolee, Derrick Stolee From: Derrick Stolee <dstolee@microsoft.com> 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 <calbabreaker@gmail.com> Helped-by: Taylor Blau <me@ttaylorr.com> Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Derrick Stolee <dstolee@microsoft.com> --- 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 d0f5c4702be..9ccdcde9832 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 3921ea80138..1f877ced0c8 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 && -- gitgitgadget ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 3/3] sparse-checkout: refuse to add to bad patterns 2021-12-16 16:13 ` [PATCH v4 " Derrick Stolee via GitGitGadget 2021-12-16 16:13 ` [PATCH v4 1/3] " Derrick Stolee via GitGitGadget 2021-12-16 16:13 ` [PATCH v4 2/3] sparse-checkout: fix OOM error with mixed patterns Derrick Stolee via GitGitGadget @ 2021-12-16 16:13 ` Derrick Stolee via GitGitGadget 2 siblings, 0 replies; 26+ messages in thread From: Derrick Stolee via GitGitGadget @ 2021-12-16 16:13 UTC (permalink / raw) To: git; +Cc: me, newren, vdye, Derrick Stolee, Derrick Stolee, Derrick Stolee From: Derrick Stolee <dstolee@microsoft.com> When in cone mode sparse-checkout, it is unclear how 'git sparse-checkout add <dir1> ...' 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 <newren@gmail.com> Signed-off-by: Derrick Stolee <dstolee@microsoft.com> --- 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 9ccdcde9832..6580075a631 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 7e72958d51d..eca75e89213 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 1f877ced0c8..34b97fd3ee0 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 <add && git -C repo sparse-checkout list >actual && -- gitgitgadget ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 0/3] Sparse checkout: fix mixed-mode pattern issues @ 2021-09-20 17:57 Derrick Stolee via GitGitGadget 2021-09-20 17:57 ` [PATCH 3/3] sparse-checkout: refuse to add to bad patterns Derrick Stolee via GitGitGadget 0 siblings, 1 reply; 26+ messages in thread From: Derrick Stolee via GitGitGadget @ 2021-09-20 17:57 UTC (permalink / raw) To: git; +Cc: gitster, me, calbabreaker, Derrick Stolee This fixes a memory leak reported in [1], but also fixes some behavior concerns around the sparse-checkout command when the sparse-checkout file does not use cone mode patterns, but cone mode is enabled. [1] https://lore.kernel.org/git/CAKRwm5a9PyqffEC5N__urSpNcZ-d5vz9GBM2Ei16eGS25B=-FQ@mail.gmail.com/ 1. The first patch fixes the OOM as recommended by Taylor. 2. The second patch changes the behavior of 'git sparse-checkout init --cone' to overwrite the sparse-checkout file if the patterns are not in cone mode. 3. The third patch causes 'git sparse-checkout add' to fail if cone mode is enabled but the existing patterns are not in cone mode. This also requires strengthening our pattern filtering to require the first character be a slash ('/'), which should have been there from the start. Thanks, -Stolee Derrick Stolee (3): sparse-checkout: fix OOM error with mixed patterns sparse-checkout: clear patterns when switching modes sparse-checkout: refuse to add to bad patterns builtin/sparse-checkout.c | 18 +++++++++++++++--- dir.c | 2 +- t/t1091-sparse-checkout-builtin.sh | 22 ++++++++++++++++++++-- 3 files changed, 36 insertions(+), 6 deletions(-) base-commit: 4c719308ce59dc70e606f910f40801f2c6051b24 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1043%2Fderrickstolee%2Fsparse-checkout-fix-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1043/derrickstolee/sparse-checkout-fix-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/1043 -- gitgitgadget ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 3/3] sparse-checkout: refuse to add to bad patterns 2021-09-20 17:57 [PATCH 0/3] Sparse checkout: fix mixed-mode pattern issues Derrick Stolee via GitGitGadget @ 2021-09-20 17:57 ` Derrick Stolee via GitGitGadget 2021-09-20 18:59 ` Taylor Blau 0 siblings, 1 reply; 26+ messages in thread From: Derrick Stolee via GitGitGadget @ 2021-09-20 17:57 UTC (permalink / raw) To: git; +Cc: gitster, me, calbabreaker, Derrick Stolee, Derrick Stolee From: Derrick Stolee <dstolee@microsoft.com> When in cone mode sparse-checkout, it is unclear how 'git sparse-checkout add <dir1> ...' 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. Signed-off-by: Derrick Stolee <dstolee@microsoft.com> --- builtin/sparse-checkout.c | 3 +++ dir.c | 2 +- t/t1091-sparse-checkout-builtin.sh | 11 +++++++---- 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c index fe76c3eedda..2492ae828a9 100644 --- a/builtin/sparse-checkout.c +++ b/builtin/sparse-checkout.c @@ -513,6 +513,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 03c4d212672..93136442103 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 af0acd32bd9..780c6a1aaae 100755 --- a/t/t1091-sparse-checkout-builtin.sh +++ b/t/t1091-sparse-checkout-builtin.sh @@ -108,14 +108,17 @@ test_expect_success 'switching to cone mode with non-cone mode patterns' ' git -C bad-patterns sparse-checkout init && git -C bad-patterns sparse-checkout add dir && git -C bad-patterns config core.sparseCheckoutCone true && - git -C bad-patterns sparse-checkout add dir && + + test_must_fail git -C bad-patterns sparse-checkout add dir 2>err && + grep "existing sparse-checkout patterns do not use cone mode" err && git -C bad-patterns sparse-checkout init --cone && cat >expect <<-\EOF && /* !/*/ EOF - test_cmp expect bad-patterns/.git/info/sparse-checkout + test_cmp expect bad-patterns/.git/info/sparse-checkout && + git -C bad-patterns sparse-checkout add dir ' test_expect_success 'interaction with clone --no-checkout (unborn index)' ' @@ -182,9 +185,9 @@ test_expect_success 'set sparse-checkout using --stdin' ' test_expect_success 'add to sparse-checkout' ' cat repo/.git/info/sparse-checkout >expect && cat >add <<-\EOF && - pattern1 + /pattern1 /folder1/ - pattern2 + /pattern2 EOF cat add >>expect && git -C repo sparse-checkout add --stdin <add && -- gitgitgadget ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 3/3] sparse-checkout: refuse to add to bad patterns 2021-09-20 17:57 ` [PATCH 3/3] sparse-checkout: refuse to add to bad patterns Derrick Stolee via GitGitGadget @ 2021-09-20 18:59 ` Taylor Blau 0 siblings, 0 replies; 26+ messages in thread From: Taylor Blau @ 2021-09-20 18:59 UTC (permalink / raw) To: Derrick Stolee via GitGitGadget Cc: git, gitster, me, calbabreaker, Derrick Stolee, Derrick Stolee On Mon, Sep 20, 2021 at 05:57:38PM +0000, Derrick Stolee via GitGitGadget wrote: > From: Derrick Stolee <dstolee@microsoft.com> > > When in cone mode sparse-checkout, it is unclear how 'git > sparse-checkout add <dir1> ...' 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. > > Signed-off-by: Derrick Stolee <dstolee@microsoft.com> > --- > builtin/sparse-checkout.c | 3 +++ > dir.c | 2 +- > t/t1091-sparse-checkout-builtin.sh | 11 +++++++---- > 3 files changed, 11 insertions(+), 5 deletions(-) > > diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c > index fe76c3eedda..2492ae828a9 100644 > --- a/builtin/sparse-checkout.c > +++ b/builtin/sparse-checkout.c > @@ -513,6 +513,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")); > + Makes sense. > 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 03c4d212672..93136442103 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 != '/' || Makes sense that we require cone-mode patterns to start with '/', which necessarily means we want to drop the other arm `*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 af0acd32bd9..780c6a1aaae 100755 > --- a/t/t1091-sparse-checkout-builtin.sh > +++ b/t/t1091-sparse-checkout-builtin.sh > @@ -108,14 +108,17 @@ test_expect_success 'switching to cone mode with non-cone mode patterns' ' > git -C bad-patterns sparse-checkout init && > git -C bad-patterns sparse-checkout add dir && > git -C bad-patterns config core.sparseCheckoutCone true && > - git -C bad-patterns sparse-checkout add dir && > + > + test_must_fail git -C bad-patterns sparse-checkout add dir 2>err && > + grep "existing sparse-checkout patterns do not use cone mode" err && > > git -C bad-patterns sparse-checkout init --cone && > cat >expect <<-\EOF && > /* > !/*/ > EOF > - test_cmp expect bad-patterns/.git/info/sparse-checkout > + test_cmp expect bad-patterns/.git/info/sparse-checkout && > + git -C bad-patterns sparse-checkout add dir Good thinking to make sure that we can add `dir` after clearing. But let's check the contents of the sparse-checkout file to make sure that it was added in cone mode. > ' > > test_expect_success 'interaction with clone --no-checkout (unborn index)' ' > @@ -182,9 +185,9 @@ test_expect_success 'set sparse-checkout using --stdin' ' > test_expect_success 'add to sparse-checkout' ' > cat repo/.git/info/sparse-checkout >expect && > cat >add <<-\EOF && > - pattern1 > + /pattern1 > /folder1/ > - pattern2 > + /pattern2 > EOF > cat add >>expect && > git -C repo sparse-checkout add --stdin <add && Hmm. Does this new restriction apply to patterns given over the command-line? (It looks like we handle the two slightly differently, so I wonder if we are expecting users to write "git sparse-checkout add /foo" instead of "... add foo" as a consequence). Thanks, Taylor ^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2021-12-16 19:16 UTC | newest] Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-12-07 20:02 [PATCH 0/3] sparse-checkout: fix segfault on malformed patterns Derrick Stolee via GitGitGadget 2021-12-07 20:02 ` [PATCH 1/3] " Derrick Stolee via GitGitGadget 2021-12-07 20:22 ` Elijah Newren 2021-12-07 20:02 ` [PATCH 2/3] sparse-checkout: fix OOM error with mixed patterns Derrick Stolee via GitGitGadget 2021-12-07 20:02 ` [PATCH 3/3] sparse-checkout: refuse to add to bad patterns Derrick Stolee via GitGitGadget 2021-12-07 21:51 ` [PATCH 0/3] sparse-checkout: fix segfault on malformed patterns Elijah Newren 2021-12-08 14:23 ` Derrick Stolee 2021-12-10 15:18 ` [PATCH v2 0/4] " Derrick Stolee via GitGitGadget 2021-12-10 15:18 ` [PATCH v2 1/4] " Derrick Stolee via GitGitGadget 2021-12-10 15:18 ` [PATCH v2 2/4] sparse-checkout: fix OOM error with mixed patterns Derrick Stolee via GitGitGadget 2021-12-10 15:18 ` [PATCH v2 3/4] sparse-checkout: refuse to add to bad patterns Derrick Stolee via GitGitGadget 2021-12-15 13:46 ` [PATCH v3 0/3] sparse-checkout: fix segfault on malformed patterns Derrick Stolee via GitGitGadget 2021-12-15 13:46 ` [PATCH v3 1/3] " Derrick Stolee via GitGitGadget 2021-12-15 20:56 ` Junio C Hamano 2021-12-16 14:23 ` Derrick Stolee 2021-12-15 13:46 ` [PATCH v3 2/3] sparse-checkout: fix OOM error with mixed patterns Derrick Stolee via GitGitGadget 2021-12-15 13:46 ` [PATCH v3 3/3] sparse-checkout: refuse to add to bad patterns Derrick Stolee via GitGitGadget 2021-12-15 20:43 ` [PATCH v3 0/3] sparse-checkout: fix segfault on malformed patterns Junio C Hamano 2021-12-16 14:24 ` Derrick Stolee 2021-12-16 19:16 ` Junio C Hamano 2021-12-16 16:13 ` [PATCH v4 " Derrick Stolee via GitGitGadget 2021-12-16 16:13 ` [PATCH v4 1/3] " Derrick Stolee via GitGitGadget 2021-12-16 16:13 ` [PATCH v4 2/3] sparse-checkout: fix OOM error with mixed patterns Derrick Stolee via GitGitGadget 2021-12-16 16:13 ` [PATCH v4 3/3] sparse-checkout: refuse to add to bad patterns Derrick Stolee via GitGitGadget -- strict thread matches above, loose matches on Subject: below -- 2021-09-20 17:57 [PATCH 0/3] Sparse checkout: fix mixed-mode pattern issues Derrick Stolee via GitGitGadget 2021-09-20 17:57 ` [PATCH 3/3] sparse-checkout: refuse to add to bad patterns Derrick Stolee via GitGitGadget 2021-09-20 18:59 ` Taylor Blau
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.