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

[Jonathan Nieder <jrnieder@gmail.com>]

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.