On Thu, May 18, 2023 at 08:02:09PM +0800, Teng Long wrote:
> +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.
