Re: [PATCH 03/23] builtin/describe: fix memory leak with `--contains=`

From: Karthik Nayak <karthik.188@gmail.com>
Date: Jul 30, 2024, 09:23 UTC
Message-ID: <CAOLa=ZTj6RxXH9t71q2fi-M+Xh-vOZnKzDk_mPmhNTEqVjkAzg@mail.gmail.com>
In-Reply-To: <08a12be13c2fed247d6086967e7a3f03fa6519e1.1721995576.git.ps@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

[snip]

Show 14 quoted lines

> @@ -637,7 +639,21 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
>  			strvec_pushv(&args, argv);
>  		else
>  			strvec_push(&args, "HEAD");
> -		return cmd_name_rev(args.nr, args.v, prefix);
> +
> +		/*
> +		 * `cmd_name_rev()` modifies the array, so we'd link its
> +		 * contained strings if we didn't do a copy here.
> +		 */
> +		ALLOC_ARRAY(argv_copy, args.nr + 1);
> +		for (size_t i = 0; i < args.nr; i++)
> +			argv_copy[i] = args.v[i];
> +		argv_copy[args.nr] = NULL;

Eventhough we pass `args.nr`, we still set the last element to NULL. This replicates what strvec does and makes it more robust. Nice.

Show 10 quoted lines

> +		ret = cmd_name_rev(args.nr, argv_copy, prefix);
> +
> +		strvec_clear(&args);
> +		free(argv_copy);
> +		return ret;
>  	}
>
>  	hashmap_init(&names, commit_name_neq, NULL, 0);
> --
> 2.46.0.rc1.dirty