Snapshotting root of drive fails when Volume Shadow Copy enabled

[Hakkin]

Snapshotting fails with ERROR upload error: The parameter is incorrect. when snapshotting the root of the drive with --enable-volume-shadow-copy set to always. Snapshotting a sub directory in the root works correctly. All commands were run with administrator privileges.

$ kopia --version
0.17.0 build: 89c8eb47af2e1d5c1d14fe299a0cf7eaac095abf from: kopia/kopia

$ kopia policy set --enable-volume-shadow-copy always C:\
Setting policy for Hakkin@desktop:C:\
 - setting "enable volume shadow copy" to always.

$ kopia snapshot create C:\
Snapshotting Hakkin@desktop:C:\ ...
creating volume shadow copy of C:\
new volume shadow copy id {8FB0579B-5364-4138-8CEA-A2CB2F553E42}
removing volume shadow copy id {8FB0579B-5364-4138-8CEA-A2CB2F553E42}
ERROR upload error: The parameter is incorrect.

$ kopia snapshot create C:\Users\
Snapshotting Hakkin@desktop:C:\Users ...
creating volume shadow copy of C:\
new volume shadow copy id {3FF5EB4F-8E6A-441A-87DD-A4975494BBBB}
 | 3 hashing, 1778 hashed (250 MB), 0 cached (0 B), uploaded 219.1 MB, estimating...

[tekert]

Same case on windows server 2019, doesn't seem to happen on Windows 10 on my other pc.

[Hakkin]

After running with debugging, I may know the issue.

DEBUG shadow copy root is \\?\GLOBALROOT\Device\HarddiskVolumeShadowCopy10
DEBUG snapshotted directory     {"path":".","error":"GetFileInformationByHandleEx \\\\?\\GLOBALROOT\\Device\\HarddiskVolumeShadowCopy10: The parameter is incorrect.","dur":"512.2µs"}

I believe I have ran into an issue like this before, it is caused by the shadow volume path not ending in \. For whatever reason Windows won't treat it as a folder without the ending \. You can observe the same behavior by trying to mount a Shadow Copy volume via mklink

$ mklink /j "vss" "\\?\GLOBALROOT\Device\HarddiskVolumeShadowCopy4"
Junction created for vss <<===>> \\?\GLOBALROOT\Device\HarddiskVolumeShadowCopy4

$ cd vss
The parameter is incorrect.

$ rmdir vss

$ mklink /j "vss" "\\?\GLOBALROOT\Device\HarddiskVolumeShadowCopy4\"
Junction created for vss <<===>> \\?\GLOBALROOT\Device\HarddiskVolumeShadowCopy4\

$ cd vss

$ cd
C:\...\vss

Notice the trailing \ on the second mklink command, without it, it doesn't work.

[Hakkin]

I tried to recompile with just a quick fix in upload_os_snapshot_windows.go by appending the \ manually, but it seems the path gets normalized somewhere, so it doesn't stick. Interestingly, I found a comment that seems to indicate a similar issue is known and is handled in other parts of the code, maybe it needs further handling to fix this issue as well:

kopia/fs/localfs/local_fs_os.go

Lines 114 to 116 in 4cf9582

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

[Hakkin]

I've traced the source of the error to the (fsd *filesystemDirectory) Iterate() function here:

kopia/fs/localfs/local_fs_os.go

