Re: [PATCH 1/3] sparse-checkout: fix OOM error with mixed patterns
From: Taylor Blau <hidden>
Date: 2021-09-21 02:22:00
On Mon, Sep 20, 2021 at 05:57:36PM +0000, Derrick Stolee via GitGitGadget wrote:
quoted hunk ↗ jump to hunk
From: Derrick Stolee <redacted> 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 <redacted> Helped-by: Taylor Blau [off-list ref] Signed-off-by: Derrick Stolee <redacted> --- builtin/sparse-checkout.c | 2 +- t/t1091-sparse-checkout-builtin.sh | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-)diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c index 8ba9f13787b..b45fd97a98b 100644 --- a/builtin/sparse-checkout.c +++ b/builtin/sparse-checkout.c@@ -389,7 +389,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;
If we are preparing to make it so that we do not blindly copy patterns from a sparse checkout without cone mode enabled, then wouldn't this new case be a BUG()? Of course, users could still tweak the contents of their sparse-checkout file as they wish, but I'd expect that we'd catch those cases, too (i.e., by validating the contents of the existing sparse-checkout before calling this function).
quoted hunk ↗ jump to hunk
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh index 38fc8340f5c..a429d2cc671 100755 --- a/t/t1091-sparse-checkout-builtin.sh +++ b/t/t1091-sparse-checkout-builtin.sh@@ -103,6 +103,14 @@ 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 && + git -C bad-patterns sparse-checkout init && + git -C bad-patterns sparse-checkout add dir && + git -C bad-patterns config core.sparseCheckoutCone true &&
Makes sense that we'd want to update the config rather than call "init --cone" here, since a subsequent patch would change the thing that this is testing (from "doesn't OOM in the above-described situation" to "clears the existing contents of your sparse-checkout").
+ git -C bad-patterns sparse-checkout add dir +' +
In other series I've suggested a cosmetic change to move all of these to a sub-shell that begins with "cd bad-patterns &&", but obviously that is a relatively unimportant suggestion. Thanks, Taylor