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]); > } > > /**