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

[Jeff King <peff@peff.net>]

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

> 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.

> 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