threads / patch / 61347

patch, 4 partsmark t3701-add-interactive.sh as leak-free

Subject: [PATCH 0/4] mark t3701-add-interactive.sh as leak-free

## tl;dr

19 messages between Apr 21, 2024 and Apr 22, 2024. Diffs are folded; open one to read it.

replies: 18people: 4as markdown or json

Rubén Justo· Apr 21, 2024, 10:22 UTC · lore
Rubén Justo (4):
  apply: plug a leak in apply_data
  add-interactive: plug a leak in get_untracked_files
  add-patch: plug a leak handling the '/' command
  add: plug a leak on interactive_add
 add-interactive.c          | 1 +
 add-patch.c                | 1 +
 apply.c                    | 4 +++-
 builtin/add.c              | 9 ++++++---
 t/t2016-checkout-patch.sh  | 1 +
 t/t3701-add-interactive.sh | 1 +
 t/t4103-apply-binary.sh    | 1 +
 t/t4104-apply-boundary.sh  | 1 +
 t/t4113-apply-ending.sh    | 1 +
 t/t4117-apply-reject.sh    | 1 +
 t/t4123-apply-shrink.sh    | 1 +
 t/t4252-am-options.sh      | 2 ++
 t/t4258-am-quoted-cr.sh    | 1 +
 t/t7514-commit-patch.sh    | 2 ++
 14 files changed, 23 insertions(+), 4 deletions(-)
-- 
2.45.0.rc0.4.g08f77eb516
Rubén Justo· Apr 21, 2024, 10:27 UTC · re: Rubén Justo · lore

[PATCH 2/4] add-interactive: plug a leak in get_untracked_files

Plug a leak we have since ab1e1cccaf (built-in add -i: re-implement `add-untracked` in C, 2019-11-29).

This leak can be triggered with:
	$ echo a | git add -i

As a curiosity, we have a somewhat similar function in builtin/stash.c, which correctly frees the memory.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 add-interactive.c | 1 +
 1 file changed, 1 insertion(+)
Show changes to add-interactive.c +1 −0
diff --git a/add-interactive.c b/add-interactive.c
index 6bf87e7ae7..e17602b5e4 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -865,6 +865,7 @@ static int get_untracked_files(struct repository *r,
 	}
 
 	strbuf_release(&buf);
+	dir_clear(&dir);
 	return 0;
 }
 

-- 
2.45.0.rc0.4.g08f77eb516
Junio C Hamano· Apr 22, 2024, 15:50 UTC · re: Rubén Justo · lore

Re: [PATCH 2/4] add-interactive: plug a leak in get_untracked_files

Rubén Justo <rjusto@gmail.com> writes:
Show 9 quoted lines
> Plug a leak we have since ab1e1cccaf (built-in add -i: re-implement
> `add-untracked` in C, 2019-11-29).
>
> This leak can be triggered with:
>
> 	$ echo a | git add -i
>
> As a curiosity, we have a somewhat similar function in builtin/stash.c,
> which correctly frees the memory.
Yup, 
    $ git grep -W -e fill_directory -e dir_clear -e setup_standard_excludes \*.c
makes an interesting read ;-)
The fix looks good.  Thanks.
Show 16 quoted lines
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>  add-interactive.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/add-interactive.c b/add-interactive.c
> index 6bf87e7ae7..e17602b5e4 100644
> --- a/add-interactive.c
> +++ b/add-interactive.c
> @@ -865,6 +865,7 @@ static int get_untracked_files(struct repository *r,
>  	}
>  
>  	strbuf_release(&buf);
> +	dir_clear(&dir);
>  	return 0;
>  }
Rubén Justo· Apr 21, 2024, 10:28 UTC · re: Rubén Justo · lore

[PATCH 1/4] apply: plug a leak in apply_data

Plug a leak we have since cfb6f9acc3 (apply: accept -3/--3way command line option, 2012-05-08).

This leak can be triggered with:
    $ echo foo >file
    $ git add file && git commit -m file
    $ echo bar >file
    $ git diff file >diff
    $ sed s/foo/frotz/ <diff >baddiff
    $ git apply --cached <baddiff
Fixing this leak allows us to mark as leak-free the following tests:
    + t2016-checkout-patch.sh
    + t4103-apply-binary.sh
    + t4104-apply-boundary.sh
    + t4113-apply-ending.sh
    + t4117-apply-reject.sh
    + t4123-apply-shrink.sh
    + t4252-am-options.sh
    + t4258-am-quoted-cr.sh

Mark them with "TEST_PASSES_SANITIZE_LEAK=true" to notice and fix promply any new leak that may be introduced and triggered by them in the future.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 apply.c                   | 4 +++-
 t/t2016-checkout-patch.sh | 1 +
 t/t4103-apply-binary.sh   | 1 +
 t/t4104-apply-boundary.sh | 1 +
 t/t4113-apply-ending.sh   | 1 +
 t/t4117-apply-reject.sh   | 1 +
 t/t4123-apply-shrink.sh   | 1 +
 t/t4252-am-options.sh     | 2 ++
 t/t4258-am-quoted-cr.sh   | 1 +
 9 files changed, 12 insertions(+), 1 deletion(-)
