| Message ID | 5d5f6867f918460001f62aaa78f24cf3cbe53a3c.1728624670.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Memory leak fixes (pt.9) | expand |
Patrick Steinhardt <ps@pks.im> writes: > The signature check in of the formatting context is never getting > released. Fix this to plug the resulting memory leak. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > pretty.c | 1 + > t/t4202-log.sh | 1 + > t/t7031-verify-tag-signed-ssh.sh | 1 + > t/t7510-signed-commit.sh | 1 + > t/t7528-signed-commit-ssh.sh | 1 + > 5 files changed, 5 insertions(+) > > diff --git a/pretty.c b/pretty.c > index 6403e268900..098378720a4 100644 > --- a/pretty.c > +++ b/pretty.c > @@ -2032,6 +2032,7 @@ void repo_format_commit_message(struct repository *r, > > free(context.commit_encoding); > repo_unuse_commit_buffer(r, commit, context.message); > + signature_check_clear(&context.signature_check); I was having a very hard time finding where this gets allocated, and to be honest, I still don't know for sure. I think in check_commit_signature() which is called by format_commit_one(). In "[PATCH 20/21] builtin/merge: release outbut buffer after performing merge" you mention it's not obvious to the caller they need know about memory they need to clean up, isn't that case here as well? -- Toon
On Fri, Oct 18, 2024 at 02:02:48PM +0200, Toon Claes wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > The signature check in of the formatting context is never getting > > released. Fix this to plug the resulting memory leak. > > > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > > --- > > pretty.c | 1 + > > t/t4202-log.sh | 1 + > > t/t7031-verify-tag-signed-ssh.sh | 1 + > > t/t7510-signed-commit.sh | 1 + > > t/t7528-signed-commit-ssh.sh | 1 + > > 5 files changed, 5 insertions(+) > > > > diff --git a/pretty.c b/pretty.c > > index 6403e268900..098378720a4 100644 > > --- a/pretty.c > > +++ b/pretty.c > > @@ -2032,6 +2032,7 @@ void repo_format_commit_message(struct repository *r, > > > > free(context.commit_encoding); > > repo_unuse_commit_buffer(r, commit, context.message); > > + signature_check_clear(&context.signature_check); > > I was having a very hard time finding where this gets allocated, and to > be honest, I still don't know for sure. I think in > check_commit_signature() which is called by format_commit_one(). > In "[PATCH 20/21] builtin/merge: release outbut buffer after performing > merge" you mention it's not obvious to the caller they need know about > memory they need to clean up, isn't that case here as well? Well, I think it's clearer in this context, but hidden by the fact that we pass around a wrapper-structure. But that's not really the problem of the `struct signature_check` subsystem, but rather of "pretty.c". In any case, the callchain is: - `format_commit_one()` - `check_commit_signature()` - `check_signature()` - `verify_signed_buffer()`, which is a function pointer depending on whether we verify a GPG, x509 or SSH signature. From my point of view, that callchain isn't all that important in this context. All we need to know is that we allocate the signature check struct on our stack, and as it may get populated we have to clean it up, as well. Patrick
diff --git a/pretty.c b/pretty.c index 6403e268900..098378720a4 100644 --- a/pretty.c +++ b/pretty.c @@ -2032,6 +2032,7 @@ void repo_format_commit_message(struct repository *r, free(context.commit_encoding); repo_unuse_commit_buffer(r, commit, context.message); + signature_check_clear(&context.signature_check); } static void pp_header(struct pretty_print_context *pp, diff --git a/t/t4202-log.sh b/t/t4202-log.sh index 51f7beb59f8..35bec4089a3 100755 --- a/t/t4202-log.sh +++ b/t/t4202-log.sh @@ -5,6 +5,7 @@ test_description='git log' 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-gpg.sh" . "$TEST_DIRECTORY/lib-terminal.sh" diff --git a/t/t7031-verify-tag-signed-ssh.sh b/t/t7031-verify-tag-signed-ssh.sh index 20913b37134..2ee62c07293 100755 --- a/t/t7031-verify-tag-signed-ssh.sh +++ b/t/t7031-verify-tag-signed-ssh.sh @@ -4,6 +4,7 @@ test_description='signed tag 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-gpg.sh" diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh index 0d2dd29fe6a..eb229082e40 100755 --- a/t/t7510-signed-commit.sh +++ b/t/t7510-signed-commit.sh @@ -4,6 +4,7 @@ test_description='signed commit tests' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh GNUPGHOME_NOT_USED=$GNUPGHOME . "$TEST_DIRECTORY/lib-gpg.sh" diff --git a/t/t7528-signed-commit-ssh.sh b/t/t7528-signed-commit-ssh.sh index 065f7806362..68e18856b66 100755 --- a/t/t7528-signed-commit-ssh.sh +++ b/t/t7528-signed-commit-ssh.sh @@ -4,6 +4,7 @@ test_description='ssh signed commit tests' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh GNUPGHOME_NOT_USED=$GNUPGHOME . "$TEST_DIRECTORY/lib-gpg.sh"
The signature check in of the formatting context is never getting released. Fix this to plug the resulting memory leak. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- pretty.c | 1 + t/t4202-log.sh | 1 + t/t7031-verify-tag-signed-ssh.sh | 1 + t/t7510-signed-commit.sh | 1 + t/t7528-signed-commit-ssh.sh | 1 + 5 files changed, 5 insertions(+)