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

[Junio C Hamano <gitster@pobox.com>]

Florian Schmidt <flosch@nutanix.com> writes:

> Currently, 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.

You do not have to say "Currently, if"; just "If" is sufficient.
Cf. Documentation/SubmittingPatches[[present-tense]]

> 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.

Good finding.  

> It seems sensible to expect that wt_status_locate_end() should honour
> the length parameter passed in, and doing so fixes this issue.

Thanks.  This is an ancient bug, not a retression from recent
changes to the trailer library [linusa CC'ed to save him from
wasting his time wondering if he broke anything].

> diff --git a/wt-status.c b/wt-status.c
> index b5a29083df..51a84575ed 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1089,14 +1089,19 @@ size_t wt_status_locate_end(const char *s, size_t len)
>  {
>  	const char *p;
>  	struct strbuf pattern = STRBUF_INIT;
> +	size_t result = 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;
> +		result = 0;
> +	else if ((p = strstr(s, pattern.buf))) {
> +		result = p - s + 1;
> +		if (result > len) {
> +			result = len;
> +		}
> +	}
>  	strbuf_release(&pattern);
> -	return len;
> +	return result;
>  }

Looks correct, but we probably can make the fix a lot more isolated
into a single block, like the attached patch.  How does this look?

 wt-status.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git c/wt-status.c w/wt-status.c
index b5a29083df..511f37cfe0 100644
--- c/wt-status.c
+++ w/wt-status.c
@@ -1093,8 +1093,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))) {
+		int newlen = p - s + 1;
+		if (newlen < len)
+			len = newlen;
+	}
 	strbuf_release(&pattern);
 	return len;
 }