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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Oct 3, 2017, 21:46 UTC
Message-ID: <20171003214646.GZ19555@aiede.mtv.corp.google.com>
In-Reply-To: <20171003211409.16037-1-sbeller@google.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?

Show 6 quoted lines

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

Show 15 quoted lines

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