Should the default pathDriver walk though volume mount points on Windows?

[TBBle]

While implementing WCOW for the containerd snapshotter (containerd/containerd#4419), I bounced off golang/go#26033 in calls to fstest.CheckDirectoryEqualWithApplier and had to work around it rather nastily.

The summary of the issue is that filepath.Walk does not walk through Volume Mount Points (i.e. how a WCOW Layer is mounted to the local filesystem), because Go see it as a symlink, rather than a mount-point (bind, overlay, etc) as you would see on Unix systems for the same behaviour.

There are four approaches I can see to fixing this:

1. Hoping Go changes behaviour for mount-points where that mount-point is the "canonical DOS name", i.e. like a non-bind mount, and hence resolves path/filepath: filepath.Walk does not work on mounted volumes in Windows containers golang/go#26033. (And for bind-mount-style mounts, but I assume at some point they chose the current approach of treating Windows bind-mounts as symlinks).
2. Keeping the current changes in the containerd tests from my PR, and hoping it passes code-review.
3. Implement a Window-specific PathDriver in containerd's test helpers, which overrides Walk, and either directly recognises volume mounts, or points to a newly-overridden Stat in a Driver, with the latter reporting a volume mount point as a directory, not a symlink.
4. Same thing, but in continuity as the default on Windows.
5. A hybrid: pathDriver in continuity has Walk changed to always call Driver.Lstat, and containerd's test framework can provide a modified Driver.Lstat.

This ticket is to ask whether that fourth or fifth options seems reasonable, before I come to creating PRs towards improving this situation; or take option 2 and just work around it.

[TBBle]

Per the comment on Walk:

// Note that filepath.Walk calls os.Stat, so if the context wants to
// to call Driver.Stat() for Walk, they need to create a new struct that
// overrides this method.
func (*pathDriver) Walk(root string, walkFn filepath.WalkFunc) error {
	return filepath.Walk(root, walkFn)
}

I guess the fourth and fifth options are considered the consuming project's problem, so when I get around to looking into this, I'll probably go with the third option, and implement a custom Walk for Windows that is identical to filepath.Walk except that it treats volume mounts like non-Windows platforms and walks through them.

[gabriel-samfira]

I think 4. is a sensible approach. An implementation of Walk that would test a junction to determine if it's a mount point for a volume, and allow Walk to visit it. Some form of recursion detection would be needed in order to avoid situations like two volumes having mount points pointing to each-other.

Do we care about regular symlinks, or are volumes enough to consider?

Do you by any chance have time to rebase the two PRs? I would like to have a stab at this and it would be great if I could easily try out our branches with the changes.

[gabriel-samfira]

Something like this maybe? https://go.dev/play/p/R-OKyD5IGBR

The result of which:

PS C:\Users\Administrator\work\walk> go run .\main.go
visiting C:\var
visiting C:\var\a-link
visiting C:\var\a-mnt
2022/09/21 10:45:50 Skipping hidden system file/folder: C:\var\a-mnt\$RECYCLE.BIN
2022/09/21 10:45:50 Skipping hidden system file/folder: C:\var\a-mnt\System Volume Information
visiting C:\var\a-mnt\sasa.txt
visiting C:\var\a-mnt\test2
2022/09/21 10:45:50 Skipping hidden system file/folder: C:\var\a-mnt\test2\$RECYCLE.BIN
2022/09/21 10:45:50 Skipping hidden system file/folder: C:\var\a-mnt\test2\System Volume Information
visiting C:\var\a-mnt\test2\gdfg.rtf
2022/09/21 10:45:50 >> C:\var\a-mnt\test2\test-recurse points to a volume we've already visited: \??\Volume{bae7ecae-0000-0000-0000-100000000000}\
visiting C:\var\a-mnt\test2\test-recurse
visiting C:\var\a.txt
visiting C:\var\aa
visiting C:\var\aa\bb
visiting C:\var\lib
visiting C:\var\lib\cni
visiting C:\var\lib\cni\results
visiting C:\var\source
visiting C:\var\target
visiting C:\var\test.txt
PS C:\Users\Administrator\work\walk> 

[TBBle]

Sorry, I won't have time to do the rebasing for at least the next week, I believe. The output and a skim of the playground link looks like it's doing pretty-much what I was thinking, a copy of filepath.Walk and helpers, with the extra block for os.ModeSymlink in func (w *walker) walk (and the recursion-catcher).

I wouldn't skip system/hidden folders though, that seems surprising in this use-case.

The thing I wonder about is if the second sight of the same volume should call walkFn with an error code or something, so it's a handleable situation; I suspect for example CheckDirectoryEqualWithApplier would probably need to fail if that happened, as it's unlikely to be an intended result of an Applier.

I also wonder if it's possible for Linux to produce a recursive directory tree with bind-mounts? There's no protection against that if it's possible.

Would it be better (for this use-case) if instead of rejecting volume mounts we've seen before, to reject volume mounts which lead back to volumes in the current path, so that we walk, e.g.,

• a/b (<mount to /VolA>)
• a/c (<mount to /VolA>)

such that we see the same thing under both b and c, i.e. the contents of /VolA. That's what you'd see in file-explorer, so seems the least-surprising result. If /VolA contained a mount-point back to the volume on which a lives, then rejecting that would be easily-justified.

That'd be more work though, as we'd have to know the volume of the starting point and track potentially a stack of volumes for the current path. Off-hand, the current use-cases we have for this aren't going to see volume mount points except for the initial directory anyway, so it may not be worth putting too much effort into.

Oh, and I say we shouldn't see through regular symlinks. Then the behaviour is consistent on Linux and Windows, and the walkFn can capture "Symlink to X" which is probably the right thing to capture in continuity anyway, since for our use-cases, we should only see relative symlinks within the walked tree anyway, I think.

[gabriel-samfira]

This was just a quick scratchpad implementation. I skipped system files because even as administrator, doing a Lstat on System volume information yields Access denied and stops walking. At least with the current copied and modified implementation from the standard package. Same thing for $RECICLE.BIN.

I'll write up a propper PR for this and we can move the discussion there. I can test it with your current branch. Shouldn't matter much. I don't think there have been any changes that would change the outcome/expectations in any way.

[gabriel-samfira]

I also wonder if it's possible for Linux to produce a recursive directory tree with bind-mounts? There's no protection against that if it's possible.

Seems on linux bind mount don't recurse. Same with rbind. same if we just bind a in b/a-mnt and vice versa:

Also, the ls command outputs something like:

[TBBle]

Makes sense. I like ls's behaviour of ignoring directories it's seen before by another name, but that would have a memory cost as I guess it is working by tracking already-seen device/inode pairs or similar.

[gabriel-samfira]

Created a PR here: #208

[TBBle]

Per golang/go#26033 (comment) it seems that as-of Go 1.21, on Windows if a path ends in a path-seperator, Walk (and LStat) will now follow the symlink first. This was already the Go behaviour on POSIX systems (as specified by the POSIX spec).

So I'll close this since both the original use-case has been removed, and because if any callers are in this situation, they are probably already passing in paths ending in a path separator to avoid being bitting by this problem on POSIX.

Also reference to #232 which may ensure that such paths always end in path separators, removing the need for callers to be aware of this.

[TBBle]

For the record as of Go 1.23, mount points will be identified as ModeIrregular, not ModeSymlink. I don't think we'll be affected as we aren't using mount-points any more in the code that prompted this ticket, but if anyone hits the same issue in similar circumstances, then the end of golang/go#63703 will be relevant reading.