[v2,0/13] leak fixes for sparse-checkout code

Message

Jeff King June 4, 2024, 10:08 a.m. UTC

On Fri, May 31, 2024 at 07:24:34AM -0400, Jeff King wrote:

> So Patrick nerd-sniped me by asking if my earlier leakfix for git-mv was
> triggered by the test suite. It was, in t7002, but that wasn't enough to
> make the script leak-free. So I figured, how hard could it be to go all
> the way?
> 
> Well. It only took a few patches (1-5), but in the process I stumbled on
> a rather tricky interface oddity of add_pattern(), which caused some
> other leaks. The interface is fixed in patch 6, and the matching leak
> goes away in patch 7. Of course, I wanted to make sure it was tested, so
> after poking around I found that t1091 triggered it.
> 
> But as you might guess, that didn't make t1091 leak-free. And I couldn't
> bear leaving it on a cliffhanger like that, so patches 8-13 fix the rest
> of the issues triggered by that script.
> 
> And along the way we managed to make t1090 and t3602 leak-free, too
> (actually in patch 2, but I didn't notice until the whole thing was
> done).
> 
> These should apply on top of jk/leakfixes, since the leak-freeness of
> t7002 depends on the fix there.

Here's a v2 with a few minor updates, based on review from Patrick. The
restoration of "old_pl" in patch 9 is now done unconditionally (it
doesn't matter in practice due to a bunch of subtle things, but it's the
less surprising and more defensive choice). And a small style update in
patch 13.

Range-diff:

 1:  1be529905f =  1:  1be529905f sparse-checkout: free string list in write_cone_to_file()
 2:  8e36bf2213 =  2:  8e36bf2213 sparse-checkout: pass string literals directly to add_pattern()
 3:  1a3c5819f9 =  3:  1a3c5819f9 dir.c: free strings in sparse cone pattern hashmaps
 4:  a492842dd3 =  4:  a492842dd3 sparse-checkout: clear patterns when init() sees existing sparse file
 5:  2d8d402809 =  5:  2d8d402809 dir.c: free removed sparse-pattern hashmap entries
 6:  addb69e229 =  6:  addb69e229 dir.c: always copy input to add_pattern()
 7:  f2ee2192f7 =  7:  f2ee2192f7 sparse-checkout: reuse --stdin buffer when reading patterns
 8:  d3f7ea12ab =  8:  d3f7ea12ab sparse-checkout: always free "line" strbuf after reading input
 9:  b5c07325d2 !  9:  b78d3b51b9 sparse-checkout: refactor temporary sparse_checkout_patterns
    @@ builtin/sparse-checkout.c: static int update_working_directory(struct pattern_li
     +	if (r->index->sparse_checkout_patterns != pl) {
     +		clear_pattern_list(r->index->sparse_checkout_patterns);
     +		FREE_AND_NULL(r->index->sparse_checkout_patterns);
    -+	} else {
    -+		r->index->sparse_checkout_patterns = old_pl;
     +	}
    ++	r->index->sparse_checkout_patterns = old_pl;
    ++
      	return result;
      }
      
10:  26b1e08e6f = 10:  ef05901aef sparse-checkout: free sparse_filename after use
11:  d14f0f0546 = 11:  7b423b1691 sparse-checkout: free pattern list in sparse_checkout_list()
12:  84b1a5eb0d = 12:  034668997b sparse-checkout: free string list after displaying
13:  460a7f9324 ! 13:  82f14304ae sparse-checkout: free duplicate hashmap entries
    @@ builtin/sparse-checkout.c: static void insert_recursive_pattern(struct pattern_l
      
     -		if (!hashmap_get_entry(&pl->parent_hashmap, e, ent, NULL))
     +		dup = hashmap_get_entry(&pl->parent_hashmap, e, ent, NULL);
    -+		if (!dup)
    ++		if (!dup) {
      			hashmap_add(&pl->parent_hashmap, &e->ent);
    -+		else {
    ++		} else {
     +			free(e->pattern);
     +			free(e);
     +			e = dup;

  [01/13]: sparse-checkout: free string list in write_cone_to_file()
  [02/13]: sparse-checkout: pass string literals directly to add_pattern()
  [03/13]: dir.c: free strings in sparse cone pattern hashmaps
  [04/13]: sparse-checkout: clear patterns when init() sees existing sparse file
  [05/13]: dir.c: free removed sparse-pattern hashmap entries
  [06/13]: dir.c: always copy input to add_pattern()
  [07/13]: sparse-checkout: reuse --stdin buffer when reading patterns
  [08/13]: sparse-checkout: always free "line" strbuf after reading input
  [09/13]: sparse-checkout: refactor temporary sparse_checkout_patterns
  [10/13]: sparse-checkout: free sparse_filename after use
  [11/13]: sparse-checkout: free pattern list in sparse_checkout_list()
  [12/13]: sparse-checkout: free string list after displaying
  [13/13]: sparse-checkout: free duplicate hashmap entries

 builtin/sparse-checkout.c          | 49 +++++++++++++++++++-----------
 dir.c                              | 42 ++++++++++++++++---------
 dir.h                              |  3 +-
 t/t1090-sparse-checkout-scope.sh   |  1 +
 t/t1091-sparse-checkout-builtin.sh |  1 +
 t/t3602-rm-sparse-checkout.sh      |  1 +
 t/t7002-mv-sparse-checkout.sh      |  1 +
 7 files changed, 65 insertions(+), 33 deletions(-)

-Peff

Comments

Patrick Steinhardt June 4, 2024, 12:15 p.m. UTC | #1

On Tue, Jun 04, 2024 at 06:08:14AM -0400, Jeff King wrote:
> On Fri, May 31, 2024 at 07:24:34AM -0400, Jeff King wrote:
> 
> > So Patrick nerd-sniped me by asking if my earlier leakfix for git-mv was
> > triggered by the test suite. It was, in t7002, but that wasn't enough to
> > make the script leak-free. So I figured, how hard could it be to go all
> > the way?
> > 
> > Well. It only took a few patches (1-5), but in the process I stumbled on
> > a rather tricky interface oddity of add_pattern(), which caused some
> > other leaks. The interface is fixed in patch 6, and the matching leak
> > goes away in patch 7. Of course, I wanted to make sure it was tested, so
> > after poking around I found that t1091 triggered it.
> > 
> > But as you might guess, that didn't make t1091 leak-free. And I couldn't
> > bear leaving it on a cliffhanger like that, so patches 8-13 fix the rest
> > of the issues triggered by that script.
> > 
> > And along the way we managed to make t1090 and t3602 leak-free, too
> > (actually in patch 2, but I didn't notice until the whole thing was
> > done).
> > 
> > These should apply on top of jk/leakfixes, since the leak-freeness of
> > t7002 depends on the fix there.
> 
> Here's a v2 with a few minor updates, based on review from Patrick. The
> restoration of "old_pl" in patch 9 is now done unconditionally (it
> doesn't matter in practice due to a bunch of subtle things, but it's the
> less surprising and more defensive choice). And a small style update in
> patch 13.

Thanks, the range-diff looks as expected.

Patrick