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

Commit Message

Jeff King Sept. 30, 2020, 12:34 p.m. UTC

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 <[e-mail]>
---
This was actually posted as part of an earlier series:

  https://lore.kernel.org/git/[e-mail]/

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(+)

Patch

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;