Re: [PATCH] wt-status: Don't find scissors line beyond buf len
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 7, 2024, 19:20 UTC
- Message-ID
- <xmqq34t1n91w.fsf@gitster.g>
- In-Reply-To
- <20240307183743.219951-1-flosch@nutanix.com>
Florian Schmidt <flosch@nutanix.com> writes:
Show 7 quoted lines
> 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]]
Show 9 quoted lines
> 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].
Show 26 quoted lines
> 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; }