On Fri, May 31, 2024 at 07:35:30AM -0400, Jeff King wrote:
> In update_working_directory(), we take in a pattern_list, attach it to
> the repository index by assigning it to index->sparse_checkout_patterns,
> and then call unpack_trees. Afterwards, we remove it by setting
> index->sparse_checkout_patterns back to NULL.
>
> 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?
> @@ -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`?
Makes me wonder whether I miss some subtlety here.
Patrick
Attachment:
signature.asc
Description: PGP signature
