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@xxxxxxxxxx>
> ---
>
> 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@xxxxxxxxx>
Thanks.
