Re: [PATCH v5 0/6] fast-export, fast-import: add support for signed-commits

From: Christian Couder <christian.couder@gmail.com>
Date: Mar 10, 2025, 15:59 UTC
Message-ID: <CAP8UFD1TyDQahYOm9D8ohU-F95XneOgk7fg5mSH_k+s3ZG7omg@mail.gmail.com>
In-Reply-To: <98b4c9e7-4034-4692-bc86-f6b905dcc5aa@gmail.com>

Hi Phillip,

On Tue, Feb 25, 2025 at 3:53 PM Phillip Wood <phillip.wood123@gmail.com> wrote:

>
> Hi Christian
>
> I've only glanced over this series,

Thanks for taking a look at it!

Show 11 quoted lines

> 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:

Show 10 quoted 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:

Show 16 quoted lines

> >      @@ 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.