[PATCH 09/10] sequencer: handle ignore_footer when parsing trailers

From: Jeff King <peff@peff.net>
Date: Sep 30, 2020, 12:34 UTC
Message-ID: <20200930123411.GI1901279@coredump.intra.peff.net>
In-Reply-To: <20200930122732.GA1901036@coredump.intra.peff.net>

The append_signoff() function takes an "ignore_footer" argument, which specifies a number of bytes at the end of the message buffer which should not be considered (they cannot contain trailers, and the trailer is spliced in before them).

But to find the existing trailers, it calls into has_conforming_trailer(). That function takes an ignore_footer parameter, but since 967dfd4d56 (sequencer: use trailer's trailer layout, 2016-11-02) the parameter is completely ignored.

The trailer interface we're using takes a single string, with no option to tell it to use part of the string. However, since we have a mutable strbuf, we can work around this by simply overwriting (and later restoring) the boundary with a NUL.

I'm not sure if this can actually trigger a bug in practice. It's easy to get a non-zero ignore_footer by doing something like this:

  git commit -F - --cleanup=verbatim <<-EOF
  subject

  body

  Signed-off-by: me

  # this looks like a comment, but is actually in the
  # message! That makes the earlier s-o-b fake.
  EOF

  git commit --amend -s

There git-commit calls ignore_non_trailer() to count up the "#" cruft, which becomes the ignore_footer header. But it works even without this patch! That's because the trailer code _also_ calls ignore_non_trailer() and skips the cruft, too. So it happens to work because the only callers with a non-zero ignore_footer are using the exact same function that the trailer parser uses internally.

And that seems true for all of the current callers, but there's nothing guaranteeing it. We're better off only feeding the correct buffer to the trailer code in the first place.

Signed-off-by: Jeff King <peff@peff.net>
---
This was actually posted as part of an earlier series:

  https://lore.kernel.org/git/20180824072629.GA11977@sigill.intra.peff.net/

It ended as:

  I think patch 9 is not hurting anything and may later help us, but I
  could take or leave it.

and we left it. But now I have the ulterior motive of wanting to get rid of the unused parameter warning. :) Another option would be to leave the code as-is and just drop the ignored parameter. But I think this is probably a safe and reasonable patch (I've been running with it in my personal fork for 2 years, though perhaps I don't really exercise the trailer code's corner cases myself).

 sequencer.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/sequencer.c b/sequencer.c
index 6e9aabaac1..e454264fbc 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -249,11 +249,20 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
 	struct trailer_info info;
 	size_t i;
 	int found_sob = 0, found_sob_last = 0;
+	char saved_char;
 
 	opts.no_divider = 1;
 
+	if (ignore_footer) {
+		saved_char = sb->buf[sb->len - ignore_footer];
+		sb->buf[sb->len - ignore_footer] = '\0';
+	}
+
 	trailer_info_get(&info, sb->buf, &opts);
 
+	if (ignore_footer)
+		sb->buf[sb->len - ignore_footer] = saved_char;
+
 	if (info.trailer_start == info.trailer_end)
 		return 0;

-- 
2.28.0.1173.gad90222cf0