Re: Sparse Checkout Trouble (2.5.0)

From: Derrick Stolee <stolee@gmail.com>
Date: Jan 29, 2020, 15:40 UTC
Message-ID: <60b34188-d05d-6753-7116-a34793592227@gmail.com>
In-Reply-To: <nycvar.QRO.7.76.6.2001291626550.46@tvgsbejvaqbjf.bet>

On 1/29/2020 10:30 AM, Johannes Schindelin wrote:

Show 22 quoted lines

> 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.
> 
> Hmm. I am actually not sure that this would be a fix. It is all too easy
> for a skip-worktree entry to become checked out (think e.g. a merge
> conflict in a cherry-pick, during a three-way merge of a file that is not
> in the cone but still needs to be handled).

This is a very good point, and something worth investigating.

> 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.

> 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.

Thanks, -Stolee