Re: Sparse Checkout Trouble (2.5.0)

From: Derrick Stolee <stolee@gmail.com>
Date: Jan 29, 2020, 16:31 UTC
Message-ID: <a180c7a5-2406-0384-9355-be9db05a16fd@gmail.com>
In-Reply-To: <60b34188-d05d-6753-7116-a34793592227@gmail.com>

On 1/29/2020 10:40 AM, Derrick Stolee wrote:

Show 18 quoted lines

> On 1/29/2020 10:30 AM, Johannes Schindelin wrote:
>> Hi,
>>
>> On Tue, 28 Jan 2020, Derrick Stolee wrote:
>>> I made some progress, at least, in root-causing the issue.
>>> The problem bisects down to 4dc42c6c1 (mingw: refuse paths
>>> containing reserved names, 2019-12-21) [1]. CC'ing Dscho.
>>>
>>> That commit updates is_valid_win32_path() to fail on these
>>> paths. We were _already_ calling this method even for paths
>>> outside the sparse cone, but the method didn't fail for these
>>> examples.
>>>
>>> This means the fix is probably even more complicated: we need
>>> to not call this method when traversing paths that have the
>>> skip-worktree bit enabled. This may lead to some tiny
>>> performance gains when hydrating a very small fraction of a
>>> very large index.

After digging into this more, at the place where we call verify_path() and create this warning, all cache entries have the same ce_flags (0x0209_0000):

* 0x0200_0000: CE_NEW_SKIP_WORKTREE
* 0x0008_0000: CE_ADDED
* 0x0001_0000: CE_UPDATE

So, at this point there is no way to differentiate between paths that will be written to disk or paths that will not. That decision appears to be made at another time, and connecting these decisions at a distance is ripe for unintended behavior.

Show 11 quoted lines

>> My preferred solution for this issue would be for the files/directories to
>> be renamed using `git -c core.protectntfs=false mv <reserved-name>
>> <non-reserved-name>`.
> 
> One thing that I realized after root-causing the issue is that now the
> Linux kernel repository cannot be checked out _at all_ on Windows due to
> the existence of an aux.c file. Git complains that the path is invalid
> and does not write a single file to the working directory.
>
> At least we _could_ allow someone to create most of the working directory
> (as we did before) by allowing invalid paths outside the sparse cone.

Of course, as you say, setting core.protectntfs=false allows the Linux
kernel to be checked out as before, but without the safety valve.

>> I think if we try to play extra games with the skip-worktree bit, we risk
>> opening a vulnerability again.
> 
> I agree that we need to be _very_ careful with this.

After my investigation, and your workaround, I'm content to settle this situation "as designed" and to leave it at that.

Thanks, -Stolee