Re: Memory leak with sparse-checkout

[Derrick Stolee <stolee@gmail.com>]

On 9/20/2021 11:52 AM, Taylor Blau wrote:
> On Mon, Sep 20, 2021 at 09:45:14PM +0930, Calbabreaker wrote:
>> What did you do before the bug happened? (Steps to reproduce your issue)
>>
>> This was ran:
>>
>> git clone https://github.com/Calbabreaker/piano --sparse
>> cd piano
>> git sparse-checkout add any_text
>> git checkout deploy-frontend
>> git sparse-checkout init --cone
>> git sparse-checkout add any_text

Thank you for pointing this out.

I'll point out that this was likely found because "--sparse" does not
initialize cone mode patterns, but you might have expected it to. This
will increase the priority of adding something like "--sparse=cone"
to the 'git clone' options.

> Thanks for the reproduction. An even simpler one may be (inside of any
> repository):
> 
>     git sparse-checkout init
>     git sparse-checkout add dir
>     git sparse-checkout init --cone
>     git sparse-checkout add dir
> 
> The problem occurs because we keep existing entries when adding to the
> sparse-checkout list, and cone-mode patterns do not mix with
> non cone-mode patterns.
> 
> So after the first init and "add dir", your sparse-checkout file looks
> like:
> 
>   /*
>   !/*/
>   dir
> 
> but then when we convert to cone-mode and try and add "dir" (which in
> cone-mode we'll convert to "/dir/"), we run into trouble when adding the
> existing "dir" entry. That's because add_patterns_cone_mode() calls
> insert_recursive_pattern() on every entry in the existing list,
> including "dir".
> 
> So when we call insert_recursive_pattern() with any pattern list and
> path containing "dir", we first insert "dir" into the list, and then:
> 
>   char *slash = strrchr(e->pattern, '/');
>   char *oldpattern = e->pattern;
> 
>   if (slash == e->pattern)
>     break;
>   // trim off a slash, repeat
> 
> except slash is NULL because "dir" doesn't contain a slash. And that
> explains the problem you're seeing, because (a) we'll stay in that while
> loop forever, and (b) because each iteration allocates memory to
> accommodate the new pattern, so we'll eventually run out of memory.

Yikes! Thanks for digging into the details.

> The wrong thing to do would be to handle this case by changing the
> conditional to "if (!slash || slash == e->pattern)", because we can't
> blindly carry forward some patterns which look like cone-mode patterns,
> since together the list of sparse-checkout entries may not represent a
> cone.
> 
> (An example here is if we added /foo/bar/baz/* without the corresponding
> /foo/, !/foo/*, and so on).
> 
> So I think the problem really is that we need to drop existing patterns
> when re-initializing the sparse-checkout in cone mode. We could try to
> recognize that existing patterns may already constitute a cone (and/or
> create a cone that covers the existing patterns).
> 
> But I think the easiest thing (if a little unfriendly) would be to just
> drop them and start afresh when re-initializing the sparse-checkout in
> cone mode.

This isn't sufficient, as a user can modify their .git/info/sparse-checkout
file whenever they want, so we should fix this bug regardless. We could add
a "Your existing patterns are not in cone mode" error.

It might still be a good idea to let "git sparse-checkout init --cone"
overwrite the sparse-checkout file _if the file is not already in cone
mode_.

Thanks,
-Stolee