Re: [PATCH v2 3/4] strbuf_setlen: don't write to strbuf_slopbuf

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 23, 2017, 17:24 UTC
Message-ID: <xmqq378i19ku.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <dccd3e75fcd1b2de93263e8373a3b4cd5da0dd32.1503323391.git.martin.agren@gmail.com>

Martin Ågren <martin.agren@gmail.com> writes:

Show 11 quoted lines

> 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@pobox.com>
> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---
> 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.

Show 19 quoted lines

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