[PATCH v2 0/9] midx: clean up t5319 under 'SANITIZE=leak'

[PATCH v2 0/9] midx: clean up t5319 under 'SANITIZE=leak'

[Taylor Blau]

Here is a small-ish update to my series which makes t5319 leak-free (i.e., it
passes even when Git is compiled with SANITIZE=leak). It is based on
ab/only-single-progress-at-once (so I dropped the cherry-picked patch towards
the end from Ævar).

Most of the fixes are straightforward from review, but the biggest change is in
7/10, which converts get_midx_filename() to write its result to a strbuf per
Junio's helpful suggestion.

Thinking ahead, there are a few things I noted while reading through replies for
future improvements to the MIDX and pack bitmap code. From my ~/notes, they are:

  - load_multi_pack_index() should not die when missing the packfile names chunk,
    instead should pretend that no MIDX exists (maybe warn?)
  - double-close when reaching the cleanup_fail codepath after xmmap returns
  - static `verify_midx_error` is ugly
  - load_bitmap() does not clean up after itself very well (e.g.,
    bitmap_git->trees and others)

I'm pretty happy with the state of this topic, and I'll plan on taking on the
above in separate series in the future.

Taylor Blau (9):
  midx.c: clean up chunkfile after reading the MIDX
  midx.c: don't leak MIDX from verify_midx_file
  t/helper/test-read-midx.c: free MIDX within read_midx_file()
  builtin/pack-objects.c: don't leak memory via arguments
  builtin/repack.c: avoid leaking child arguments
  builtin/multi-pack-index.c: don't leak concatenated options
  midx.c: write MIDX filenames to strbuf
  pack-bitmap.c: don't leak type-level bitmaps
  pack-bitmap.c: more aggressively free in free_bitmap_index()

 builtin/multi-pack-index.c |  4 +++
 builtin/pack-objects.c     | 11 ++++---
 builtin/repack.c           | 23 +++++++++----
 midx.c                     | 66 ++++++++++++++++++++++----------------
 midx.h                     |  4 +--
 pack-bitmap.c              | 29 ++++++++++++++---
 pack-revindex.c            |  8 ++---
 t/helper/test-read-midx.c  |  3 +-
 8 files changed, 97 insertions(+), 51 deletions(-)