Show changes to 9 files +12 −1

apply.c, t/t2016-checkout-patch.sh, t/t4103-apply-binary.sh, t/t4104-apply-boundary.sh, t/t4113-apply-ending.sh, t/t4117-apply-reject.sh, t/t4123-apply-shrink.sh, t/t4252-am-options.sh, t/t4258-am-quoted-cr.sh

diff --git a/apply.c b/apply.c
index 34f20326a7..2f752d71a8 100644
--- a/apply.c
+++ b/apply.c
@@ -3712,8 +3712,10 @@ static int apply_data(struct apply_state *state, struct patch *patch,
 			fprintf(stderr, _("Falling back to direct application...\n"));
 
 		/* Note: with --reject, apply_fragments() returns 0 */
-		if (patch->direct_to_threeway || apply_fragments(state, &image, patch) < 0)
+		if (patch->direct_to_threeway || apply_fragments(state, &image, patch) < 0) {
+			clear_image(&image);
 			return -1;
+		}
 	}
 	patch->result = image.buf;
 	patch->resultsize = image.len;
diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh
index c4f9bf09aa..c40b661ac1 100755
--- a/t/t2016-checkout-patch.sh
+++ b/t/t2016-checkout-patch.sh
@@ -2,6 +2,7 @@
 
 test_description='git checkout --patch'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./lib-patch-mode.sh
 
 test_expect_success 'setup' '
diff --git a/t/t4103-apply-binary.sh b/t/t4103-apply-binary.sh
index d370ecfe0d..144619ab87 100755
--- a/t/t4103-apply-binary.sh
+++ b/t/t4103-apply-binary.sh
@@ -9,6 +9,7 @@ test_description='git apply handling binary patches
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t4104-apply-boundary.sh b/t/t4104-apply-boundary.sh
index 71ef4132d1..dc501aac38 100755
--- a/t/t4104-apply-boundary.sh
+++ b/t/t4104-apply-boundary.sh
@@ -5,6 +5,7 @@
 
 test_description='git apply boundary tests'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 L="c d e f g h i j k l m n o p q r s t u v w x"
diff --git a/t/t4113-apply-ending.sh b/t/t4113-apply-ending.sh
index 66fa51591e..2c65c6a169 100755
--- a/t/t4113-apply-ending.sh
+++ b/t/t4113-apply-ending.sh
@@ -6,6 +6,7 @@
 test_description='git apply trying to add an ending line.
 
 '
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # setup
diff --git a/t/t4117-apply-reject.sh b/t/t4117-apply-reject.sh
index c86d05a96f..4d15ccd28e 100755
--- a/t/t4117-apply-reject.sh
+++ b/t/t4117-apply-reject.sh
@@ -7,6 +7,7 @@ test_description='git apply with rejects
 
 '
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '
diff --git a/t/t4123-apply-shrink.sh b/t/t4123-apply-shrink.sh
index 3ef84619f5..3601c0c5dc 100755
--- a/t/t4123-apply-shrink.sh
+++ b/t/t4123-apply-shrink.sh
@@ -2,6 +2,7 @@
 
 test_description='apply a patch that is larger than the preimage'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 cat >F  <<\EOF
diff --git a/t/t4252-am-options.sh b/t/t4252-am-options.sh
index e758e634a3..5b680dc755 100755
--- a/t/t4252-am-options.sh
+++ b/t/t4252-am-options.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='git am with options and not losing them'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 tm="$TEST_DIRECTORY/t4252"
diff --git a/t/t4258-am-quoted-cr.sh b/t/t4258-am-quoted-cr.sh
index 201915b45a..3573c9147f 100755
--- a/t/t4258-am-quoted-cr.sh
+++ b/t/t4258-am-quoted-cr.sh
@@ -2,6 +2,7 @@
 
 test_description='test am --quoted-cr=<action>'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 DATA="$TEST_DIRECTORY/t4258"

-- 
2.45.0.rc0.4.g08f77eb516
Phillip Wood· Apr 22, 2024, 15:41 UTC · re: Rubén Justo · lore

Re: [PATCH 1/4] apply: plug a leak in apply_data

Hi Rubén
On 21/04/2024 11:28, Rubén Justo wrote:
> Plug a leak we have since cfb6f9acc3 (apply: accept -3/--3way command
> line option, 2012-05-08).
Looking at that commit I see
-        if (apply_fragments(&image, patch) < 0)
-                return -1; /* note with --reject this succeeds. */
+        if (apply_fragments(&image, patch) < 0) {
+                /* Note: with --reject, apply_fragments() returns 0 */
+                if (!threeway || try_threeway(&image, patch, st, ce) < 0)
+                        return -1;
+        }

So the leak existed before that commit. Indeed it looks like the leak predates the introduction of struct image in b94f2eda99 (builtin-apply.c: make it more line oriented, 2008-01-26) and when the patch does not apply we have been leaking the buffer passed to apply_fragments() since the beginning of the builtin apply added in ac6245e31a3 (Builtin git-apply., 2006-05-23)

