| Message ID | 8e6ab088-2026-43e7-a869-b1c7185ee765@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mark t3701-add-interactive.sh as leak-free | expand |
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"
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.
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.
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"
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(-)