Range-diff against v1:
 1:  30f6f23daf =  1:  dcc5998072 midx.c: clean up chunkfile after reading the MIDX
 2:  b0c79904ab !  2:  258a9e2e57 midx.c: don't leak MIDX from verify_midx_file
    @@ Metadata
      ## Commit message ##
         midx.c: don't leak MIDX from verify_midx_file
     
    -    The function midx.c:verify_midx_file() allocate a MIDX struct by calling
    -    load_multi_pack_index(). But when cleaning up, it calls free() without
    -    freeing any resources associated with the MIDX.
    +    The function midx.c:verify_midx_file() allocates a MIDX struct by
    +    calling load_multi_pack_index(). But when cleaning up, it calls free()
    +    without freeing any resources associated with the MIDX.
     
         Call the more appropriate close_midx() which does free those resources,
         which causes t5319.3 to pass when Git is compiled with SANITIZE=leak.
 3:  5157edb41e =  3:  84859d5b53 t/helper/test-read-midx.c: free MIDX within read_midx_file()
 4:  dd3b9a949e =  4:  aedb1713b4 builtin/pack-objects.c: don't leak memory via arguments
 5:  a68c77c006 !  5:  bcd12ecab8 builtin/repack.c: avoid leaking child arguments
    @@ Commit message
         In none of these cases do we bother to call child_process_clear(), which
         frees the memory associated with each child's arguments and environment.
     
    -    In order to do so, tweak each function that spawns a child process to
    -    have a `cleanup` label that we always visit before returning from each
    -    function. Then, make sure that we call child_process_clear() as a part
    -    of that label.
    +    Make sure that we call child_process_clear() in any functions which
    +    initialize a struct child_process before returning along a path which
    +    did not call finish_command().
    +
    +    In cmd_repack(), take a slightly different approach to use a cleanup
    +    label to clear the child_process, unless finish_command() was called.
    +    This allows us to free other memory allocated during the lifetime of
    +    that function. But it avoids calling child_process_clear() twice (the
    +    other call coming from inside of finish_command()) to avoid assuming the
    +    function's implementation is idempotent.
     
         Signed-off-by: Taylor Blau <me@ttaylorr.com>
     
      ## builtin/repack.c ##
     @@ builtin/repack.c: static void repack_promisor_objects(const struct pack_objects_args *args,
    - 	struct child_process cmd = CHILD_PROCESS_INIT;
    - 	FILE *out;
    - 	struct strbuf line = STRBUF_INIT;
    -+	int ret = 0;
    + 	for_each_packed_object(write_oid, &cmd,
    + 			       FOR_EACH_OBJECT_PROMISOR_ONLY);
      
    - 	prepare_pack_objects(&cmd, args);
    - 	cmd.in = -1;
    -@@ builtin/repack.c: static void repack_promisor_objects(const struct pack_objects_args *args,
    - 
    - 	if (cmd.in == -1)
    +-	if (cmd.in == -1)
    ++	if (cmd.in == -1) {
      		/* No packed objects; cmd was never started */
    --		return;
    -+		goto cleanup;
    ++		child_process_clear(&cmd);
    + 		return;
    ++	}
      
      	close(cmd.in);
      
    -@@ builtin/repack.c: static void repack_promisor_objects(const struct pack_objects_args *args,
    - 		free(promisor_name);
    - 	}
    - 	fclose(out);
    --	if (finish_command(&cmd))
    -+	ret = finish_command(&cmd);
    -+
    -+cleanup:
    -+	child_process_clear(&cmd);
    -+
    -+	if (ret)
    - 		die(_("could not finish pack-objects to repack promisor objects"));
    - }
    - 
    -@@ builtin/repack.c: static int write_midx_included_packs(struct string_list *include,
    - 	struct string_list_item *item;
    - 	struct packed_git *largest = get_largest_active_pack(geometry);
    - 	FILE *in;
    --	int ret;
    -+	int ret = 0;
    - 
    - 	if (!include->nr)
    --		return 0;
    -+		goto cleanup;
    - 
    - 	cmd.in = -1;
    - 	cmd.git_cmd = 1;
     @@ builtin/repack.c: static int write_midx_included_packs(struct string_list *include,
    + 		strvec_pushf(&cmd.args, "--refs-snapshot=%s", refs_snapshot);
      
      	ret = start_command(&cmd);
    - 	if (ret)
    --		return ret;
    -+		goto cleanup;
    +-	if (ret)
    ++	if (ret) {
    ++		child_process_clear(&cmd);
    + 		return ret;
    ++	}
      
      	in = xfdopen(cmd.in, "w");
      	for_each_string_list_item(item, include)
    - 		fprintf(in, "%s\n", item->string);
    - 	fclose(in);
    - 
    --	return finish_command(&cmd);
    -+	ret = finish_command(&cmd);
    -+
    -+cleanup:
    -+	child_process_clear(&cmd);
    -+
    -+	return ret;
    - }
    - 
    - int cmd_repack(int argc, const char **argv, const char *prefix)
     @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix)
      	struct pack_geometry *geometry = NULL;
      	struct strbuf line = STRBUF_INIT;
    @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix
     +	int i, ext, ret = 0;
      	FILE *out;
      	int show_progress = isatty(2);
    ++	int cmd_cleared = 0;
      
    + 	/* variables to be filled by option parsing */
    + 	int pack_everything = 0;
     @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix)
      
      	ret = start_command(&cmd);
    @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix
      	if (geometry) {
      		FILE *in = xfdopen(cmd.in, "w");
     @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix)
    + 	}
      	fclose(out);
      	ret = finish_command(&cmd);
    ++	cmd_cleared = 1;
      	if (ret)
     -		return ret;
     +		goto cleanup;
    @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix
      	string_list_clear(&existing_kept_packs, 0);
      	clear_pack_geometry(geometry);
      	strbuf_release(&line);
    -+	child_process_clear(&cmd);
    ++	if (!cmd_cleared)
    ++		child_process_clear(&cmd);
      
     -	return 0;
     +	return ret;
 6:  ffded80c7d =  6:  0d252cd323 builtin/multi-pack-index.c: don't leak concatenated options
 7:  f3897c3afc <  -:  ---------- pack-bitmap.c: avoid leaking via midx_bitmap_filename()
 -:  ---------- >  7:  0f293ab638 midx.c: write MIDX filenames to strbuf
 8:  29920e7735 =  8:  77a4454632 pack-bitmap.c: don't leak type-level bitmaps
 9:  e65ac7deb5 !  9:  c1e7e6cc92 pack-bitmap.c: more aggressively free in free_bitmap_index()
    @@ Commit message
         The function free_bitmap_index() is somewhat lax in what it frees. There
         are two notable examples:
     
    -      - While it does call kh_destroy_oid_map on the "bitmaps" map (which
    -        maps) commit OIDs to their corresponding bitmaps, the bitmaps
    +      - While it does call kh_destroy_oid_map on the "bitmaps" map, which
    +        maps commit OIDs to their corresponding bitmaps, the bitmaps
             themselves are not freed. Note here that we recycle already-freed
             ewah_bitmaps into a pool, but these are handled correctly by
             ewah_pool_free().
10:  cb30aa67c0 <  -:  ---------- pack-bitmap-write.c: don't return without stop_progress()
11:  f1bb8b73ff <  -:  ---------- t5319: UNLEAK() the remaining leaks
-- 
2.33.0.96.g73915697e6

[PATCH v2 1/9] midx.c: clean up chunkfile after reading the MIDX

[Taylor Blau]

