Re: [PATCH v10 4/6] notes.c: introduce '[--[no-]separator|--separator=<paragraph-break>]' option

From: Jeff King <peff@peff.net>
Date: May 19, 2023, 00:54 UTC
Message-ID: <20230519005447.GA2955320@coredump.intra.peff.net>
In-Reply-To: <820dda0458994fdf7ff37870736ce6ed7871720c.1684411136.git.dyroneteng@gmail.com>

On Thu, May 18, 2023 at 08:02:09PM +0800, Teng Long wrote:

Show 9 quoted lines

> +static void insert_separator(struct strbuf *message, size_t pos)
> +{
> +	if (!separator)
> +		return;
> +	else if (separator[strlen(separator) - 1] == '\n')
> +		strbuf_insertstr(message, pos, separator);
> +	else
> +		strbuf_insertf(message, pos, "%s%s", separator, "\n");
> +}

This function causes UBSan to complain on 'next' (though curiously only with clang, not with gcc[1]). The version in next seems to be from your v9, but it's largely the same except for the "if (!separator)" condition.

The problem is in the middle condition here. If "separator" is non-NULL, but is an empty string, then strlen() will return 0, and we will look at the out-of-bounds byte just before the string.

We'd probably want something like this:

diff --git a/builtin/notes.c b/builtin/notes.c
index 3215bce19b..a46d6dac5c 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -231,7 +231,8 @@ static void write_note_data(struct note_data *d, struct object_id *oid)
 
 static void insert_separator(struct strbuf *message, size_t pos)
 {
-	if (separator[strlen(separator) - 1] == '\n')
+	size_t sep_len = strlen(separator);
+	if (sep_len && separator[sep_len - 1] == '\n')
 		strbuf_addstr(message, separator);
 	else
 		strbuf_insertf(message, pos, "%s%s", separator, "\n");

to fix it, though I am not 100% clear on what is supposed to happen for
an empty separator here.

I was also confused that applying the fix on top of the culprit in
'next', 3993a53a13 (notes.c: introduce '--separator=<paragraph-break>'
option, 2023-04-28), still leads to test failures in t3301. But I think
that is independent of this fix. It fails even without my patch above
(and without UBSan) in test 66, "append: specify separator with line
break". But the failure goes away in the following patch, ad3d1f8feb
(notes.c: append separator instead of insert by pos, 2023-04-28).

I haven't been following this series enough to know what's going on, but
you may want to figure out where the failure is coming from in
3993a53a13. If the change in ad3d1f8feb is merely papering over it, then
we'd need to find and fix the true cause. If the bug is really fixed by
ad3d1f8feb, we might want to squash those two together to avoid broken
bisections.

-Peff

[1] To reproduce, I did:

      git checkout 3993a53a13
      make SANITIZE=address,undefined CC=clang
      cd t && ./t3301-notes.sh -v -i

    I'm using clang-14 on a Debian machine.