The fix itself looks good to me
Best Wishes
Phillip
Show 150 quoted lines
> This leak can be triggered with:
> 
>      $ echo foo >file
>      $ git add file && git commit -m file
>      $ echo bar >file
>      $ git diff file >diff
>      $ sed s/foo/frotz/ <diff >baddiff
>      $ git apply --cached <baddiffén
> 
> Fixing this leak allows us to mark as leak-free the following tests:
> 
>      + t2016-checkout-patch.sh
>      + t4103-apply-binary.sh
>      + t4104-apply-boundary.sh
>      + t4113-apply-ending.sh
>      + t4117-apply-reject.sh
>      + t4123-apply-shrink.sh
>      + t4252-am-options.sh
>      + t4258-am-quoted-cr.sh
> 
> Mark them with "TEST_PASSES_SANITIZE_LEAK=true" to notice and fix
> promply any new leak that may be introduced and triggered by them in the
> future.
> 
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>   apply.c                   | 4 +++-
>   t/t2016-checkout-patch.sh | 1 +
>   t/t4103-apply-binary.sh   | 1 +
>   t/t4104-apply-boundary.sh | 1 +
>   t/t4113-apply-ending.sh   | 1 +
>   t/t4117-apply-reject.sh   | 1 +
>   t/t4123-apply-shrink.sh   | 1 +
>   t/t4252-am-options.sh     | 2 ++
>   t/t4258-am-quoted-cr.sh   | 1 +
>   9 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/apply.c b/apply.c
> index 34f20326a7..2f752d71a8 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -3712,8 +3712,10 @@ static int apply_data(struct apply_state *state, struct patch *patch,
>   			fprintf(stderr, _("Falling back to direct application...\n"));
>   
>   		/* Note: with --reject, apply_fragments() returns 0 */
> -		if (patch->direct_to_threeway || apply_fragments(state, &image, patch) < 0)
> +		if (patch->direct_to_threeway || apply_fragments(state, &image, patch) < 0) {
> +			clear_image(&image);
>   			return -1;
> +		}
>   	}
>   	patch->result = image.buf;
>   	patch->resultsize = image.len;
> diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh
> index c4f9bf09aa..c40b661ac1 100755
> --- a/t/t2016-checkout-patch.sh
> +++ b/t/t2016-checkout-patch.sh
> @@ -2,6 +2,7 @@
>   
>   test_description='git checkout --patch'
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./lib-patch-mode.sh
>   
>   test_expect_success 'setup' '
> diff --git a/t/t4103-apply-binary.sh b/t/t4103-apply-binary.sh
> index d370ecfe0d..144619ab87 100755
> --- a/t/t4103-apply-binary.sh
> +++ b/t/t4103-apply-binary.sh
> @@ -9,6 +9,7 @@ test_description='git apply handling binary patches
>   GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>   export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   
>   test_expect_success 'setup' '
> diff --git a/t/t4104-apply-boundary.sh b/t/t4104-apply-boundary.sh
> index 71ef4132d1..dc501aac38 100755
> --- a/t/t4104-apply-boundary.sh
> +++ b/t/t4104-apply-boundary.sh
> @@ -5,6 +5,7 @@
>   
>   test_description='git apply boundary tests'
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   
>   L="c d e f g h i j k l m n o p q r s t u v w x"
> diff --git a/t/t4113-apply-ending.sh b/t/t4113-apply-ending.sh
> index 66fa51591e..2c65c6a169 100755
> --- a/t/t4113-apply-ending.sh
> +++ b/t/t4113-apply-ending.sh
> @@ -6,6 +6,7 @@
>   test_description='git apply trying to add an ending line.
>   
>   '
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   
>   # setup
> diff --git a/t/t4117-apply-reject.sh b/t/t4117-apply-reject.sh
> index c86d05a96f..4d15ccd28e 100755
> --- a/t/t4117-apply-reject.sh
> +++ b/t/t4117-apply-reject.sh
> @@ -7,6 +7,7 @@ test_description='git apply with rejects
>   
>   '
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   
>   test_expect_success setup '
> diff --git a/t/t4123-apply-shrink.sh b/t/t4123-apply-shrink.sh
> index 3ef84619f5..3601c0c5dc 100755
> --- a/t/t4123-apply-shrink.sh
> +++ b/t/t4123-apply-shrink.sh
> @@ -2,6 +2,7 @@
>   
>   test_description='apply a patch that is larger than the preimage'
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   
>   cat >F  <<\EOF
> diff --git a/t/t4252-am-options.sh b/t/t4252-am-options.sh
> index e758e634a3..5b680dc755 100755
> --- a/t/t4252-am-options.sh
> +++ b/t/t4252-am-options.sh
> @@ -1,6 +1,8 @@
>   #!/bin/sh
>   
>   test_description='git am with options and not losing them'
> +
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   
>   tm="$TEST_DIRECTORY/t4252"
> diff --git a/t/t4258-am-quoted-cr.sh b/t/t4258-am-quoted-cr.sh
> index 201915b45a..3573c9147f 100755
> --- a/t/t4258-am-quoted-cr.sh
> +++ b/t/t4258-am-quoted-cr.sh
> @@ -2,6 +2,7 @@
>   
>   test_description='test am --quoted-cr=<action>'
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   
>   DATA="$TEST_DIRECTORY/t4258"
Rubén Justo· Apr 22, 2024, 22:04 UTC · re: Phillip Wood · lore

