Re: [PATCH v2 3/4] strbuf_setlen: don't write to strbuf_slopbuf
From: Junio C Hamano <hidden>
Date: 2017-08-23 17:24:30
Martin Ågren [off-list ref] 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 <redacted> Signed-off-by: Martin Ågren <redacted> --- 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.
quoted hunk
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]); } /**