Hi Phillip,
On Tue, Feb 25, 2025 at 3:53 PM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote:
>
> Hi Christian
>
> I've only glanced over this series,
Thanks for taking a look at it!
> but I did notice a memory leak
>
> On 24/02/2025 14:27, Christian Couder wrote:
> >
> > + * The returned string has had the ' ' line continuation markers
> > -+ * removed, and points to staticly allocated memory (not to memory
> > ++ * removed, and points to statically allocated memory (not to memory
>
> This corrects the spelling but the changes below remove the static
> buffer so the user is now responsible for freeing the returned string.
> That means this comment is wrong
Yeah, this part of the comment is wrong. I have changed it in the next
version to the following:
* The returned string has had the ' ' line continuation markers
* removed, and points to allocated memory that must be free()d (not
* to memory within 'msg').
> and I don't see any corresponding
> changes to the callers to free the memory.
It is called by the following lines:
> > -+ if ((signature = find_commit_multiline_header(commit_buffer_cursor + 1, "gpgsig", &commit_buffer_cursor)))
> > -+ signature_alg = "sha1";
> > -+ else if ((signature = find_commit_multiline_header(commit_buffer_cursor + 1, "gpgsig-sha256", &commit_buffer_cursor)))
> > -+ signature_alg = "sha256";
> > ++ if (*commit_buffer_cursor == '\n') {
> > ++ if ((signature = find_commit_multiline_header(commit_buffer_cursor + 1, "gpgsig", &commit_buffer_cursor)))
> > ++ signature_alg = "sha1";
> > ++ else if ((signature = find_commit_multiline_header(commit_buffer_cursor + 1, "gpgsig-sha256", &commit_buffer_cursor)))
> > ++ signature_alg = "sha256";
> > ++ }
so the 'signature' variable points to the allocated memory, and then
it's used like this:
> > @@ builtin/fast-export.c: static void handle_commit(struct commit *commit, struct r
> > printf("%.*s\n%.*s\n",
> > (int)(author_end - author), author,
> > (int)(committer_end - committer), committer);
> > -+ if (signature)
> > -+ switch(signed_commit_mode) {
> > ++ if (signature) {
> > ++ switch (signed_commit_mode) {
> > + case SIGN_ABORT:
> > + die("encountered signed commit %s; use "
> > + "--signed-commits=<mode> to handle it",
> > @@ builtin/fast-export.c: static void handle_commit(struct commit *commit, struct r
> > + case SIGN_STRIP:
> > + break;
> > + }
> > ++ free((char *)signature);
And eventually the memory is freed by the added call to free() above.
> > ++ }
But yeah, the description of the changes since the previous version in
the cover letter might have done a better job of explaining this.