Re: [PATCH 1/4] apply: plug a leak in apply_data

On Mon, Apr 22, 2024 at 04:41:18PM +0100, Phillip Wood wrote:
Show 20 quoted lines
> On 21/04/2024 11:28, Rubén Justo wrote:
> > Plug a leak we have since cfb6f9acc3 (apply: accept -3/--3way command
> > line option, 2012-05-08).
> 
> Looking at that commit I see
> 
> -        if (apply_fragments(&image, patch) < 0)
> -                return -1; /* note with --reject this succeeds. */
> +        if (apply_fragments(&image, patch) < 0) {
> +                /* Note: with --reject, apply_fragments() returns 0 */
> +                if (!threeway || try_threeway(&image, patch, st, ce) < 0)
> +                        return -1;
> +        }
> 
> So the leak existed before that commit. Indeed it looks
> like the leak predates the introduction of struct image in b94f2eda99
> (builtin-apply.c: make it more line oriented, 2008-01-26) and when
> the patch does not apply we have been leaking the buffer passed to
> apply_fragments() since the beginning of the builtin apply added in
> ac6245e31a3 (Builtin git-apply., 2006-05-23)
You are very right.

I saw that commit. I reviewed again my notes and I recorded the hash of the previous hop; the one that my message, incorrectly, refers to. I'll reroll to fix it.

Thank you very much for your attention.
Junio C Hamano· Apr 22, 2024, 22:27 UTC · re: Rubén Justo · lore

Re: [PATCH 1/4] apply: plug a leak in apply_data

Rubén Justo <rjusto@gmail.com> writes:
Show 12 quoted lines
>> So the leak existed before that commit. Indeed it looks
>> like the leak predates the introduction of struct image in b94f2eda99
>> (builtin-apply.c: make it more line oriented, 2008-01-26) and when
>> the patch does not apply we have been leaking the buffer passed to
>> apply_fragments() since the beginning of the builtin apply added in
>> ac6245e31a3 (Builtin git-apply., 2006-05-23)
>
> You are very right.
>
> I saw that commit.  I reviewed again my notes and I recorded the hash of
> the previous hop;  the one that my message, incorrectly, refers to.
> I'll reroll to fix it.

FWIW, the very first version of apply_one_fragment() in 3cca928d (git-apply: first cut at actually checking fragment data, 2005-06-05) already had leak; it "returns -1" upon an error without freeing old/new buffers. Over the years, the shape of the code changed, but the leak survived.

Rubén Justo· Apr 21, 2024, 10:28 UTC · re: Rubén Justo · lore

[PATCH 3/4] add-patch: plug a leak handling the '/' command

Plug a leak we have since d6cf873340 (built-in add -p: implement the '/' ("search regex") command, 2019-12-13).

This leak can be triggered with:
    $ printf "A\n\nB\n" >file
    $ git add file && git commit -m file
    $ printf "AA\n\nBB\n" >file
    $ printf "s\n/ .\n" >lines
    $ git add -p <lines
Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 add-patch.c | 1 +
 1 file changed, 1 insertion(+)
Show changes to add-patch.c +1 −0
diff --git a/add-patch.c b/add-patch.c
index a06dd18985..0997d4af73 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1646,6 +1646,7 @@ static int patch_update_file(struct add_p_state *s,
 				err(s, _("No hunk matches the given pattern"));
 				break;
 			}
