Re: git ssh signing changed broke tag merge message contents

From: Taylor Blau <me@ttaylorr.com>
Date: Jan 10, 2022, 17:19 UTC
Message-ID: <YdxqshqXB/+ApOn2@nand.local>
In-Reply-To: <CAHk-=whXPxWL7z3GiPkaDt+yygrRmagrYUnib7Lx=Vvrqx2ufg@mail.gmail.com>

On Mon, Jan 10, 2022 at 08:42:07AM -0800, Linus Torvalds wrote:

Show 6 quoted lines

> So I made the mistake of updating my git tree as I started doing my
> merge window for 5.17, and suddenly all the messages from signed tags
> disappeared from the merge commits.
>
> I bisected it to commit 02769437e1 ("ssh signing: use sigc struct to
> pass payload"), but haven't done any other analysis.

Thanks for the reproduction and bisection.

> I assume it's the change to fmt-merge-msg.c, but have no time to actually check.

Yes, 02769437e1 appears to introduces an inadvertent use-after-free. I'll write up the details and post the patch shortly, but an easy fix is:

--- 8< ---

diff --git a/fmt-merge-msg.c b/fmt-merge-msg.c
index e5c0aff2bf..baca57d5b6 100644
--- a/fmt-merge-msg.c
+++ b/fmt-merge-msg.c
@@ -541,7 +541,6 @@ static void fmt_merge_msg_sigs(struct strbuf *out)
 			else
 				strbuf_addstr(&sig, sigc.output);
 		}
-		signature_check_clear(&sigc);

 		if (!tag_number++) {
 			fmt_tag_signature(&tagbuf, &sig, buf, len);
@@ -565,6 +564,7 @@ static void fmt_merge_msg_sigs(struct strbuf *out)
 		}
 		strbuf_release(&payload);
 		strbuf_release(&sig);
+		signature_check_clear(&sigc);
 	next:
 		free(origbuf);
 	}

--- >8 ---

Our coverage in t6200 (which should have ordinarily caught such a bug)
is lacking and does not search for the tag message in fmt-merge-msg's
output.

Thanks,
Taylor