Re: [PATCH] branch: reset instead of release a strbuf

From: Jeff King <peff@peff.net>
Date: Oct 3, 2017, 23:49 UTC
Message-ID: <20171003234919.qsr54ncmw6cihowi@sigill.intra.peff.net>
In-Reply-To: <20171003222414.GC19555@aiede.mtv.corp.google.com>

On Tue, Oct 03, 2017 at 03:24:14PM -0700, Jonathan Nieder wrote:

Show 12 quoted lines

> Here's a patch to address the surprising strbuf.h advice.
> 
> -- >8 --
> Subject: strbuf: do not encourage init-after-release
> 
> strbuf_release already leaves the strbuf in a valid, initialized
> state, so there is not need to call strbuf_init after it.
> 
> Moreover, this is not likely to change in the future: strbuf_release
> leaving the strbuf in a valid state has been easy to maintain and has
> been very helpful for Git's robustness and simplicity (e.g.,
> preventing use-after-free vulnerabilities).

Thanks for picking this up. Like you, I was quite surprised when I saw Stefan's original patch.

Show 12 quoted lines

> diff --git a/strbuf.h b/strbuf.h
> index 7496cb8ec5..6e175c3694 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -83,7 +83,7 @@ extern void strbuf_init(struct strbuf *, size_t);
>  
>  /**
>   * Release a string buffer and the memory it used. You should not use the
> - * string buffer after using this function, unless you initialize it again.
> + * string buffer after using this function.
>   */
>  extern void strbuf_release(struct strbuf *);

I think it's actually OK to use the string buffer after this function. It's just an empty string.

Perhaps we should be more explicit: this releases any resources and resets to a pristine, empty state. I suspect strbuf_detach() probably should make the same claim.

Earlier you mentioned:

> It is still not advisable to call strbuf_release until done using a
> strbuf because it is wasteful, so keep that part of the advice.

Is this what you meant? If so, I think we should probably be more explicit in giving people a hint to use strbuf_reset() for efficiency.

-Peff