From: Phillip Wood <phillip.wood123@gmail.com>
Date: Mon, 22 Apr 2024 15:41:18 GMT
Subject: Re: [PATCH 1/4] apply: plug a leak in apply_data
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


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