In order to read the contents of a MIDX, we initialize a chunkfile
structure which can read the table of contents and assign pointers into
different sections of the file for us.

We do call free(), since the chunkfile struct is heap allocated, but not
the more appropriate free_chunkfile(), which also frees memory that the
structure itself owns.

Call that instead to avoid leaking memory in this function.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 midx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/midx.c b/midx.c
index 8433086ac1..36e4754767 100644
--- a/midx.c
+++ b/midx.c
@@ -179,12 +179,13 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local
 	trace2_data_intmax("midx", the_repository, "load/num_packs", m->num_packs);
 	trace2_data_intmax("midx", the_repository, "load/num_objects", m->num_objects);
 
+	free_chunkfile(cf);
 	return m;
 
 cleanup_fail:
 	free(m);
 	free(midx_name);
-	free(cf);
+	free_chunkfile(cf);
 	if (midx_map)
 		munmap(midx_map, midx_size);
 	if (0 <= fd)
-- 
2.33.0.96.g73915697e6

[PATCH v2 2/9] midx.c: don't leak MIDX from verify_midx_file

[Taylor Blau]

The function midx.c:verify_midx_file() allocates a MIDX struct by
calling load_multi_pack_index(). But when cleaning up, it calls free()
without freeing any resources associated with the MIDX.

Call the more appropriate close_midx() which does free those resources,
which causes t5319.3 to pass when Git is compiled with SANITIZE=leak.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 midx.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/midx.c b/midx.c
index 36e4754767..ad60e48468 100644
--- a/midx.c
+++ b/midx.c
@@ -1611,7 +1611,7 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag
 		 * Remaining tests assume that we have objects, so we can
 		 * return here.
 		 */
-		return verify_midx_error;
+		goto cleanup;
 	}
 
 	if (flags & MIDX_PROGRESS)
@@ -1689,7 +1689,9 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag
 	}
 	stop_progress(&progress);
 
+cleanup:
 	free(pairs);
+	close_midx(m);
 
 	return verify_midx_error;
 }
-- 
2.33.0.96.g73915697e6

[PATCH v2 3/9] t/helper/test-read-midx.c: free MIDX within read_midx_file()

[Taylor Blau]

When calling `read_midx_file()` to show information about a MIDX or list
the objects contained within it we fail to call `close_midx()`, leaking
the memory allocated to store that MIDX.

Fix this by calling `close_midx()` before exiting the function. We can
drop the "early" return when `show_objects` is non-zero, since the next
instruction is also a return.

(We could just as easily put a `cleanup` label here as with previous
patches. But the only other time we terminate the function early is
when we fail to load a MIDX in the first place. `close_midx()` does
handle a NULL argument, but the extra complexity is likely not
warranted).

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/helper/test-read-midx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/helper/test-read-midx.c b/t/helper/test-read-midx.c
index 9d6fa7a377..27072ba94d 100644
--- a/t/helper/test-read-midx.c
+++ b/t/helper/test-read-midx.c
@@ -55,9 +55,10 @@ static int read_midx_file(const char *object_dir, int show_objects)
 			printf("%s %"PRIu64"\t%s\n",
 			       oid_to_hex(&oid), e.offset, e.p->pack_name);
 		}
-		return 0;
 	}
 
+	close_midx(m);
+
 	return 0;
 }
 
-- 
2.33.0.96.g73915697e6

[PATCH v2 4/9] builtin/pack-objects.c: don't leak memory via arguments

[Taylor Blau]

When constructing arguments to pass to setup_revision(), pack-objects
only frees the memory used by that array after calling
get_object_list().

Ensure that we call strvec_clear() whether or not we use the arguments
array by cleaning up whenever we exit the function (and rewriting one
early return to jump to a label which frees the memory and then
returns).

We could avoid setting this array up altogether unless we are in the
if-else block that calls get_object_list(), but setting up the argument
array is intermingled with lots of other side-effects, e.g.:

    if (exclude_promisor_objects) {
      use_internal_rev_list = 1;
      fetch_if_missing = 0;
      strvec_push(&rp, "--exclude-promisor-objects");
    }

So it would be awkward to check exclude_promisor_objects twice: first to
set use_internal_rev_list and fetch_if_missing, and then again above
get_object_list() to push the relevant argument onto the array.

Instead, leave the array's construction alone and make sure to free it
unconditionally.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 builtin/pack-objects.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 1a3dd445f8..857be7826f 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -4148,11 +4148,10 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 		read_packs_list_from_stdin();
 		if (rev_list_unpacked)
 			add_unreachable_loose_objects();
-	} else if (!use_internal_rev_list)
+	} else if (!use_internal_rev_list) {
 		read_object_list_from_stdin();
-	else {
+	} else {
 		get_object_list(rp.nr, rp.v);
-		strvec_clear(&rp);
 	}
 	cleanup_preferred_base();
 	if (include_tag && nr_result)
@@ -4162,7 +4161,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 			    the_repository);
 
 	if (non_empty && !nr_result)
