Re: [PATCH 11/15] find multi-byte comment chars in unterminated buffers

[Jeff King <peff@peff.net>]

On Thu, Mar 07, 2024 at 04:26:38AM -0500, Jeff King wrote:

> IMHO this is the trickiest commit of the whole series, as it would be
> easy to get the length computations subtly wrong.

And sure enough...

> diff --git a/trailer.c b/trailer.c
> index fe18faf6c5..f59c90b4b5 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -882,7 +882,7 @@ static size_t find_trailer_block_start(const char *buf, size_t len)
>  
>  	/* The first paragraph is the title and cannot be trailers */
>  	for (s = buf; s < buf + len; s = next_line(s)) {
> -		if (s[0] == comment_line_char)
> +		if (starts_with_mem(s, buf + len - s, comment_line_str))
>  			continue;
>  		if (is_blank_line(s))
>  			break;
> @@ -902,7 +902,7 @@ static size_t find_trailer_block_start(const char *buf, size_t len)
>  		const char **p;
>  		ssize_t separator_pos;
>  
> -		if (bol[0] == comment_line_char) {
> +		if (starts_with_mem(bol, buf + end_of_title - bol, comment_line_str)) {
>  			non_trailer_lines += possible_continuation_lines;
>  			possible_continuation_lines = 0;
>  			continue;

This second hunk needs:

diff --git a/trailer.c b/trailer.c
index f59c90b4b5..fdb0b8137e 100644
--- a/trailer.c
+++ b/trailer.c
@@ -902,7 +902,7 @@ static size_t find_trailer_block_start(const char *buf, size_t len)
 		const char **p;
 		ssize_t separator_pos;
 
-		if (starts_with_mem(bol, buf + end_of_title - bol, comment_line_str)) {
+		if (starts_with_mem(bol, buf + len - bol, comment_line_str)) {
 			non_trailer_lines += possible_continuation_lines;
 			possible_continuation_lines = 0;
 			continue;

I was trying to bound the size based on the loop, which is:

          for (l = last_line(buf, len);
               l >= end_of_title;
               l = last_line(buf, l)) {
                  const char *bol = buf + l;

but I misread "end_of_title" as an upper bound, not a lower one. Which
makes sense because we're iterating backwards over the lines. So I
suppose we could bound it by the previous "bol" value. But in practice,
your prefix won't cross such a boundary anyway, as it won't have a
newline in it (maybe that's something we should enforce? I guess you
could set core.commentChar to '\n' even before my series, which would be
slightly insane).

So just bounding ourselves to "buf + len" seems reasonable, as that
makes sure we don't step outside the buffer passed into the function.

Curiously, this was found by the sanitizer job in CI, where UBSan
complains of integer overflow in the pointer computation. I had run with
both ASan/UBSan locally, but just using gcc, which doesn't seem to find
it (the CI job uses clang). So I'll that to my mental tally of "clang
seems to be better with sanitizers".

-Peff