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

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

[Rubén Justo]

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

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

[Rubén Justo]

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(+)

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

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

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> 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.

> 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;
>  }

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

[Rubén Justo]

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(-)

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

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

[Phillip Wood]

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


> 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"

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

[Rubén Justo]

On Mon, Apr 22, 2024 at 04:41:18PM +0100, Phillip Wood wrote:

> 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.

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

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

>> 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.

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

[Rubén Justo]

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(+)

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

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

[Rubén Justo]

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(-)

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

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

[Phillip Wood]

Hi Rubén

On 21/04/2024 11:29, Rubén Justo wrote:
> 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

> 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)' '

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

[Rubén Justo]

On 4/22/24 5:43 PM, Phillip Wood wrote:

>> 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.

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 341 bytes --]

On Sun, Apr 21, 2024 at 12:22:46PM +0200, Rubén Justo wrote:
> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> 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.

>
>  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(-)

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

[Rubén Justo]

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

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

[Rubén Justo]

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(-)

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

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

[Rubén Justo]

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(+)

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

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

[Rubén Justo]

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(+)

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

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

[Rubén Justo]

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(-)

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

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

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> 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.