-		return 0;
+		goto cleanup;
 	if (nr_result) {
 		trace2_region_enter("pack-objects", "prepare-pack",
 				    the_repository);
@@ -4183,5 +4182,9 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 			     " pack-reused %"PRIu32),
 			   written, written_delta, reused, reused_delta,
 			   reuse_packfile_objects);
+
+cleanup:
+	strvec_clear(&rp);
+
 	return 0;
 }
-- 
2.33.0.96.g73915697e6

[PATCH v2 5/9] builtin/repack.c: avoid leaking child arguments

[Taylor Blau]

`git repack` invokes a handful of child processes: one to write the
actual pack, and optionally ones to repack promisor objects and update
the MIDX.

In none of these cases do we bother to call child_process_clear(), which
frees the memory associated with each child's arguments and environment.

Make sure that we call child_process_clear() in any functions which
initialize a struct child_process before returning along a path which
did not call finish_command().

In cmd_repack(), take a slightly different approach to use a cleanup
label to clear the child_process, unless finish_command() was called.
This allows us to free other memory allocated during the lifetime of
that function. But it avoids calling child_process_clear() twice (the
other call coming from inside of finish_command()) to avoid assuming the
function's implementation is idempotent.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 builtin/repack.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index 0b2d1e5d82..b82f6b485c 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -258,9 +258,11 @@ static void repack_promisor_objects(const struct pack_objects_args *args,
 	for_each_packed_object(write_oid, &cmd,
 			       FOR_EACH_OBJECT_PROMISOR_ONLY);
 
-	if (cmd.in == -1)
+	if (cmd.in == -1) {
 		/* No packed objects; cmd was never started */
+		child_process_clear(&cmd);
 		return;
+	}
 
 	close(cmd.in);
 
@@ -586,8 +588,10 @@ static int write_midx_included_packs(struct string_list *include,
 		strvec_pushf(&cmd.args, "--refs-snapshot=%s", refs_snapshot);
 
 	ret = start_command(&cmd);
-	if (ret)
+	if (ret) {
+		child_process_clear(&cmd);
 		return ret;
+	}
 
 	in = xfdopen(cmd.in, "w");
 	for_each_string_list_item(item, include)
@@ -608,9 +612,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	struct pack_geometry *geometry = NULL;
 	struct strbuf line = STRBUF_INIT;
 	struct tempfile *refs_snapshot = NULL;
-	int i, ext, ret;
+	int i, ext, ret = 0;
 	FILE *out;
 	int show_progress = isatty(2);
+	int cmd_cleared = 0;
 
 	/* variables to be filled by option parsing */
 	int pack_everything = 0;
@@ -794,7 +799,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 
 	ret = start_command(&cmd);
 	if (ret)
-		return ret;
+		goto cleanup;
 
 	if (geometry) {
 		FILE *in = xfdopen(cmd.in, "w");
@@ -818,8 +823,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	}
 	fclose(out);
 	ret = finish_command(&cmd);
+	cmd_cleared = 1;
 	if (ret)
-		return ret;
+		goto cleanup;
 
 	if (!names.nr && !po_args.quiet)
 		printf_ln(_("Nothing new to pack."));
@@ -893,7 +899,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 		string_list_clear(&include, 0);
 
 		if (ret)
-			return ret;
+			goto cleanup;
 	}
 
 	reprepare_packed_git(the_repository);
@@ -946,12 +952,15 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 		write_midx_file(get_object_directory(), NULL, NULL, flags);
 	}
 
+cleanup:
 	string_list_clear(&names, 0);
 	string_list_clear(&rollback, 0);
 	string_list_clear(&existing_nonkept_packs, 0);
 	string_list_clear(&existing_kept_packs, 0);
 	clear_pack_geometry(geometry);
 	strbuf_release(&line);
+	if (!cmd_cleared)
+		child_process_clear(&cmd);
 
-	return 0;
+	return ret;
 }
-- 
2.33.0.96.g73915697e6

[PATCH v2 6/9] builtin/multi-pack-index.c: don't leak concatenated options

[Taylor Blau]

The `multi-pack-index` builtin dynamically allocates an array of
command-line option for each of its separate modes by calling
add_common_options() to concatante the common options with sub-command
specific ones.

Because this operation allocates a new array, we have to be careful to
remember to free it. We already do this in the repack and write
sub-commands, but verify and expire don't. Rectify this by calling
FREE_AND_NULL as the other modes do.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 builtin/multi-pack-index.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
index 075d15d706..4480ba3982 100644
--- a/builtin/multi-pack-index.c
+++ b/builtin/multi-pack-index.c
@@ -167,6 +167,8 @@ static int cmd_multi_pack_index_verify(int argc, const char **argv)
 		usage_with_options(builtin_multi_pack_index_verify_usage,
 				   options);
 
+	FREE_AND_NULL(options);
+
 	return verify_midx_file(the_repository, opts.object_dir, opts.flags);
 }
 
