Re: [PATCH v3 1/3] sparse-checkout: fix segfault on malformed patterns

> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> Then core.sparseCheckoutCone is enabled, the sparse-checkout patterns are
>> used to populate two hashsets that accelerate pattern matching. If the user
>> modifies the sparse-checkout file outside of the 'sparse-checkout' builtin,
>> then strange patterns can happen, triggering some error checks.
>>
>> One of these error checks is possible to hit when some special characters
>> exist in a line. A warning message is correctly written to stderr, but then
>> there is additional logic that attempts to remove the line from the hashset
>> and free the data.
> 
> Makes sense.
> 
>> This leads to a segfault in the 'git sparse-checkout
>> list' command because it iterates over the contents of the hashset, which is
>> now invalid.
> 
> Understandable.
> 
>> The fix here is to stop trying to remove from the hashset. Better to leave
>> bad data in the sparse-checkout matching logic (with a warning) than to
>> segfault.
> 
> True, as long as it won't make the situation worse by depending on
> that bad data to further damage working tree data or in-repo data
> when damaged working tree data gets committed.  And "list segfaults
> with freed/NULLed data---so leave the bad ones in to print these bad
> ones" feels OK-ish.  
> 
> As long as the user is not transporting the listed output to another
> repository, which may fall into "making the situation worse"
> category by spreading an existing breakage, that is.
> 
> In other words, this may paper over the segfault, and it may be safe
> only for "sparse-checkout list", but is it safe for other operations
> that actually use this bad data to further affect other things in
> the repository?  If not, I wonder if we want to hard die to lock the
> repository down before the issue is fixed to avoid spreading the
> damage?

>> +test_expect_success 'malformed cone-mode patterns' '
>> +	git -C repo sparse-checkout init --cone &&
>> +	mkdir -p repo/foo/bar &&
>> +	touch repo/foo/bar/x repo/foo/y &&
>> +	cat >repo/.git/info/sparse-checkout <<-\EOF &&
>> +	/*
>> +	!/*/
>> +	/foo/
>> +	!/foo/*/
>> +	/foo/\*/
>> +	EOF
>> +	cat repo/.git/info/sparse-checkout &&
> 
> Stray debugging output?