Lines 66 to 69 in 2b92388

	func (fsd *filesystemDirectory) Iterate(ctx context.Context) (fs.DirectoryIterator, error) {
	fullPath := fsd.fullPath()
	f, direrr := os.Open(fullPath) //nolint:gosec

The first time this is called on a VSS snapshot, the fullPath will be the root Shadow Copy volume without any trailing \, the os.Open will work fine, but then later in (it *filesystemDirectoryIterator) Next(), we will error here on a *os.File.ReadDir call:

kopia/fs/localfs/local_fs_os.go

Lines 25 to 33 in 2b92388

	func (it *filesystemDirectoryIterator) Next(ctx context.Context) (fs.Entry, error) {
	for {
	// we're at the end of the current batch, fetch the next batch
	if it.currentIndex >= len(it.currentBatch) {
	batch, err := it.dirHandle.ReadDir(numEntriesToRead)
	if err != nil && !errors.Is(err, io.EOF) {
	// stop iteration
	return nil, err //nolint:wrapcheck
	}

The underlying error is returned by windows.GetFileInformationByHandleEx in the stdlib ReadDir implementation for Windows.

I managed to write a patch that fixes this issue for me, but I'm not entirely convinced this is the right part of the code to fix it in. I'd love if someone more familiar with this part of the code could look into this more...

diff --git a/fs/localfs/local_fs_os.go b/fs/localfs/local_fs_os.go
index e790b5b3..2a3ffbb2 100644
--- a/fs/localfs/local_fs_os.go
+++ b/fs/localfs/local_fs_os.go
@@ -66,7 +66,21 @@ func (it *filesystemDirectoryIterator) Close() {
 func (fsd *filesystemDirectory) Iterate(ctx context.Context) (fs.DirectoryIterator, error) {
        fullPath := fsd.fullPath()

-       f, direrr := os.Open(fullPath) //nolint:gosec
+       // Trying to list the shadow volume root directory will fail unless it contains a trailing \
+       // We detect that here and fix it
+       var openPath = fullPath
+       if runtime.GOOS == "windows" {
+               const shadowVolumePrefix = `\\?\GLOBALROOT\Device\HarddiskVolumeShadowCopy`
+               if strings.HasPrefix(fullPath, shadowVolumePrefix) {
+                       remainder := fullPath[len(shadowVolumePrefix):]
+                       // Check if we're at the root of the shadow volume, any subdirectories should contain a path separator
+                       if !strings.ContainsRune(remainder, os.PathSeparator) {
+                               openPath = fullPath + string(os.PathSeparator)
+                       }
+               }
+       }
+
+       f, direrr := os.Open(openPath) //nolint:gosec
        if direrr != nil {
                return nil, errors.Wrap(direrr, "unable to read directory")
        }

[Whissi]

I also encountered this bug when attempting to create a backup of an entire volume using a Volume Shadow Copy, and then I found your bug report.

Thank you for the detailed analysis.

At first, I wondered why you didn't choose a similar construct as in the NewEntry function further down:

kopia/fs/localfs/local_fs_os.go

Lines 113 to 123 in 89c8eb4

	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))
	}

However, when I tried to apply the same solution, I realized why: As you correctly analyzed, the problem occurs when calling ReadDir -- at that point, it is not possible to respond to the issue, as the handler does not allow adjusting the path there.

Nonetheless, I slightly modified your proposed solution, as I don't see the necessity for introducing the remainder variable.
To stay consistent with the existing code style, I found the following solution better -- am I overlooking something?

--- a/fs/localfs/local_fs_os.go
+++ b/fs/localfs/local_fs_os.go
@@ -66,12 +66,24 @@ func (it *filesystemDirectoryIterator) Close() {
 func (fsd *filesystemDirectory) Iterate(ctx context.Context) (fs.DirectoryIterator, error) {
        fullPath := fsd.fullPath()

+       if runtime.GOOS == "windows" {
+               const shadowVolumePrefix = `\\?\GLOBALROOT\Device\HarddiskVolumeShadowCopy`
+
+               if strings.HasPrefix(fullPath, shadowVolumePrefix) &&
+                       !strings.HasSuffix(fullPath, string(filepath.Separator)) {
+                       fullPath = fullPath + string(filepath.Separator)
+               }
+       }
+
        f, direrr := os.Open(fullPath) //nolint:gosec
        if direrr != nil {
                return nil, errors.Wrap(direrr, "unable to read directory")
        }

-       childPrefix := fullPath + string(filepath.Separator)
+       childPrefix := fullPath
+       if !strings.HasSuffix(childPrefix, string(filepath.Separator)) {
+               childPrefix += string(filepath.Separator)
+       }

        return &filesystemDirectoryIterator{dirHandle: f, childPrefix: childPrefix}, nil
 }
 

However, in principle, I wonder if this is really the ideal place: Wouldn't it be better to patch

kopia/fs/localfs/local_fs.go

Lines 51 to 53 in 89c8eb4

	func (e *filesystemEntry) fullPath() string {
	return e.prefix + e.Name()
	}

and ensure that the return value ends with a trailing backslash if e.prefix == "\\?\GLOBALROOT\Device"?

[Hakkin]

Thanks for the additional eyes,

I don't see the necessity for introducing the remainder variable.

The remainder part ensures we only apply the trailing path separator to the top level shadow volume root directory, no directory below it requires it. The remainder code is checking if we're at the root or in a subdirectory.

I separated openPath and fullPath because only the Open call needs the trailing slash, all the rest of Kopia's code doesn't expect a trailing slash there. I don't know if it would actually break anything, but I didn't see a reason to change the behavior. This is the same reason I didn't modify the fullPath function itself.

[Hakkin]

After checking your patch again, I think your childPrefix check actually handles the second part of my comment. For some reason I was under the impression that fullPath got used or returned somewhere by itself, but since it's just being used for childPrefix, your check should be sufficient.

The first part of my comment is still valid I believe.