@@ -191,6 +193,8 @@ static int cmd_multi_pack_index_expire(int argc, const char **argv)
 		usage_with_options(builtin_multi_pack_index_expire_usage,
 				   options);
 
+	FREE_AND_NULL(options);
+
 	return expire_midx_packs(the_repository, opts.object_dir, opts.flags);
 }
 
-- 
2.33.0.96.g73915697e6

[PATCH v2 7/9] midx.c: write MIDX filenames to strbuf

[Taylor Blau]

To ask for the name of a MIDX and its corresponding .rev file, callers
invoke get_midx_filename() and get_midx_rev_filename(), respectively.
These both invoke xstrfmt(), allocating a chunk of memory which must be
freed later on.

This makes callers in pack-bitmap.c somewhat awkward. Specifically,
midx_bitmap_filename(), which is implemented like:

    return xstrfmt("%s-%s.bitmap",
                   get_midx_filename(midx->object_dir),
                   hash_to_hex(get_midx_checksum(midx)));

this leaks the second argument to xstrfmt(), which itself was allocated
with xstrfmt(). This caller could assign both the result of
get_midx_filename() and the outer xstrfmt() to a temporary variable,
remembering to free() the former before returning. But that involves a
wasteful copy.

Instead, get_midx_filename() and get_midx_rev_filename() take a strbuf
as an output parameter. This way midx_bitmap_filename() can manipulate
and pass around a temporary buffer which it detaches back to its caller.

That allows us to implement the function without copying or open-coding
get_midx_filename() in a way that doesn't leak.

Update the other callers of get_midx_filename() and
get_midx_rev_filename() accordingly.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 midx.c          | 59 +++++++++++++++++++++++++++----------------------
 midx.h          |  4 ++--
 pack-bitmap.c   | 15 ++++++++-----
 pack-revindex.c |  8 +++----
 4 files changed, 49 insertions(+), 37 deletions(-)

diff --git a/midx.c b/midx.c
index ad60e48468..837b46b2af 100644
--- a/midx.c
+++ b/midx.c
@@ -57,15 +57,15 @@ const unsigned char *get_midx_checksum(struct multi_pack_index *m)
 	return m->data + m->data_len - the_hash_algo->rawsz;
 }
 
-char *get_midx_filename(const char *object_dir)
+void get_midx_filename(struct strbuf *out, const char *object_dir)
 {
-	return xstrfmt("%s/pack/multi-pack-index", object_dir);
+	strbuf_addf(out, "%s/pack/multi-pack-index", object_dir);
 }
 
