Re: [PATCH 1/4] apply: plug a leak in apply_data
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Apr 22, 2024, 15:41 UTC
- Message-ID
- <74a6465c-01b6-4a65-8491-751244d37c82@gmail.com>
- In-Reply-To
- <8e6ab088-2026-43e7-a869-b1c7185ee765@gmail.com>
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"