Re: [PATCH] branch: reset instead of release a strbuf
- Date: Tue, 3 Oct 2017 19:49:19 -0400
- From: Jeff King <peff@xxxxxxxx>
- Subject: Re: [PATCH] branch: reset instead of release a strbuf
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.