From: Patrick Steinhardt <ps@pks.im>
Date: Mon, 21 Oct 2024 08:44:53 GMT
Subject: Re: [PATCH 07/21] pretty: clear signature check
Message-ID: <ZxYUfyh5j7EiVRdC@pks.im>
In-Reply-To: <87a5f1d3fb.fsf@iotcl.com>

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