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

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

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

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