[PATCH 9/9] sequencer: handle ignore_footer when parsing trailers

From: Jeff King <peff@peff.net>
Date: Aug 23, 2018, 00:51 UTC
Message-ID: <20180823005157.GI3126@sigill.intra.peff.net>
In-Reply-To: <20180823004300.GA1355@sigill.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>
---
I think this is probably worth doing, even if it's a noop. But I'm
really leaning towards the idea that the trailer code calling
ignore_non_trailer() is probably the wrong thing (in which case this
would be very important, since we'd be relying on the caller to remove
any cruft).

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

diff --git a/sequencer.c b/sequencer.c
index c01ff79ab0..a1f0f17bcd 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -231,11 +231,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.19.0.rc0.412.g7005db4e88