Re: [PATCH v3 3/4] builtin/stash: provide a way to export stashes to a ref

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Apr 4, 2022, 06:46 UTC
Message-ID: <220404.86pmlxjppz.gmgdl@evledraar.gmail.com>
In-Reply-To: <20220403182250.904933-4-sandals@crustytoothpaste.net>

On Sun, Apr 03 2022, brian m. carlson wrote:

> [...snip...]

Per
https://lore.kernel.org/git/220404.86tub9jql5.gmgdl@evledraar.gmail.com/
there's a bug here, in the preceding commit you:
	
	+static int parse_revision(struct strbuf *revision, const char *commit, int quiet)
	+{
	+	strbuf_init(revision, 0);
	[...]
	 static int get_stash_info(struct stash_info *info, int argc, const char **argv)
	 {
	 	int ret;
	@@ -157,19 +175,9 @@ static int get_stash_info(struct stash_info *info, int argc, const char **argv)
	 	if (argc == 1)
	 		commit = argv[0];
	 
	-	strbuf_init(&info->revision, 0);
	-	if (!commit) {
	-		if (!ref_exists(ref_stash)) {
	[...]
	+	if (parse_revision(&info->revision, commit, 0)) {
	+		free_stash_info(info);
	+		return -1;
	 	}

That caller wasn't dealing with an init'd "info", so we init'd it late
and freed it (which my revisions_release() makes a lot less icky). But
unrelated to that:

Show 7 quoted lines

> +	if (argc) {
> +		/*
> +		 * Find each specified stash, and load data into the array.
> +		 */
> +		for (i = 0; i < argc; i++) {
> +			ALLOC_GROW_BY(items, nitems, 1, nalloc);
> +			if (parse_revision(&revision, argv[i], 0) ||

Here...

Show 19 quoted lines

> +			    get_oid_with_context(the_repository, revision.buf,
> +						 GET_OID_QUIETLY | GET_OID_GENTLY,
> +						 &items[i], &unused)) {
> +				error(_("unable to find stash entry %s"), argv[i]);
> +				res = -1;
> +				goto out;
> +			}
> +		}
> +	} else {
> +		/*
> +		 * Walk the reflog, finding each stash entry, and load data into the
> +		 * array.
> +		 */
> +		for (i = 0;; i++) {
> +			char buf[32];
> +			struct object_id oid;
> +
> +			snprintf(buf, sizeof(buf), "%d", i);
> +			if (parse_revision(&revision, buf, 1) ||

...and here you are leaking memory in a loop. Per the above you need a strbuf_reset() for both.

Then (moved from earlier):

Show 10 quoted lines

> +static int write_commit_with_parents(struct object_id *out, const struct object_id *oid, struct commit_list *parents)
> +{
> +	size_t author_len, committer_len;
> +	struct commit *this;
> +	const char *orig_author, *orig_committer;
> +	char *author = NULL, *committer = NULL;
> +	const char *buffer;
> +	unsigned long bufsize;
> +	const char *p;
> +	struct strbuf msg = STRBUF_INIT;

This is a leak, because:

> +out:
> +	strbuf_reset(&msg);

This should be strbuf_release(), not strbuf_reset().

Anyway, even if builtin/stash.c is currently a big fail whale it's still useful to test new code with SANITIZE=leak, e.g. (and most of this is a lot less painful on top of my in-flight topic) on top of yours you can try:

        printf "leak:%s\n" setup_revisions add_object_array_with_path \
        	add_rev_cmdline cmd_log_init_finish strvec_push \
                get_untracked_files pump_io_round setup_rename_conflict_info parse_pathspec \
                prep_parse_options init_reflog_walk >/tmp/supp.txt;
	LSAN_OPTIONS=print_suppressions=0:suppressions=/tmp/supp.txt ./t3903-stash.sh  -vixd

Where that printf is just something I came up with via trial & error by re-running the test with -vixd and quieting existing leaks, until getting to the first leak new in your topic.