fix(snapshots): Append path separator to Shadow Copy root directory on Windows

[Hakkin]

This is a fix for #3842.
It's a slightly modified version of the patch in #3842 (comment), I separated the code out into a maybe function and added some tests.
The root cause of this issue seems to be a regression in Go 1.22 (see #3842 (comment)), but as discussed with jkowalski in the issue, it seems better to just fix this up with a special case in Kopia.

[Hakkin]

Something additional to note, there is a similar check going on here in the NewEntry function:

kopia/fs/localfs/local_fs_os.go

Lines 112 to 128 in fcb8197

	fi, err := os.Lstat(path)
	if err != nil {
	// Paths such as `\\?\GLOBALROOT\Device\HarddiskVolumeShadowCopy01`
	// cause os.Lstat to fail with "Incorrect function" error unless they
	// end with a separator. Retry the operation with the separator added.
	var e syscall.Errno
	//nolint:goconst
	if runtime.GOOS == "windows" &&
	!strings.HasSuffix(path, string(filepath.Separator)) &&
	errors.As(err, &e) && e == 1 {
	fi, err = os.Lstat(path + string(filepath.Separator))
	}
	if err != nil {
	return nil, errors.Wrap(err, "unable to determine entry type")
	}
	}

I wonder if it would be useful to change this to use the new maybeAppendPathSeparatorForVSSOnWindows function as well.

[Whissi]

I applied this PR on top of v0.17.0 tag which I confirmed to be showing the problem reported in #3842 using go version go1.22.2 windows/amd64. With this PR applied, I am now able again to snapshot an entire volume when using a volume shadow copy.

However, it's worth noting that appending a trailing backslash when detecting the \\?\GLOBALROOT\Device\HarddiskVolumeShadowCopy prefix is not specific to shadow copies but rather ensures proper handling of paths within the Windows Object namespace. \\?\GLOBALROOT\Device\HarddiskVolumeShadowCopyXX represents a symbolic link within the Windows Object namespace (\\GLOBALROOT folder) pointing to \Device\HarddiskVolumeShadowCopyXX (which is a device and nothing within \\GLOBALROOT folder). For Windows, without a trailing backslash, it may not be clear whether we intend to operate on the symbolic link itself or on the actual resource, \Device\HarddiskVolumeShadowCopyXX. Thus, the workaround is essential for disambiguation. It might be beneficial to consider a more generalized approach by checking if a path starts with \ and ensuring it ends with a trailing backslash. This would provide robustness in handling various path scenarios, especially in our case where we always expect to deal with iterable objects (directories). And if we wouldn't access the shadow copy through Windows Object namespace at all (for example we could mount the shadow copy somewhere) we wouldn't need to take care of this at all.

[Hakkin]

Thanks for the extra info.

\?\GLOBALROOT\Device\HarddiskVolumeShadowCopyXX represents a symbolic link within the Windows Object namespace (\GLOBALROOT folder) pointing to \Device\HarddiskVolumeShadowCopyXX (which is a device and nothing within \GLOBALROOT folder).

I wonder if there's any Windows API to detect or resolve links like this in the Object namespace? For normal symlinks / reparse points in the filesystem this can be done with os.Lstat, but as noted in the code in my second comment, Lstat seems to fail on these kinds of paths unless you "manually" resolve them with the trailing backslash. If there's a way to detect them directly, it might make sense to implement them like normal symlinks are now, and there wouldn't be any need to handle shadow volumes explicitly.

It might be beneficial to consider a more generalized approach by checking if a path starts with \ and ensuring it ends with a trailing backslash. This would provide robustness in handling various path scenarios, especially in our case where we always expect to deal with iterable objects (directories).

I considered doing something like this but I was worried about edge cases, which is why I went out of my way to make sure the workaround is only applied to the top-level root directory. Since "inside" the shadow volume device is just a normal filesystem, appending a backslash anywhere after the root might have different consequences, like resolving links. If we did something more generic, it would definitely need a lot more testing to make sure all the edge cases are handled correctly.

[Hakkin]

I ended up reading a lot about Windows path handling, this article seems to contain a lot of the relevant information.

I believe the fundamental issue here is that Windows treats opening the path \\?\GLOBALROOT\Device\HarddiskVolumeShadowCopy1 as attempting to open the raw block device for that volume, while it treats \\?\GLOBALROOT\Device\HarddiskVolumeShadowCopy1\ as opening the root directory of the volume, and it passes it to the underlying filesystem driver.

Unfortunately, it seems non-triviable to handle this "correctly". It seems impossible to know whether a UNC path points to a device object without doing multiple Windows-specific syscalls for every path.

That being said, I think there is a way to handle this a bit more cleanly. For one, rather than the current maybe function, after thinking about it more, I think it's better to implement a method on filesystemEntry that checks the path parts. filesystemEntry already has the prefix and filename split out, so it simplifies the logic and we don't have to do any path handling in the checking function.

func (e *filesystemEntry) isWindowsVSSVolume() bool {
	return runtime.GOOS == "windows" &&
		e.prefix == `\\?\GLOBALROOT\Device\` &&
		strings.HasPrefix(e.Name(), "HarddiskVolumeShadowCopy")
}

then we can just do this in Iterate:

fullPath := fsd.fullPath()

appendPath := ""
if fsd.isWindowsVSSVolume() {
	appendPath = string(os.PathSeparator)
}

f, direrr := os.Open(fullPath + appendPath) //nolint:gosec

and that's all the logic needed.

If we ever want to make it a bit more generic in the future. the isWindowsVSSVolume function can be expanded to support other common volume paths like \\?\Volume{...} as well.

If there's no objections to this revised patch, I think I'll push another commit with it in a little bit since it's pretty much the same logic as the previous commit, just cleaner.

[codecov]

Codecov Report

Attention: Patch coverage is 75.00000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 77.13%. Comparing base (cb455c6) to head (b197a4c).
Report is 148 commits behind head on master.

❗ Current head b197a4c differs from pull request most recent head a8c2bde

Please upload reports for the commit a8c2bde to get more accurate results.

Files	Patch %	Lines
fs/localfs/local_fs_os.go	50.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3891      +/-   ##
==========================================
+ Coverage   75.86%   77.13%   +1.27%     
==========================================
  Files         470      479       +9     
  Lines       37301    28755    -8546     
==========================================
- Hits        28299    22181    -6118     
+ Misses       7071     4669    -2402     
+ Partials     1931     1905      -26     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Hakkin]

Regarding the test coverage, should I add a test that covers this case? I think I could just mostly copy https://github.com/kopia/kopia/blob/master/tests/os_snapshot_test/os_snapshot_windows_test.go but enable enable-volume-shadow-copy=always and set it to snapshot the root of the C:\ drive instead, we can add an ignore policy so it doesn't actually snapshot any of the files, just tests creating the shadow copy, since that's where it would fail before.