Martin Ågren <martin.agren@xxxxxxxxx> writes:
> strbuf_setlen(., 0) writes '\0' to sb.buf[0], where buf is either
> allocated and unique to sb, or the global slopbuf. The slopbuf is meant
> to provide a guarantee that buf is not NULL and that a freshly
> initialized buffer contains the empty string, but it is not supposed to
> be written to. That strbuf_setlen writes to slopbuf has at least two
> implications:
>
> First, it's wrong in principle. Second, it might be hiding misuses which
> are just waiting to wreak havoc. Third, ThreadSanitizer detects a race
> when multiple threads write to slopbuf at roughly the same time, thus
> potentially making any more critical races harder to spot.
There are two hard things in computer science ;-).
> Suggested-by: Junio C Hamano <gitster@xxxxxxxxx>
> Signed-off-by: Martin Ågren <martin.agren@xxxxxxxxx>
> ---
> v2: no "ifdef TSAN"; moved check from strbuf_reset into strbuf_setlen
Looks much better. I have a mild objection to "suggested-by",
though. It makes it sound as if this were my itch, but it is not.
All the credit for being motivate to fix the issue should go to you.
For what I did during the review of the previous one to lead to this
simpler version, if you want to document it, "helped-by" would be
more appropriate.
> strbuf.h | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/strbuf.h b/strbuf.h
> index e705b94db..7496cb8ec 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -147,7 +147,10 @@ static inline void strbuf_setlen(struct strbuf *sb, size_t len)
> if (len > (sb->alloc ? sb->alloc - 1 : 0))
> die("BUG: strbuf_setlen() beyond buffer");
> sb->len = len;
> - sb->buf[len] = '\0';
> + if (sb->buf != strbuf_slopbuf)
> + sb->buf[len] = '\0';
> + else
> + assert(!strbuf_slopbuf[0]);
> }
>
> /**
