On Tue, Jun 04, 2024 at 09:42:51AM +0200, Patrick Steinhardt wrote:
> > 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.
> > @@ -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
