Re: [PATCH] wt-status: Don't find scissors line beyond buf len

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 7, 2024, 20:47 UTC
Message-ID: <xmqq7cidlqg5.fsf@gitster.g>
In-Reply-To: <xmqq34t1n91w.fsf@gitster.g>

From: Florian Schmidt <flosch@nutanix.com>
Date: Thu, 7 Mar 2024 18:37:38 +0000
Subject: [PATCH] wt-status: don't find scissors line beyond buf len

If

  (a) There is a "---" divider in a commit message,

  (b) At some point beyond that divider, there is a cut-line (that is,
      "# ------------------------ >8 ------------------------") in the
      commit message,

  (c) the user does not explicitly set the "no-divider" option,

then "git interpret-trailers" will hang indefinitively.

This is because when (a) is true, find_end_of_log_message() will invoke ignored_log_message_bytes() with a len that is intended to make it ignore the part of the commit message beyond the divider. However, ignored_log_message_bytes() calls wt_status_locate_end(), and that function ignores the length restriction when it tries to locate the cut line. If it manages to find one, the returned cutoff value is greater than len. At this point, ignored_log_message_bytes() goes into an infinite loop, because it won't advance the string parsing beyond len, but the exit condition expects to reach cutoff.

Make wt_status_locate_end() honor the length parameter passed in, to fix this issue.

In general, if wt_status_locate_end() is given a piece of the memory that lacks NUL at all, strstr() may continue across page boundaries and run into an unmapped page. For our current callers, this is not a problem, as all of them except one uses a memory owned by a strbuf (which guarantees an implicit NUL-termination after its payload), and the one exeption in trailer.c:find_end_of_log_message() uses strlen() to compute the length before calling this function.

Signed-off-by: Florian Schmidt <flosch@nutanix.com>
Reviewed-by: Jonathan Davies <jonathan.davies@nutanix.com>
[jc: tweaked the commit log message and the implementation a bit]
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * So here is the version I queued.  I have a new paragraph at the
   end of the log message to talk about use of strstr() and how it
   is OK in the current codebase.

 t/t7513-interpret-trailers.sh | 14 ++++++++++++++
 wt-status.c                   |  7 +++++--
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 6602790b5f..5efe70d675 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -1476,4 +1476,18 @@ test_expect_success 'suppress --- handling' '
 	test_cmp expected actual
 '
 
+test_expect_success 'handling of --- lines in conjunction with cut-lines' '
+	echo "my-trailer: here" >expected &&
+
+	git interpret-trailers --parse >actual <<-\EOF &&
+	subject
+
+	my-trailer: here
+	---
+	# ------------------------ >8 ------------------------
+	EOF
+
+	test_cmp expected actual
+'
+
 test_done
diff --git a/wt-status.c b/wt-status.c
index 40b59be478..16c1b9b7ee 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1007,8 +1007,11 @@ size_t wt_status_locate_end(const char *s, size_t len)
 	strbuf_addf(&pattern, "\n%c %s", comment_line_char, cut_line);
 	if (starts_with(s, pattern.buf + 1))
 		len = 0;
-	else if ((p = strstr(s, pattern.buf)))
-		len = p - s + 1;
+	else if ((p = strstr(s, pattern.buf))) {
+		size_t newlen = p - s + 1;
+		if (newlen < len)
+			len = newlen;
+	}
 	strbuf_release(&pattern);
 	return len;
 }

-- 
2.44.0-117-g43072b4ca1