-char *get_midx_rev_filename(struct multi_pack_index *m)
+void get_midx_rev_filename(struct strbuf *out, struct multi_pack_index *m)
 {
-	return xstrfmt("%s/pack/multi-pack-index-%s.rev",
-		       m->object_dir, hash_to_hex(get_midx_checksum(m)));
+	get_midx_filename(out, m->object_dir);
+	strbuf_addf(out, "-%s.rev", hash_to_hex(get_midx_checksum(m)));
 }
 
 static int midx_read_oid_fanout(const unsigned char *chunk_start,
@@ -89,28 +89,30 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local
 	size_t midx_size;
 	void *midx_map = NULL;
 	uint32_t hash_version;
-	char *midx_name = get_midx_filename(object_dir);
+	struct strbuf midx_name = STRBUF_INIT;
 	uint32_t i;
 	const char *cur_pack_name;
 	struct chunkfile *cf = NULL;
 
-	fd = git_open(midx_name);
+	get_midx_filename(&midx_name, object_dir);
+
+	fd = git_open(midx_name.buf);
 
 	if (fd < 0)
 		goto cleanup_fail;
 	if (fstat(fd, &st)) {
-		error_errno(_("failed to read %s"), midx_name);
+		error_errno(_("failed to read %s"), midx_name.buf);
 		goto cleanup_fail;
 	}
 
 	midx_size = xsize_t(st.st_size);
 
 	if (midx_size < MIDX_MIN_SIZE) {
-		error(_("multi-pack-index file %s is too small"), midx_name);
+		error(_("multi-pack-index file %s is too small"), midx_name.buf);
 		goto cleanup_fail;
 	}
 
-	FREE_AND_NULL(midx_name);
+	strbuf_release(&midx_name);
 
 	midx_map = xmmap(NULL, midx_size, PROT_READ, MAP_PRIVATE, fd, 0);
 	close(fd);
@@ -184,7 +186,7 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local
 
 cleanup_fail:
 	free(m);
-	free(midx_name);
+	strbuf_release(&midx_name);
 	free_chunkfile(cf);
 	if (midx_map)
 		munmap(midx_map, midx_size);
@@ -1131,7 +1133,7 @@ static int write_midx_internal(const char *object_dir,
 			       const char *refs_snapshot,
 			       unsigned flags)
 {
-	char *midx_name;
+	struct strbuf midx_name = STRBUF_INIT;
 	unsigned char midx_hash[GIT_MAX_RAWSZ];
 	uint32_t i;
 	struct hashfile *f = NULL;
@@ -1142,10 +1144,10 @@ static int write_midx_internal(const char *object_dir,
 	int result = 0;
 	struct chunkfile *cf;
 
-	midx_name = get_midx_filename(object_dir);
-	if (safe_create_leading_directories(midx_name))
+	get_midx_filename(&midx_name, object_dir);
+	if (safe_create_leading_directories(midx_name.buf))
 		die_errno(_("unable to create leading directories of %s"),
-			  midx_name);
+			  midx_name.buf);
 
 	if (!packs_to_include) {
 		/*
@@ -1374,7 +1376,7 @@ static int write_midx_internal(const char *object_dir,
 		pack_name_concat_len += MIDX_CHUNK_ALIGNMENT -
 					(pack_name_concat_len % MIDX_CHUNK_ALIGNMENT);
 
-	hold_lock_file_for_update(&lk, midx_name, LOCK_DIE_ON_ERROR);
+	hold_lock_file_for_update(&lk, midx_name.buf, LOCK_DIE_ON_ERROR);
 	f = hashfd(get_lock_file_fd(&lk), get_lock_file_path(&lk));
 
 	if (ctx.nr - dropped_packs == 0) {
@@ -1411,9 +1413,9 @@ static int write_midx_internal(const char *object_dir,
 		ctx.pack_order = midx_pack_order(&ctx);
 
 	if (flags & MIDX_WRITE_REV_INDEX)
-		write_midx_reverse_index(midx_name, midx_hash, &ctx);
+		write_midx_reverse_index(midx_name.buf, midx_hash, &ctx);
 	if (flags & MIDX_WRITE_BITMAP) {
-		if (write_midx_bitmap(midx_name, midx_hash, &ctx,
+		if (write_midx_bitmap(midx_name.buf, midx_hash, &ctx,
 				      refs_snapshot, flags) < 0) {
 			error(_("could not write multi-pack bitmap"));
 			result = 1;
@@ -1443,7 +1445,7 @@ static int write_midx_internal(const char *object_dir,
 	free(ctx.entries);
 	free(ctx.pack_perm);
 	free(ctx.pack_order);
-	free(midx_name);
+	strbuf_release(&midx_name);
 
 	return result;
 }
@@ -1507,20 +1509,22 @@ static void clear_midx_files_ext(const char *object_dir, const char *ext,
 
 void clear_midx_file(struct repository *r)
 {
-	char *midx = get_midx_filename(r->objects->odb->path);
+	struct strbuf midx = STRBUF_INIT;
+
+	get_midx_filename(&midx, r->objects->odb->path);
 
 	if (r->objects && r->objects->multi_pack_index) {
 		close_midx(r->objects->multi_pack_index);
 		r->objects->multi_pack_index = NULL;
 	}
 
-	if (remove_path(midx))
-		die(_("failed to clear multi-pack-index at %s"), midx);
+	if (remove_path(midx.buf))
+		die(_("failed to clear multi-pack-index at %s"), midx.buf);
 
 	clear_midx_files_ext(r->objects->odb->path, ".bitmap", NULL);
 	clear_midx_files_ext(r->objects->odb->path, ".rev", NULL);
 
-	free(midx);
+	strbuf_release(&midx);
 }
 
 static int verify_midx_error;
@@ -1573,12 +1577,15 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag
 	if (!m) {
 		int result = 0;
 		struct stat sb;
-		char *filename = get_midx_filename(object_dir);
-		if (!stat(filename, &sb)) {
+		struct strbuf filename = STRBUF_INIT;
+
+		get_midx_filename(&filename, object_dir);
+
+		if (!stat(filename.buf, &sb)) {
 			error(_("multi-pack-index file exists, but failed to parse"));
 			result = 1;
 		}
-		free(filename);
+		strbuf_release(&filename);
 		return result;
 	}
 
diff --git a/midx.h b/midx.h
index 6e32297fa3..b7d79a515c 100644
--- a/midx.h
+++ b/midx.h
@@ -48,8 +48,8 @@ struct multi_pack_index {
 #define MIDX_WRITE_BITMAP_HASH_CACHE (1 << 3)
 
 const unsigned char *get_midx_checksum(struct multi_pack_index *m);
-char *get_midx_filename(const char *object_dir);
-char *get_midx_rev_filename(struct multi_pack_index *m);
+void get_midx_filename(struct strbuf *out, const char *object_dir);
+void get_midx_rev_filename(struct strbuf *out, struct multi_pack_index *m);
 
 struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local);
 int prepare_midx_pack(struct repository *r, struct multi_pack_index *m, uint32_t pack_int_id);
diff --git a/pack-bitmap.c b/pack-bitmap.c
index f47a0a7db4..3f603425c9 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -292,9 +292,12 @@ static int load_bitmap_entries_v1(struct bitmap_index *index)
 
 char *midx_bitmap_filename(struct multi_pack_index *midx)
 {
-	return xstrfmt("%s-%s.bitmap",
-		       get_midx_filename(midx->object_dir),
-		       hash_to_hex(get_midx_checksum(midx)));
+	struct strbuf buf = STRBUF_INIT;
+
+	get_midx_filename(&buf, midx->object_dir);
+	strbuf_addf(&buf, "-%s.bitmap", hash_to_hex(get_midx_checksum(midx)));
+
+	return strbuf_detach(&buf, NULL);
 }
 
 char *pack_bitmap_filename(struct packed_git *p)
@@ -324,10 +327,12 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
 	}
 
 	if (bitmap_git->pack || bitmap_git->midx) {
+		struct strbuf buf = STRBUF_INIT;
+		get_midx_filename(&buf, midx->object_dir);
 		/* ignore extra bitmap file; we can only handle one */
-		warning("ignoring extra bitmap file: %s",
-			get_midx_filename(midx->object_dir));
+		warning("ignoring extra bitmap file: %s", buf.buf);
 		close(fd);
+		strbuf_release(&buf);
 		return -1;
 	}
 
diff --git a/pack-revindex.c b/pack-revindex.c
index 0e4a31d9db..70d0fbafcb 100644
--- a/pack-revindex.c
+++ b/pack-revindex.c
@@ -296,14 +296,14 @@ int load_pack_revindex(struct packed_git *p)
 
 int load_midx_revindex(struct multi_pack_index *m)
 {
-	char *revindex_name;
+	struct strbuf revindex_name = STRBUF_INIT;
 	int ret;
 	if (m->revindex_data)
 		return 0;
 
-	revindex_name = get_midx_rev_filename(m);
+	get_midx_rev_filename(&revindex_name, m);
 
-	ret = load_revindex_from_disk(revindex_name,
+	ret = load_revindex_from_disk(revindex_name.buf,
 				      m->num_objects,
 				      &m->revindex_map,
 				      &m->revindex_len);
@@ -313,7 +313,7 @@ int load_midx_revindex(struct multi_pack_index *m)
 	m->revindex_data = (const uint32_t *)((const char *)m->revindex_map + RIDX_HEADER_SIZE);
 
 cleanup:
-	free(revindex_name);
+	strbuf_release(&revindex_name);
 	return ret;
 }
 
-- 
2.33.0.96.g73915697e6

[PATCH v2 8/9] pack-bitmap.c: don't leak type-level bitmaps

[Taylor Blau]

test_bitmap_walk() is used to implement `git rev-list --test-bitmap`,
which compares the result of the on-disk bitmaps with ones generated
on-the-fly during a revision walk.

In fa95666a40 (pack-bitmap.c: harden 'test_bitmap_walk()' to check type
bitmaps, 2021-08-24), we hardened those tests to also check the four
special type-level bitmaps, but never freed those bitmaps. We should
have, since each required an allocation when we EWAH-decompressed them.

Free those, plugging that leak, and also free the base (the scratch-pad
bitmap), too.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 pack-bitmap.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 3f603425c9..3d81425c29 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -1726,6 +1726,12 @@ void test_bitmap_walk(struct rev_info *revs)
 	else
 		die("mismatch in bitmap results");
 
+	bitmap_free(result);
+	bitmap_free(tdata.base);
+	bitmap_free(tdata.commits);
+	bitmap_free(tdata.trees);
+	bitmap_free(tdata.blobs);
+	bitmap_free(tdata.tags);
 	free_bitmap_index(bitmap_git);
 }
 
-- 
2.33.0.96.g73915697e6

[PATCH v2 9/9] pack-bitmap.c: more aggressively free in free_bitmap_index()

[Taylor Blau]

The function free_bitmap_index() is somewhat lax in what it frees. There
are two notable examples:

  - While it does call kh_destroy_oid_map on the "bitmaps" map, which
    maps commit OIDs to their corresponding bitmaps, the bitmaps
    themselves are not freed. Note here that we recycle already-freed
    ewah_bitmaps into a pool, but these are handled correctly by
    ewah_pool_free().

  - We never bother to free the extended index's "positions" map, which
    we always allocate in load_bitmap().

Fix both of these.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 pack-bitmap.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/pack-bitmap.c b/pack-bitmap.c
index 3d81425c29..a56ceb9441 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -1859,9 +1859,17 @@ void free_bitmap_index(struct bitmap_index *b)
 	ewah_pool_free(b->trees);
 	ewah_pool_free(b->blobs);
 	ewah_pool_free(b->tags);
+	if (b->bitmaps) {
+		struct stored_bitmap *sb;
+		kh_foreach_value(b->bitmaps, sb, {
+			ewah_pool_free(sb->root);
+			free(sb);
+		});
+	}
 	kh_destroy_oid_map(b->bitmaps);
 	free(b->ext_index.objects);
 	free(b->ext_index.hashes);
+	kh_destroy_oid_pos(b->ext_index.positions);
 	bitmap_free(b->result);
 	bitmap_free(b->haves);
 	if (bitmap_is_midx(b)) {
-- 
2.33.0.96.g73915697e6

Re: [PATCH v2 5/9] builtin/repack.c: avoid leaking child arguments

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> @@ -586,8 +588,10 @@ static int write_midx_included_packs(struct string_list *include,
>  		strvec_pushf(&cmd.args, "--refs-snapshot=%s", refs_snapshot);
>  
>  	ret = start_command(&cmd);
> -	if (ret)
> +	if (ret) {
> +		child_process_clear(&cmd);
>  		return ret;
> +	}

This happens only when start_command() returns an error.  But the
function always calls child_process_clear() before doing so.

So I am not sure if this hunk is needed.  It didn't exist in v1, if
I recall correctly.  Am I missing something obvious?

> @@ -608,9 +612,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  	struct pack_geometry *geometry = NULL;
>  	struct strbuf line = STRBUF_INIT;
>  	struct tempfile *refs_snapshot = NULL;
> -	int i, ext, ret;
> +	int i, ext, ret = 0;
>  	FILE *out;
>  	int show_progress = isatty(2);
> +	int cmd_cleared = 0;
>  
>  	/* variables to be filled by option parsing */
>  	int pack_everything = 0;
> @@ -794,7 +799,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  
>  	ret = start_command(&cmd);
>  	if (ret)
> -		return ret;
> +		goto cleanup;

Likewise, at this point, start_command() should have already cleared
cmd before it returned an error, no?

> @@ -818,8 +823,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  	}
>  	fclose(out);
>  	ret = finish_command(&cmd);
> +	cmd_cleared = 1;
>  	if (ret)
> -		return ret;
> +		goto cleanup;

And cmd is also cleared after we pass this point.  So perhaps after
the cleanup label, there is no need to call child_process_clear() at
all, no?

Re: [PATCH v2 0/9] midx: clean up t5319 under 'SANITIZE=leak'

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> writes:

> Here is a small-ish update to my series which makes t5319 leak-free (i.e., it
> passes even when Git is compiled with SANITIZE=leak). It is based on
> ab/only-single-progress-at-once (so I dropped the cherry-picked patch towards
> the end from Ævar).

The range-diff was almost unreadable, so I looked at the difference
relative to the previous tip (i.e. "git diff @{1}").  I spotted two
classes of changes, one is where and how "struct child_process" gets
cleared, the other is how midx-related filenames are released.  I
think the latter changes are improvements, and among the former, the
"write_oid() was never called, so cmd was left unused" change does
look an improvement, but I do not know about the others.

Thanks.

Re: [PATCH v2 5/9] builtin/repack.c: avoid leaking child arguments

[Taylor Blau]

On Wed, Oct 27, 2021 at 04:44:48PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > @@ -586,8 +588,10 @@ static int write_midx_included_packs(struct string_list *include,
> >  		strvec_pushf(&cmd.args, "--refs-snapshot=%s", refs_snapshot);
> >
> >  	ret = start_command(&cmd);
> > -	if (ret)
> > +	if (ret) {
> > +		child_process_clear(&cmd);
> >  		return ret;
> > +	}
>
> This happens only when start_command() returns an error.  But the
> function always calls child_process_clear() before doing so.
>
> So I am not sure if this hunk is needed.  It didn't exist in v1, if
> I recall correctly.  Am I missing something obvious?

No, it was the person replying to you missing something obvious ;).

Any hunks like this that call child_process_clear() after
start_command() returns a non-zero value are unnecessary. But the one in
repack_promisor_objects() is good, and does prevent the leak that had
led me in this direction in the first place.

Here is a suitable replacement for this patch (I believe that everything
else in this version is fine as-is):

--- >8 ---

Subject: [PATCH] builtin/repack.c: avoid leaking child arguments

`git repack` invokes a handful of child processes: one to write the
actual pack, and optionally ones to repack promisor objects and update
the MIDX.

Most of these are freed automatically by calling `start_command()` (which
invokes it on error) and `finish_command()` which calls it
automatically.

But repack_promisor_objects() can initialize a child_process, populate
its array of arguments, and then return from the function before even
calling start_command().

Make sure that the prepared list of arguments is freed by calling
child_process_clear() ourselves to avoid leaking memory along this path.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 builtin/repack.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index 0b2d1e5d82..9b74e0d468 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -258,9 +258,11 @@ static void repack_promisor_objects(const struct pack_objects_args *args,
 	for_each_packed_object(write_oid, &cmd,
 			       FOR_EACH_OBJECT_PROMISOR_ONLY);

-	if (cmd.in == -1)
+	if (cmd.in == -1) {
 		/* No packed objects; cmd was never started */
+		child_process_clear(&cmd);
 		return;
+	}

 	close(cmd.in);

--
2.33.0.96.g73915697e6