[PATCH] branch: reset instead of release a strbuf

[PATCH] branch: reset instead of release a strbuf

[Stefan Beller]

Our documentation advises to not re-use a strbuf, after strbuf_release
has been called on it. Use the proper reset instead.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

Maybe one of the #leftoverbits is to remove the re-init call in release
and see what breaks? (And then fixing up more of such cases as presented
in this patch)

Thanks,
Stefan

 builtin/branch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index b998e16d0c..9758012390 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -217,7 +217,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 		if (!head_rev)
 			die(_("Couldn't look up commit object for HEAD"));
 	}
-	for (i = 0; i < argc; i++, strbuf_release(&bname)) {
+	for (i = 0; i < argc; i++, strbuf_reset(&bname)) {
 		char *target = NULL;
 		int flags = 0;
 
-- 
2.14.0.rc0.3.g6c2e499285

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

[Jonathan Nieder]

Hi,

Stefan Beller wrote:

> Our documentation advises to not re-use a strbuf, after strbuf_release
> has been called on it. Use the proper reset instead.

I'm super surprised at this documentation.  strbuf_release maintains
the invariant that a strbuf is always usable (i.e., that we do not have
to fear use-after-free problems).

On second thought, though, strbuf_release is a more expensive operation
than strbuf_reset --- constantly free()-ing and re-malloc()ing is
unnecessary churn in malloc data structures.  So maybe that is the
motivation here?

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
> Maybe one of the #leftoverbits is to remove the re-init call in release
> and see what breaks? (And then fixing up more of such cases as presented
> in this patch)

As mentioned above: please no.  That would be problematic both for
ease of development and for the risk of security bugs.

>  builtin/branch.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index b998e16d0c..9758012390 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -217,7 +217,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
>  		if (!head_rev)
>  			die(_("Couldn't look up commit object for HEAD"));
>  	}
> -	for (i = 0; i < argc; i++, strbuf_release(&bname)) {
> +	for (i = 0; i < argc; i++, strbuf_reset(&bname)) {
>  		char *target = NULL;
>  		int flags = 0;

Should there be a strbuf_release (or UNLEAK if you are very very sure)
call at the end of the function to replace this?

With that change (but not without it),
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

[PATCH] branch: reset instead of release a strbuf

[Stefan Beller]

Our documentation advises to not re-use a strbuf, after strbuf_release
has been called on it. Use the proper reset instead.

Currently 'strbuf_release' releases and re-initializes the strbuf, so it
is safe, but slow. 'strbuf_reset' only resets the internal length variable,
such that this could also be accounted for as a micro-optimization.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/branch.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index b998e16d0c..71ed1c7036 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -217,7 +217,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 		if (!head_rev)
 			die(_("Couldn't look up commit object for HEAD"));
 	}
-	for (i = 0; i < argc; i++, strbuf_release(&bname)) {
+	for (i = 0; i < argc; i++, strbuf_reset(&bname)) {
 		char *target = NULL;
 		int flags = 0;
 
@@ -282,8 +282,9 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 	}
 
 	free(name);
+	strbuf_release(&bname);
 
-	return(ret);
+	return ret;
 }
 
 static int calc_maxwidth(struct ref_array *refs, int remote_bonus)
-- 
2.14.0.rc0.3.g6c2e499285

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

[Jonathan Nieder]

Hi,

Stefan Beller wrote:

> Our documentation advises to not re-use a strbuf, after strbuf_release
> has been called on it. Use the proper reset instead.
>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

This is indeed
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thank you.

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  builtin/branch.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

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

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.

Reported-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 strbuf.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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 *);
 
-- 
2.14.2.920.gcf0c67979c

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

[Jeff King]

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

Yes, "should not use" above is simply misleading.  Either drop it
altogether, or say something like

	If you find yourself reusing the same strbuf in a loop and
	calling strbuf_release() each iteration, you may want to
	consider if it makes more sense to use strbuf_reset()
	instead in each iteration and calling strbuf_release() at
	the end.

perhaps.

[PATCH v2] strbuf doc: reuse after strbuf_release is fine

[Jonathan Nieder]

strbuf_release leaves the strbuf in a valid, initialized state, so
there is no 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).

Document the semantics so the next generation of Git developers can
become familiar with them without reading the implementation.  It is
still not advisable to call strbuf_release too often because it is
wasteful, so add a note pointing to strbuf_reset for that.

The same semantics apply to strbuf_detach.  Add a similar note to its
docstring to make that clear.

Improved-by: Jeff King <peff@peff.net>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Jeff King wrote:

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

Like this?

Thanks,
Jonathan

 strbuf.h | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/strbuf.h b/strbuf.h
index 7496cb8ec5..249df86711 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -82,8 +82,12 @@ extern char strbuf_slopbuf[];
 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.
+ * Release a string buffer and the memory it used. After this call, the
+ * strbuf points to an empty string that does not need to be free()ed, as
+ * if it had been set to `STRBUF_INIT` and never modified.
+ *
+ * To clear a strbuf in preparation for further use without the overhead
+ * of free()ing and malloc()ing again, use strbuf_reset() instead.
  */
 extern void strbuf_release(struct strbuf *);
 
@@ -91,6 +95,9 @@ extern void strbuf_release(struct strbuf *);
  * Detach the string from the strbuf and returns it; you now own the
  * storage the string occupies and it is your responsibility from then on
  * to release it with `free(3)` when you are done with it.
+ *
+ * The strbuf that previously held the string is reset to `STRBUF_INIT` so
+ * it can be reused after calling this function.
  */
 extern char *strbuf_detach(struct strbuf *, size_t *);
 
-- 
2.14.2.920.gcf0c67979c

Re: [PATCH v2] strbuf doc: reuse after strbuf_release is fine

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> strbuf_release leaves the strbuf in a valid, initialized state, so
> there is no 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).
>
> Document the semantics so the next generation of Git developers can
> become familiar with them without reading the implementation.  It is
> still not advisable to call strbuf_release too often because it is
> wasteful, so add a note pointing to strbuf_reset for that.
>
> The same semantics apply to strbuf_detach.  Add a similar note to its
> docstring to make that clear.
>
> Improved-by: Jeff King <peff@peff.net>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> Jeff King wrote:
>
>> 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.
>
> Like this?

Looks good to me.


>
> Thanks,
> Jonathan
>
>  strbuf.h | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/strbuf.h b/strbuf.h
> index 7496cb8ec5..249df86711 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -82,8 +82,12 @@ extern char strbuf_slopbuf[];
>  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.
> + * Release a string buffer and the memory it used. After this call, the
> + * strbuf points to an empty string that does not need to be free()ed, as
> + * if it had been set to `STRBUF_INIT` and never modified.
> + *
> + * To clear a strbuf in preparation for further use without the overhead
> + * of free()ing and malloc()ing again, use strbuf_reset() instead.
>   */
>  extern void strbuf_release(struct strbuf *);
>  
> @@ -91,6 +95,9 @@ extern void strbuf_release(struct strbuf *);
>   * Detach the string from the strbuf and returns it; you now own the
>   * storage the string occupies and it is your responsibility from then on
>   * to release it with `free(3)` when you are done with it.
> + *
> + * The strbuf that previously held the string is reset to `STRBUF_INIT` so
> + * it can be reused after calling this function.
>   */
>  extern char *strbuf_detach(struct strbuf *, size_t *);

Re: [PATCH v2] strbuf doc: reuse after strbuf_release is fine

[Jeff King]

On Tue, Oct 03, 2017 at 07:39:54PM -0700, Jonathan Nieder wrote:

> > 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.
> 
> Like this?

Yes, perfect. Thanks!

-Peff