* [PATCH 0/4] mark t3701-add-interactive.sh as leak-free
@ 2024-04-21 10:22 Rubén Justo
2024-04-21 10:27 ` [PATCH 2/4] add-interactive: plug a leak in get_untracked_files Rubén Justo
` (6 more replies)
0 siblings, 7 replies; 19+ messages in thread
From: Rubén Justo @ 2024-04-21 10:22 UTC (permalink / raw)
To: Git List
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 2/4] add-interactive: plug a leak in get_untracked_files
2024-04-21 10:22 [PATCH 0/4] mark t3701-add-interactive.sh as leak-free Rubén Justo
@ 2024-04-21 10:27 ` Rubén Justo
2024-04-22 15:50 ` Junio C Hamano
2024-04-21 10:28 ` [PATCH 1/4] apply: plug a leak in apply_data Rubén Justo
` (5 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Rubén Justo @ 2024-04-21 10:27 UTC (permalink / raw)
To: Git List
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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 1/4] apply: plug a leak in apply_data
2024-04-21 10:22 [PATCH 0/4] mark t3701-add-interactive.sh as leak-free Rubén Justo
2024-04-21 10:27 ` [PATCH 2/4] add-interactive: plug a leak in get_untracked_files Rubén Justo
@ 2024-04-21 10:28 ` Rubén Justo
2024-04-22 15:41 ` Phillip Wood
2024-04-21 10:28 ` [PATCH 3/4] add-patch: plug a leak handling the '/' command Rubén Justo
` (4 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Rubén Justo @ 2024-04-21 10:28 UTC (permalink / raw)
To: Git List
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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 3/4] add-patch: plug a leak handling the '/' command
2024-04-21 10:22 [PATCH 0/4] mark t3701-add-interactive.sh as leak-free Rubén Justo
2024-04-21 10:27 ` [PATCH 2/4] add-interactive: plug a leak in get_untracked_files Rubén Justo
2024-04-21 10:28 ` [PATCH 1/4] apply: plug a leak in apply_data Rubén Justo
@ 2024-04-21 10:28 ` Rubén Justo
2024-04-21 10:29 ` [PATCH 4/4] add: plug a leak on interactive_add Rubén Justo
` (3 subsequent siblings)
6 siblings, 0 replies; 19+ messages in thread
From: Rubén Justo @ 2024-04-21 10:28 UTC (permalink / raw)
To: Git List
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(®ex);
hunk_index = i;
} else if (s->answer.buf[0] == 's') {
size_t splittable_into = hunk->splittable_into;
--
2.45.0.rc0.4.g08f77eb516
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 4/4] add: plug a leak on interactive_add
2024-04-21 10:22 [PATCH 0/4] mark t3701-add-interactive.sh as leak-free Rubén Justo
` (2 preceding siblings ...)
2024-04-21 10:28 ` [PATCH 3/4] add-patch: plug a leak handling the '/' command Rubén Justo
@ 2024-04-21 10:29 ` Rubén Justo
2024-04-22 15:43 ` Phillip Wood
2024-04-22 6:06 ` [PATCH 0/4] mark t3701-add-interactive.sh as leak-free Patrick Steinhardt
` (2 subsequent siblings)
6 siblings, 1 reply; 19+ messages in thread
From: Rubén Justo @ 2024-04-21 10:29 UTC (permalink / raw)
To: Git List
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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 0/4] mark t3701-add-interactive.sh as leak-free
2024-04-21 10:22 [PATCH 0/4] mark t3701-add-interactive.sh as leak-free Rubén Justo
` (3 preceding siblings ...)
2024-04-21 10:29 ` [PATCH 4/4] add: plug a leak on interactive_add Rubén Justo
@ 2024-04-22 6:06 ` Patrick Steinhardt
2024-04-22 15:30 ` Junio C Hamano
2024-04-22 22:51 ` [PATCH v2 " Rubén Justo
6 siblings, 0 replies; 19+ messages in thread
From: Patrick Steinhardt @ 2024-04-22 6:06 UTC (permalink / raw)
To: Rubén Justo; +Cc: Git List
[-- 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 --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/4] mark t3701-add-interactive.sh as leak-free
2024-04-21 10:22 [PATCH 0/4] mark t3701-add-interactive.sh as leak-free Rubén Justo
` (4 preceding siblings ...)
2024-04-22 6:06 ` [PATCH 0/4] mark t3701-add-interactive.sh as leak-free Patrick Steinhardt
@ 2024-04-22 15:30 ` Junio C Hamano
2024-04-22 22:51 ` [PATCH v2 " Rubén Justo
6 siblings, 0 replies; 19+ messages in thread
From: Junio C Hamano @ 2024-04-22 15:30 UTC (permalink / raw)
To: Rubén Justo; +Cc: Git List
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(-)
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/4] apply: plug a leak in apply_data
2024-04-21 10:28 ` [PATCH 1/4] apply: plug a leak in apply_data Rubén Justo
@ 2024-04-22 15:41 ` Phillip Wood
2024-04-22 22:04 ` Rubén Justo
0 siblings, 1 reply; 19+ messages in thread
From: Phillip Wood @ 2024-04-22 15:41 UTC (permalink / raw)
To: Rubén Justo, Git List
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"
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 4/4] add: plug a leak on interactive_add
2024-04-21 10:29 ` [PATCH 4/4] add: plug a leak on interactive_add Rubén Justo
@ 2024-04-22 15:43 ` Phillip Wood
2024-04-22 23:04 ` Rubén Justo
0 siblings, 1 reply; 19+ messages in thread
From: Phillip Wood @ 2024-04-22 15:43 UTC (permalink / raw)
To: Rubén Justo, Git List
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)' '
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/4] add-interactive: plug a leak in get_untracked_files
2024-04-21 10:27 ` [PATCH 2/4] add-interactive: plug a leak in get_untracked_files Rubén Justo
@ 2024-04-22 15:50 ` Junio C Hamano
0 siblings, 0 replies; 19+ messages in thread
From: Junio C Hamano @ 2024-04-22 15:50 UTC (permalink / raw)
To: Rubén Justo; +Cc: Git List
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;
> }
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/4] apply: plug a leak in apply_data
2024-04-22 15:41 ` Phillip Wood
@ 2024-04-22 22:04 ` Rubén Justo
2024-04-22 22:27 ` Junio C Hamano
0 siblings, 1 reply; 19+ messages in thread
From: Rubén Justo @ 2024-04-22 22:04 UTC (permalink / raw)
To: phillip.wood; +Cc: Junio C Hamano, Git List
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/4] apply: plug a leak in apply_data
2024-04-22 22:04 ` Rubén Justo
@ 2024-04-22 22:27 ` Junio C Hamano
0 siblings, 0 replies; 19+ messages in thread
From: Junio C Hamano @ 2024-04-22 22:27 UTC (permalink / raw)
To: Rubén Justo; +Cc: phillip.wood, Git List
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 0/4] mark t3701-add-interactive.sh as leak-free
2024-04-21 10:22 [PATCH 0/4] mark t3701-add-interactive.sh as leak-free Rubén Justo
` (5 preceding siblings ...)
2024-04-22 15:30 ` Junio C Hamano
@ 2024-04-22 22:51 ` Rubén Justo
2024-04-22 22:54 ` [PATCH v2 1/4] apply: plug a leak in apply_data Rubén Justo
` (4 more replies)
6 siblings, 5 replies; 19+ messages in thread
From: Rubén Justo @ 2024-04-22 22:51 UTC (permalink / raw)
To: Git List
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 1/4] apply: plug a leak in apply_data
2024-04-22 22:51 ` [PATCH v2 " Rubén Justo
@ 2024-04-22 22:54 ` Rubén Justo
2024-04-22 22:54 ` [PATCH v2 2/4] add-interactive: plug a leak in get_untracked_files Rubén Justo
` (3 subsequent siblings)
4 siblings, 0 replies; 19+ messages in thread
From: Rubén Justo @ 2024-04-22 22:54 UTC (permalink / raw)
To: Git List
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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v2 2/4] add-interactive: plug a leak in get_untracked_files
2024-04-22 22:51 ` [PATCH v2 " Rubén Justo
2024-04-22 22:54 ` [PATCH v2 1/4] apply: plug a leak in apply_data Rubén Justo
@ 2024-04-22 22:54 ` Rubén Justo
2024-04-22 22:54 ` [PATCH v2 3/4] add-patch: plug a leak handling the '/' command Rubén Justo
` (2 subsequent siblings)
4 siblings, 0 replies; 19+ messages in thread
From: Rubén Justo @ 2024-04-22 22:54 UTC (permalink / raw)
To: Git List
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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v2 3/4] add-patch: plug a leak handling the '/' command
2024-04-22 22:51 ` [PATCH v2 " Rubén Justo
2024-04-22 22:54 ` [PATCH v2 1/4] apply: plug a leak in apply_data Rubén Justo
2024-04-22 22:54 ` [PATCH v2 2/4] add-interactive: plug a leak in get_untracked_files Rubén Justo
@ 2024-04-22 22:54 ` Rubén Justo
2024-04-22 22:54 ` [PATCH v2 4/4] add: plug a leak on interactive_add Rubén Justo
2024-04-22 23:26 ` [PATCH v2 0/4] mark t3701-add-interactive.sh as leak-free Junio C Hamano
4 siblings, 0 replies; 19+ messages in thread
From: Rubén Justo @ 2024-04-22 22:54 UTC (permalink / raw)
To: Git List
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(®ex);
hunk_index = i;
} else if (s->answer.buf[0] == 's') {
size_t splittable_into = hunk->splittable_into;
--
2.45.0.rc0.4.gfff7e48949
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v2 4/4] add: plug a leak on interactive_add
2024-04-22 22:51 ` [PATCH v2 " Rubén Justo
` (2 preceding siblings ...)
2024-04-22 22:54 ` [PATCH v2 3/4] add-patch: plug a leak handling the '/' command Rubén Justo
@ 2024-04-22 22:54 ` Rubén Justo
2024-04-22 23:26 ` [PATCH v2 0/4] mark t3701-add-interactive.sh as leak-free Junio C Hamano
4 siblings, 0 replies; 19+ messages in thread
From: Rubén Justo @ 2024-04-22 22:54 UTC (permalink / raw)
To: Git List
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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 4/4] add: plug a leak on interactive_add
2024-04-22 15:43 ` Phillip Wood
@ 2024-04-22 23:04 ` Rubén Justo
0 siblings, 0 replies; 19+ messages in thread
From: Rubén Justo @ 2024-04-22 23:04 UTC (permalink / raw)
To: phillip.wood, Git List
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 0/4] mark t3701-add-interactive.sh as leak-free
2024-04-22 22:51 ` [PATCH v2 " Rubén Justo
` (3 preceding siblings ...)
2024-04-22 22:54 ` [PATCH v2 4/4] add: plug a leak on interactive_add Rubén Justo
@ 2024-04-22 23:26 ` Junio C Hamano
4 siblings, 0 replies; 19+ messages in thread
From: Junio C Hamano @ 2024-04-22 23:26 UTC (permalink / raw)
To: Rubén Justo; +Cc: Git List
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2024-04-22 23:27 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-21 10:22 [PATCH 0/4] mark t3701-add-interactive.sh as leak-free Rubén Justo
2024-04-21 10:27 ` [PATCH 2/4] add-interactive: plug a leak in get_untracked_files Rubén Justo
2024-04-22 15:50 ` Junio C Hamano
2024-04-21 10:28 ` [PATCH 1/4] apply: plug a leak in apply_data Rubén Justo
2024-04-22 15:41 ` Phillip Wood
2024-04-22 22:04 ` Rubén Justo
2024-04-22 22:27 ` Junio C Hamano
2024-04-21 10:28 ` [PATCH 3/4] add-patch: plug a leak handling the '/' command Rubén Justo
2024-04-21 10:29 ` [PATCH 4/4] add: plug a leak on interactive_add Rubén Justo
2024-04-22 15:43 ` Phillip Wood
2024-04-22 23:04 ` Rubén Justo
2024-04-22 6:06 ` [PATCH 0/4] mark t3701-add-interactive.sh as leak-free Patrick Steinhardt
2024-04-22 15:30 ` Junio C Hamano
2024-04-22 22:51 ` [PATCH v2 " Rubén Justo
2024-04-22 22:54 ` [PATCH v2 1/4] apply: plug a leak in apply_data Rubén Justo
2024-04-22 22:54 ` [PATCH v2 2/4] add-interactive: plug a leak in get_untracked_files Rubén Justo
2024-04-22 22:54 ` [PATCH v2 3/4] add-patch: plug a leak handling the '/' command Rubén Justo
2024-04-22 22:54 ` [PATCH v2 4/4] add: plug a leak on interactive_add Rubén Justo
2024-04-22 23:26 ` [PATCH v2 0/4] mark t3701-add-interactive.sh as leak-free Junio C Hamano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).