+			regfree(&regex);
 			hunk_index = i;
 		} else if (s->answer.buf[0] == 's') {
 			size_t splittable_into = hunk->splittable_into;

-- 
2.45.0.rc0.4.g08f77eb516
Rubén Justo· Apr 21, 2024, 10:29 UTC · re: Rubén Justo · lore

[PATCH 4/4] add: plug a leak on interactive_add

Plug a leak we have since 5a76aff1a6 (add: convert to use parse_pathspec, 2013-07-14).

This leak can be triggered with:
    $ git add -p anything
Fixing this leak allows us to mark as leak-free the following tests:
    + t3701-add-interactive.sh
    + t7514-commit-patch.sh

Mark them with "TEST_PASSES_SANITIZE_LEAK=true" to notice and fix promply any new leak that may be introduced and triggered by them in the future.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/add.c              | 9 ++++++---
 t/t3701-add-interactive.sh | 1 +
 t/t7514-commit-patch.sh    | 2 ++
 3 files changed, 9 insertions(+), 3 deletions(-)
Show changes to 3 files +9 −3

builtin/add.c, t/t3701-add-interactive.sh, t/t7514-commit-patch.sh

diff --git a/builtin/add.c b/builtin/add.c
index ae723bc85e..b7d3ff1e28 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -150,7 +150,7 @@ static int refresh(int verbose, const struct pathspec *pathspec)
 int interactive_add(const char **argv, const char *prefix, int patch)
 {
 	struct pathspec pathspec;
-	int unused;
+	int unused, ret;
 
 	if (!git_config_get_bool("add.interactive.usebuiltin", &unused))
 		warning(_("the add.interactive.useBuiltin setting has been removed!\n"
@@ -163,9 +163,12 @@ int interactive_add(const char **argv, const char *prefix, int patch)
 		       prefix, argv);
 
 	if (patch)
-		return !!run_add_p(the_repository, ADD_P_ADD, NULL, &pathspec);
+		ret = !!run_add_p(the_repository, ADD_P_ADD, NULL, &pathspec);
 	else
-		return !!run_add_i(the_repository, &pathspec);
+		ret = !!run_add_i(the_repository, &pathspec);
+
+	clear_pathspec(&pathspec);
+	return ret;
 }
 
 static int edit_patch(int argc, const char **argv, const char *prefix)
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index bc55255b0a..04d8333373 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -4,6 +4,7 @@ test_description='add -i basic tests'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-terminal.sh
 
diff --git a/t/t7514-commit-patch.sh b/t/t7514-commit-patch.sh
index b4de10a5dd..03ba0c0e73 100755
--- a/t/t7514-commit-patch.sh
+++ b/t/t7514-commit-patch.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='hunk edit with "commit -p -m"'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup (initial)' '

-- 
2.45.0.rc0.4.g08f77eb516
Phillip Wood· Apr 22, 2024, 15:43 UTC · re: Rubén Justo · lore

Re: [PATCH 4/4] add: plug a leak on interactive_add

Hi Rubén
On 21/04/2024 11:29, Rubén Justo wrote:
Show 14 quoted lines
> Plug a leak we have since 5a76aff1a6 (add: convert to use
> parse_pathspec, 2013-07-14).
> 
> This leak can be triggered with:
>      $ git add -p anything
> 
> Fixing this leak allows us to mark as leak-free the following tests:
> 
>      + t3701-add-interactive.sh
>      + t7514-commit-patch.sh
> 
> Mark them with "TEST_PASSES_SANITIZE_LEAK=true" to notice and fix
> promply any new leak that may be introduced and triggered by them in the
> future.

This makes me wonder if we're freeing the pathspec properly when using '--patch' in checkout, reset, restore and stash.

Best Wishes
Phillip
Show 60 quoted lines
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>   builtin/add.c              | 9 ++++++---
>   t/t3701-add-interactive.sh | 1 +
>   t/t7514-commit-patch.sh    | 2 ++
>   3 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/builtin/add.c b/builtin/add.c
> index ae723bc85e..b7d3ff1e28 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -150,7 +150,7 @@ static int refresh(int verbose, const struct pathspec *pathspec)
>   int interactive_add(const char **argv, const char *prefix, int patch)
>   {
>   	struct pathspec pathspec;
> -	int unused;
> +	int unused, ret;
>   
>   	if (!git_config_get_bool("add.interactive.usebuiltin", &unused))
>   		warning(_("the add.interactive.useBuiltin setting has been removed!\n"
> @@ -163,9 +163,12 @@ int interactive_add(const char **argv, const char *prefix, int patch)
>   		       prefix, argv);
>   
>   	if (patch)
> -		return !!run_add_p(the_repository, ADD_P_ADD, NULL, &pathspec);
> +		ret = !!run_add_p(the_repository, ADD_P_ADD, NULL, &pathspec);
>   	else
> -		return !!run_add_i(the_repository, &pathspec);
> +		ret = !!run_add_i(the_repository, &pathspec);
> +
> +	clear_pathspec(&pathspec);
> +	return ret;
>   }
>   
>   static int edit_patch(int argc, const char **argv, const char *prefix)
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index bc55255b0a..04d8333373 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -4,6 +4,7 @@ test_description='add -i basic tests'
>   GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>   export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   . "$TEST_DIRECTORY"/lib-terminal.sh
>   
> diff --git a/t/t7514-commit-patch.sh b/t/t7514-commit-patch.sh
> index b4de10a5dd..03ba0c0e73 100755
> --- a/t/t7514-commit-patch.sh
> +++ b/t/t7514-commit-patch.sh
> @@ -1,6 +1,8 @@
>   #!/bin/sh
>   
>   test_description='hunk edit with "commit -p -m"'
> +
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   
>   test_expect_success 'setup (initial)' '
Rubén Justo· Apr 22, 2024, 23:04 UTC · re: Phillip Wood · lore

Re: [PATCH 4/4] add: plug a leak on interactive_add

On 4/22/24 5:43 PM, Phillip Wood wrote:
Show 16 quoted lines
>> Plug a leak we have since 5a76aff1a6 (add: convert to use
>> parse_pathspec, 2013-07-14).
>>
>> This leak can be triggered with:
>>      $ git add -p anything
>>
>> Fixing this leak allows us to mark as leak-free the following tests:
>>
>>      + t3701-add-interactive.sh
>>      + t7514-commit-patch.sh
>>
>> Mark them with "TEST_PASSES_SANITIZE_LEAK=true" to notice and fix
>> promply any new leak that may be introduced and triggered by them in the
>> future.
> 
> This makes me wonder if we're freeing the pathspec properly when using '--patch' in checkout, reset, restore and stash.
I haven't checked it thoroughly, but I think we're fine in those cases.
Patrick Steinhardt· Apr 22, 2024, 06:06 UTC · re: Rubén Justo · lore

Re: [PATCH 0/4] mark t3701-add-interactive.sh as leak-free

On Sun, Apr 21, 2024 at 12:22:46PM +0200, Rubén Justo wrote:
Show 5 quoted lines
> Rubén Justo (4):
>   apply: plug a leak in apply_data
>   add-interactive: plug a leak in get_untracked_files
>   add-patch: plug a leak handling the '/' command
>   add: plug a leak on interactive_add
All of these patches look obviously good to me. Thanks!
Patrick
Junio C Hamano· Apr 22, 2024, 15:30 UTC · re: Rubén Justo · lore

Re: [PATCH 0/4] mark t3701-add-interactive.sh as leak-free

Rubén Justo <rjusto@gmail.com> writes:
Show 5 quoted lines
> Rubén Justo (4):
>   apply: plug a leak in apply_data
>   add-interactive: plug a leak in get_untracked_files
>   add-patch: plug a leak handling the '/' command
>   add: plug a leak on interactive_add
Thanks, will queue.
Show 16 quoted lines
>
>  add-interactive.c          | 1 +
>  add-patch.c                | 1 +
>  apply.c                    | 4 +++-
>  builtin/add.c              | 9 ++++++---
>  t/t2016-checkout-patch.sh  | 1 +
>  t/t3701-add-interactive.sh | 1 +
>  t/t4103-apply-binary.sh    | 1 +
>  t/t4104-apply-boundary.sh  | 1 +
>  t/t4113-apply-ending.sh    | 1 +
>  t/t4117-apply-reject.sh    | 1 +
>  t/t4123-apply-shrink.sh    | 1 +
>  t/t4252-am-options.sh      | 2 ++
>  t/t4258-am-quoted-cr.sh    | 1 +
>  t/t7514-commit-patch.sh    | 2 ++
>  14 files changed, 23 insertions(+), 4 deletions(-)
Rubén Justo· Apr 22, 2024, 22:51 UTC · re: Rubén Justo · lore

[PATCH v2 0/4] mark t3701-add-interactive.sh as leak-free

Rubén Justo (4):
  apply: plug a leak in apply_data
  add-interactive: plug a leak in get_untracked_files
  add-patch: plug a leak handling the '/' command
  add: plug a leak on interactive_add
 add-interactive.c          | 1 +
 add-patch.c                | 1 +
 apply.c                    | 4 +++-
 builtin/add.c              | 9 ++++++---
 t/t2016-checkout-patch.sh  | 1 +
 t/t3701-add-interactive.sh | 1 +
 t/t4103-apply-binary.sh    | 1 +
 t/t4104-apply-boundary.sh  | 1 +
 t/t4113-apply-ending.sh    | 1 +
 t/t4117-apply-reject.sh    | 1 +
 t/t4123-apply-shrink.sh    | 1 +
 t/t4252-am-options.sh      | 2 ++
 t/t4258-am-quoted-cr.sh    | 1 +
 t/t7514-commit-patch.sh    | 2 ++
 14 files changed, 23 insertions(+), 4 deletions(-)
Range-diff against v1:
1:  18e4c7f653 ! 1:  75cb700eab apply: plug a leak in apply_data
    @@ Metadata
      ## Commit message ##
         apply: plug a leak in apply_data
     
    -    Plug a leak we have since cfb6f9acc3 (apply: accept -3/--3way command
    -    line option, 2012-05-08).
    +    We have an execution path in apply_data that leaks the local struct
    +    image.  Plug it.
     
         This leak can be triggered with:
     
2:  21d6c2dd16 = 2:  bee002b0ae add-interactive: plug a leak in get_untracked_files
3:  f69a6a30a1 = 3:  7d1a94dd91 add-patch: plug a leak handling the '/' command
4:  5d9607f153 = 4:  fff7e48949 add: plug a leak on interactive_add
-- 
2.45.0.rc0.4.gfff7e48949
Rubén Justo· Apr 22, 2024, 22:54 UTC · re: Rubén Justo · lore

[PATCH v2 1/4] apply: plug a leak in apply_data

We have an execution path in apply_data that leaks the local struct image. Plug it.

This leak can be triggered with:
    $ echo foo >file
    $ git add file && git commit -m file
    $ echo bar >file
    $ git diff file >diff
    $ sed s/foo/frotz/ <diff >baddiff
    $ git apply --cached <baddiff
Fixing this leak allows us to mark as leak-free the following tests:
    + t2016-checkout-patch.sh
    + t4103-apply-binary.sh
    + t4104-apply-boundary.sh
    + t4113-apply-ending.sh
    + t4117-apply-reject.sh
    + t4123-apply-shrink.sh
    + t4252-am-options.sh
    + t4258-am-quoted-cr.sh

Mark them with "TEST_PASSES_SANITIZE_LEAK=true" to notice and fix promply any new leak that may be introduced and triggered by them in the future.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 apply.c                   | 4 +++-
 t/t2016-checkout-patch.sh | 1 +
 t/t4103-apply-binary.sh   | 1 +
 t/t4104-apply-boundary.sh | 1 +
 t/t4113-apply-ending.sh   | 1 +
 t/t4117-apply-reject.sh   | 1 +
 t/t4123-apply-shrink.sh   | 1 +
 t/t4252-am-options.sh     | 2 ++
 t/t4258-am-quoted-cr.sh   | 1 +
 9 files changed, 12 insertions(+), 1 deletion(-)
Show changes to 9 files +12 −1

apply.c, t/t2016-checkout-patch.sh, t/t4103-apply-binary.sh, t/t4104-apply-boundary.sh, t/t4113-apply-ending.sh, t/t4117-apply-reject.sh, t/t4123-apply-shrink.sh, t/t4252-am-options.sh, t/t4258-am-quoted-cr.sh

diff --git a/apply.c b/apply.c
index 34f20326a7..2f752d71a8 100644
--- a/apply.c
+++ b/apply.c
@@ -3712,8 +3712,10 @@ static int apply_data(struct apply_state *state, struct patch *patch,
 			fprintf(stderr, _("Falling back to direct application...\n"));
 
 		/* Note: with --reject, apply_fragments() returns 0 */
-		if (patch->direct_to_threeway || apply_fragments(state, &image, patch) < 0)
+		if (patch->direct_to_threeway || apply_fragments(state, &image, patch) < 0) {
+			clear_image(&image);
 			return -1;
+		}
 	}
 	patch->result = image.buf;
 	patch->resultsize = image.len;
diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh
index c4f9bf09aa..c40b661ac1 100755
--- a/t/t2016-checkout-patch.sh
+++ b/t/t2016-checkout-patch.sh
@@ -2,6 +2,7 @@
 
 test_description='git checkout --patch'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./lib-patch-mode.sh
 
 test_expect_success 'setup' '
diff --git a/t/t4103-apply-binary.sh b/t/t4103-apply-binary.sh
index d370ecfe0d..144619ab87 100755
--- a/t/t4103-apply-binary.sh
+++ b/t/t4103-apply-binary.sh
@@ -9,6 +9,7 @@ test_description='git apply handling binary patches
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '
diff --git a/t/t4104-apply-boundary.sh b/t/t4104-apply-boundary.sh
index 71ef4132d1..dc501aac38 100755
--- a/t/t4104-apply-boundary.sh
+++ b/t/t4104-apply-boundary.sh
@@ -5,6 +5,7 @@
 
 test_description='git apply boundary tests'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 L="c d e f g h i j k l m n o p q r s t u v w x"
diff --git a/t/t4113-apply-ending.sh b/t/t4113-apply-ending.sh
index 66fa51591e..2c65c6a169 100755
--- a/t/t4113-apply-ending.sh
+++ b/t/t4113-apply-ending.sh
@@ -6,6 +6,7 @@
 test_description='git apply trying to add an ending line.
 
 '
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # setup
diff --git a/t/t4117-apply-reject.sh b/t/t4117-apply-reject.sh
index c86d05a96f..4d15ccd28e 100755
--- a/t/t4117-apply-reject.sh
+++ b/t/t4117-apply-reject.sh
@@ -7,6 +7,7 @@ test_description='git apply with rejects
 
 '
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '
diff --git a/t/t4123-apply-shrink.sh b/t/t4123-apply-shrink.sh
index 3ef84619f5..3601c0c5dc 100755
--- a/t/t4123-apply-shrink.sh
+++ b/t/t4123-apply-shrink.sh
@@ -2,6 +2,7 @@
 
 test_description='apply a patch that is larger than the preimage'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 cat >F  <<\EOF
diff --git a/t/t4252-am-options.sh b/t/t4252-am-options.sh
index e758e634a3..5b680dc755 100755
--- a/t/t4252-am-options.sh
+++ b/t/t4252-am-options.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='git am with options and not losing them'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 tm="$TEST_DIRECTORY/t4252"
diff --git a/t/t4258-am-quoted-cr.sh b/t/t4258-am-quoted-cr.sh
index 201915b45a..3573c9147f 100755
--- a/t/t4258-am-quoted-cr.sh
+++ b/t/t4258-am-quoted-cr.sh
@@ -2,6 +2,7 @@
 
 test_description='test am --quoted-cr=<action>'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 DATA="$TEST_DIRECTORY/t4258"

-- 
2.45.0.rc0.4.gfff7e48949
Rubén Justo· Apr 22, 2024, 22:54 UTC · re: Rubén Justo · lore

[PATCH v2 2/4] add-interactive: plug a leak in get_untracked_files

Plug a leak we have since ab1e1cccaf (built-in add -i: re-implement `add-untracked` in C, 2019-11-29).

This leak can be triggered with:
	$ echo a | git add -i

As a curiosity, we have a somewhat similar function in builtin/stash.c, which correctly frees the memory.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 add-interactive.c | 1 +
 1 file changed, 1 insertion(+)
Show changes to add-interactive.c +1 −0
diff --git a/add-interactive.c b/add-interactive.c
index 6bf87e7ae7..e17602b5e4 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -865,6 +865,7 @@ static int get_untracked_files(struct repository *r,
 	}
 
 	strbuf_release(&buf);
+	dir_clear(&dir);
 	return 0;
 }
 

-- 
2.45.0.rc0.4.gfff7e48949
Rubén Justo· Apr 22, 2024, 22:54 UTC · re: Rubén Justo · lore

[PATCH v2 3/4] add-patch: plug a leak handling the '/' command

Plug a leak we have since d6cf873340 (built-in add -p: implement the '/' ("search regex") command, 2019-12-13).

This leak can be triggered with:
    $ printf "A\n\nB\n" >file
    $ git add file && git commit -m file
    $ printf "AA\n\nBB\n" >file
    $ printf "s\n/ .\n" >lines
    $ git add -p <lines
Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 add-patch.c | 1 +
 1 file changed, 1 insertion(+)
Show changes to add-patch.c +1 −0
diff --git a/add-patch.c b/add-patch.c
index a06dd18985..0997d4af73 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1646,6 +1646,7 @@ static int patch_update_file(struct add_p_state *s,
 				err(s, _("No hunk matches the given pattern"));
 				break;
 			}
+			regfree(&regex);
 			hunk_index = i;
 		} else if (s->answer.buf[0] == 's') {
 			size_t splittable_into = hunk->splittable_into;

-- 
2.45.0.rc0.4.gfff7e48949
Rubén Justo· Apr 22, 2024, 22:54 UTC · re: Rubén Justo · lore

[PATCH v2 4/4] add: plug a leak on interactive_add

Plug a leak we have since 5a76aff1a6 (add: convert to use parse_pathspec, 2013-07-14).

This leak can be triggered with:
    $ git add -p anything
Fixing this leak allows us to mark as leak-free the following tests:
    + t3701-add-interactive.sh
    + t7514-commit-patch.sh

Mark them with "TEST_PASSES_SANITIZE_LEAK=true" to notice and fix promply any new leak that may be introduced and triggered by them in the future.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/add.c              | 9 ++++++---
 t/t3701-add-interactive.sh | 1 +
 t/t7514-commit-patch.sh    | 2 ++
 3 files changed, 9 insertions(+), 3 deletions(-)
Show changes to 3 files +9 −3

builtin/add.c, t/t3701-add-interactive.sh, t/t7514-commit-patch.sh

diff --git a/builtin/add.c b/builtin/add.c
index ae723bc85e..b7d3ff1e28 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -150,7 +150,7 @@ static int refresh(int verbose, const struct pathspec *pathspec)
 int interactive_add(const char **argv, const char *prefix, int patch)
 {
 	struct pathspec pathspec;
-	int unused;
+	int unused, ret;
 
 	if (!git_config_get_bool("add.interactive.usebuiltin", &unused))
 		warning(_("the add.interactive.useBuiltin setting has been removed!\n"
@@ -163,9 +163,12 @@ int interactive_add(const char **argv, const char *prefix, int patch)
 		       prefix, argv);
 
 	if (patch)
-		return !!run_add_p(the_repository, ADD_P_ADD, NULL, &pathspec);
+		ret = !!run_add_p(the_repository, ADD_P_ADD, NULL, &pathspec);
 	else
-		return !!run_add_i(the_repository, &pathspec);
+		ret = !!run_add_i(the_repository, &pathspec);
+
+	clear_pathspec(&pathspec);
+	return ret;
 }
 
 static int edit_patch(int argc, const char **argv, const char *prefix)
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index bc55255b0a..04d8333373 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -4,6 +4,7 @@ test_description='add -i basic tests'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-terminal.sh
 
diff --git a/t/t7514-commit-patch.sh b/t/t7514-commit-patch.sh
index b4de10a5dd..03ba0c0e73 100755
--- a/t/t7514-commit-patch.sh
+++ b/t/t7514-commit-patch.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 
 test_description='hunk edit with "commit -p -m"'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup (initial)' '

-- 
2.45.0.rc0.4.gfff7e48949
Junio C Hamano· Apr 22, 2024, 23:26 UTC · re: Rubén Justo · lore

Re: [PATCH v2 0/4] mark t3701-add-interactive.sh as leak-free

Rubén Justo <rjusto@gmail.com> writes:
Show 10 quoted lines
> Range-diff against v1:
> 1:  18e4c7f653 ! 1:  75cb700eab apply: plug a leak in apply_data
>     @@ Metadata
>       ## Commit message ##
>          apply: plug a leak in apply_data
>      
>     -    Plug a leak we have since cfb6f9acc3 (apply: accept -3/--3way command
>     -    line option, 2012-05-08).
>     +    We have an execution path in apply_data that leaks the local struct
>     +    image.  Plug it.
This is a nice improvement.

If a bug existed in an ancient version and survived across evolution of the code for a long time, naming a random old version that happened to already have it does not help very much.

Saying something like
    This is an ancient bug whose moral equivalent existed even
    before the data & code structure became the current shape in
    commit X

would give a bit more information, in that X gives a rough estimate how far back in the codebase the fix in the patch can be applied more or less cleanly. In this case, X would be where the struct image was introduced, I guess?

But the updated text is good enough and I do not see a need for reroll. Will queue. Thanks.

← back to recent threads