From: Phillip Wood <phillip.wood123@gmail.com>
To: "Rubén Justo" <rjusto@gmail.com>, "Git List" <git@vger.kernel.org>
Subject: Re: [PATCH 1/4] apply: plug a leak in apply_data
Date: Mon, 22 Apr 2024 16:41:18 +0100 [thread overview]
Message-ID: <74a6465c-01b6-4a65-8491-751244d37c82@gmail.com> (raw)
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"
next prev parent reply other threads:[~2024-04-22 15:41 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=74a6465c-01b6-4a65-8491-751244d37c82@gmail.com \
--to=phillip.wood123@gmail.com \
--cc=git@vger.kernel.org \
--cc=phillip.wood@dunelm.org.uk \
--cc=rjusto@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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 read-only IMAP folder(s) and NNTP newsgroup(s).