Re: [PATCH 09/13] sparse-checkout: refactor temporary sparse_checkout_patterns

From: Jeff King <peff@peff.net>
Date: Jun 4, 2024, 09:53 UTC
Message-ID: <20240604095348.GB1298378@coredump.intra.peff.net>
In-Reply-To: <Zl7Fe6132gy1JclD@framework>

On Tue, Jun 04, 2024 at 09:42:51AM +0200, Patrick Steinhardt wrote:

Show 8 quoted lines

> > But there are two possible leaks here:
> > 
> >   1. If the index already had a populated sparse_checkout_patterns,
> >      we've obliterated it. We can fix this by saving and restoring it,
> >      rather than always setting it back to NULL.
> 
> So this isn't only a leak, but also a potential bug because we did not
> restore the old pattern list?

That's a good question, but I think in practice, it's not possible to trigger the bug. Within a run that calls this function, we only care about index->sparse_checkout_patterns in the context of this function itself (that is, we do not otherwise try to read the sparse index). And in this function, we always want to use the pattern_list that the caller hands us.

So nobody actually cares about the "restored" pattern list at all (except that in the lazy-load case, we leak it). Every call will overwrite it anyway.

Show 18 quoted lines

> > @@ -241,7 +243,12 @@ static int update_working_directory(struct pattern_list *pl)
> >  
> >  	clean_tracked_sparse_directories(r);
> >  
> > -	r->index->sparse_checkout_patterns = NULL;
> > +	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;
> > +	}
> >  	return result;
> >  }
> 
> What I find weird is that we sometimes restore the old value, and
> sometimes we don't. I get that it makes sense to conditionally free only
> the lazy-loaded list. But shouldn't we then unconditionally assign
> `old_pl`?

Yes, I think it would be more correct to do so (otherwise we risk leaking old_pl). In practice, as I mentioned above, this is the only function that actually assigns to the index list. So after this hunk, I think we'd always come in to the function with a NULL value in the index (it starts NULL, we restore NULL after using a passed-in value, and we restore free-and-NULL after lazy-loading).

So we _could_ actually drop the "save old_pl" hunk from the beginning of the function entirely. I only hit that case after trying something like:

  /* only throw away patterns if they were the ones we own */
  if (r->index->sparse_checkout_patterns == pl)
	r->index->sparse_checkout_patterns = NULL;

at the end of the function to fix the leak. And with that code, we _do_ see the lazy-loaded entries in subsequent calls. If we instead do:

  /* throw away lazy-loaded ones */
  if (r->index->sparse_checkout_patterns != pl) {
	clear_pattern_list(r->index->sparse_checkout_patterns);
	FREE_AND_NULL(r->index->sparse_checkout_patterns);
  }

Then I think you don't really need the else clause at all. But I was trying to be defensive and not make too many assumptions. I think in that spirit, just restoring old_pl always is the right thing, even though I don't think we'd ever trigger that.

-Peff