From 1d392a36f9df04840364dce29f553a41b449f3c8 Mon Sep 17 00:00:00 2001 From: aneesh-n <99904+aneesh-n@users.noreply.github.com> Date: Sun, 11 Aug 2024 01:23:47 -0600 Subject: [PATCH 1/4] Fix extended attributes handling for VSS snapshots --- internal/fs/ea_windows.go | 18 +++++++ internal/restic/node_windows.go | 90 +++++++++++++++++++++++---------- 2 files changed, 80 insertions(+), 28 deletions(-) diff --git a/internal/fs/ea_windows.go b/internal/fs/ea_windows.go index d19a1ee6a..bf7b02fd4 100644 --- a/internal/fs/ea_windows.go +++ b/internal/fs/ea_windows.go @@ -8,6 +8,7 @@ import ( "encoding/binary" "errors" "fmt" + "strings" "syscall" "unsafe" @@ -298,3 +299,20 @@ func PathSupportsExtendedAttributes(path string) (supported bool, err error) { supported = (fileSystemFlags & windows.FILE_SUPPORTS_EXTENDED_ATTRIBUTES) != 0 return supported, nil } + +// GetVolumePathName returns the volume path name for the given path. +func GetVolumePathName(path string) (volumeName string, err error) { + utf16Path, err := windows.UTF16PtrFromString(path) + if err != nil { + return "", err + } + // Get the volume path (e.g., "D:") + var volumePath [windows.MAX_PATH + 1]uint16 + err = windows.GetVolumePathName(utf16Path, &volumePath[0], windows.MAX_PATH+1) + if err != nil { + return "", err + } + // Trim any trailing backslashes + volumeName = strings.TrimRight(windows.UTF16ToString(volumePath[:]), "\\") + return volumeName, nil +} diff --git a/internal/restic/node_windows.go b/internal/restic/node_windows.go index ceb304d0c..6adb51f0d 100644 --- a/internal/restic/node_windows.go +++ b/internal/restic/node_windows.go @@ -407,40 +407,74 @@ func (node *Node) fillGenericAttributes(path string, fi os.FileInfo, stat *statT // checkAndStoreEASupport checks if the volume of the path supports extended attributes and stores the result in a map // If the result is already in the map, it returns the result from the map. func checkAndStoreEASupport(path string) (isEASupportedVolume bool, err error) { - // Check if it's an extended length path - if strings.HasPrefix(path, uncPathPrefix) { - // Convert \\?\UNC\ extended path to standard path to get the volume name correctly - path = `\\` + path[len(uncPathPrefix):] - } else if strings.HasPrefix(path, extendedPathPrefix) { - //Extended length path prefix needs to be trimmed to get the volume name correctly - path = path[len(extendedPathPrefix):] - } else if strings.HasPrefix(path, globalRootPrefix) { - // EAs are not supported for \\?\GLOBALROOT i.e. VSS snapshots - return false, nil - } else { - // Use the absolute path - path, err = filepath.Abs(path) - if err != nil { - return false, fmt.Errorf("failed to get absolute path: %w", err) - } - } - volumeName := filepath.VolumeName(path) - if volumeName == "" { - return false, nil - } - eaSupportedValue, exists := eaSupportedVolumesMap.Load(volumeName) - if exists { - return eaSupportedValue.(bool), nil + var volumeName string + volumeName, err = prepareVolumeName(path) + if err != nil { + return false, err } - // Add backslash to the volume name to ensure it is a valid path - isEASupportedVolume, err = fs.PathSupportsExtendedAttributes(volumeName + `\`) - if err == nil { - eaSupportedVolumesMap.Store(volumeName, isEASupportedVolume) + if volumeName != "" { + // First check if the manually prepared volume name is already in the map + eaSupportedValue, exists := eaSupportedVolumesMap.Load(volumeName) + if exists { + return eaSupportedValue.(bool), nil + } + // If not found, check if EA is supported with manually prepared volume name + isEASupportedVolume, err = fs.PathSupportsExtendedAttributes(volumeName + `\`) + if err != nil { + return false, err + } } + // If an entry is not found, get the actual volume name using the GetVolumePathName function + volumeNameActual, err := fs.GetVolumePathName(path) + if err != nil { + return false, err + } + if volumeNameActual != volumeName { + // If the actual volume name is different, check cache for the actual volume name + eaSupportedValue, exists := eaSupportedVolumesMap.Load(volumeNameActual) + if exists { + return eaSupportedValue.(bool), nil + } + // If the actual volume name is different and is not in the map, again check if the new volume supports extended attributes with the actual volume name + isEASupportedVolume, err = fs.PathSupportsExtendedAttributes(volumeNameActual + `\`) + if err != nil { + return false, err + } + } + eaSupportedVolumesMap.Store(volumeNameActual, isEASupportedVolume) return isEASupportedVolume, err } +// prepareVolumeName prepares the volume name for different cases in Windows +func prepareVolumeName(path string) (volumeName string, err error) { + // Check if it's an extended length path + if strings.HasPrefix(path, globalRootPrefix) { + // Extract the VSS snapshot volume name eg. `\\?\GLOBALROOT\Device\HarddiskVolumeShadowCopyXX` + if parts := strings.SplitN(path, `\`, 7); len(parts) >= 6 { + volumeName = strings.Join(parts[:6], `\`) + } else { + volumeName = filepath.VolumeName(path) + } + } else { + if strings.HasPrefix(path, uncPathPrefix) { + // Convert \\?\UNC\ extended path to standard path to get the volume name correctly + path = `\\` + path[len(uncPathPrefix):] + } else if strings.HasPrefix(path, extendedPathPrefix) { + //Extended length path prefix needs to be trimmed to get the volume name correctly + path = path[len(extendedPathPrefix):] + } else { + // Use the absolute path + path, err = filepath.Abs(path) + if err != nil { + return "", fmt.Errorf("failed to get absolute path: %w", err) + } + } + volumeName = filepath.VolumeName(path) + } + return volumeName, nil +} + // windowsAttrsToGenericAttributes converts the WindowsAttributes to a generic attributes map using reflection func WindowsAttrsToGenericAttributes(windowsAttributes WindowsAttributes) (attrs map[GenericAttributeType]json.RawMessage, err error) { // Get the value of the WindowsAttributes From b5b5c1fe8e2ffe17d5b836af2d3a54326a28a945 Mon Sep 17 00:00:00 2001 From: aneesh-n <99904+aneesh-n@users.noreply.github.com> Date: Sun, 11 Aug 2024 01:32:55 -0600 Subject: [PATCH 2/4] Add changelog --- changelog/unreleased/pull-4998 | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 changelog/unreleased/pull-4998 diff --git a/changelog/unreleased/pull-4998 b/changelog/unreleased/pull-4998 new file mode 100644 index 000000000..23ff3dbd2 --- /dev/null +++ b/changelog/unreleased/pull-4998 @@ -0,0 +1,8 @@ +Bugfix: Fix extended attributes handling for VSS snapshots + +Restic was failing to backup extended attributes for VSS snapshots +after the fix for https://github.com/restic/restic/pull/4980. +Restic now correctly handles extended attributes for VSS snapshots. + +https://github.com/restic/restic/pull/4998 +https://github.com/restic/restic/pull/4980 From 849c4414552528d326c7adc053670abbfd820485 Mon Sep 17 00:00:00 2001 From: aneesh-n <99904+aneesh-n@users.noreply.github.com> Date: Sun, 11 Aug 2024 01:48:25 -0600 Subject: [PATCH 3/4] Gracefully handle invalid prepared volume names --- internal/restic/node_windows.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/internal/restic/node_windows.go b/internal/restic/node_windows.go index 6adb51f0d..2ca7e42e6 100644 --- a/internal/restic/node_windows.go +++ b/internal/restic/node_windows.go @@ -417,11 +417,13 @@ func checkAndStoreEASupport(path string) (isEASupportedVolume bool, err error) { // First check if the manually prepared volume name is already in the map eaSupportedValue, exists := eaSupportedVolumesMap.Load(volumeName) if exists { + // Cache hit, immediately return the cached value return eaSupportedValue.(bool), nil } // If not found, check if EA is supported with manually prepared volume name isEASupportedVolume, err = fs.PathSupportsExtendedAttributes(volumeName + `\`) - if err != nil { + // If the prepared volume name is not valid, we will next fetch the actual volume name. + if err != nil && !errors.Is(err, windows.DNS_ERROR_INVALID_NAME) { return false, err } } @@ -434,6 +436,7 @@ func checkAndStoreEASupport(path string) (isEASupportedVolume bool, err error) { // If the actual volume name is different, check cache for the actual volume name eaSupportedValue, exists := eaSupportedVolumesMap.Load(volumeNameActual) if exists { + // Cache hit, immediately return the cached value return eaSupportedValue.(bool), nil } // If the actual volume name is different and is not in the map, again check if the new volume supports extended attributes with the actual volume name From 19f487750ee2d8a19bcc4c8be9963ec1b97210af Mon Sep 17 00:00:00 2001 From: aneesh-n <99904+aneesh-n@users.noreply.github.com> Date: Sun, 11 Aug 2024 19:25:58 -0600 Subject: [PATCH 4/4] Add test cases and handle volume GUID paths Gracefully handle errors while checking for EA and add debug logs. --- changelog/unreleased/pull-4980 | 1 + changelog/unreleased/pull-4998 | 8 -- internal/fs/ea_windows_test.go | 76 +++++++++++ internal/restic/node_windows.go | 49 ++++--- internal/restic/node_windows_test.go | 196 +++++++++++++++++++++++++++ 5 files changed, 306 insertions(+), 24 deletions(-) delete mode 100644 changelog/unreleased/pull-4998 diff --git a/changelog/unreleased/pull-4980 b/changelog/unreleased/pull-4980 index 264f347fa..5713db7a2 100644 --- a/changelog/unreleased/pull-4980 +++ b/changelog/unreleased/pull-4980 @@ -8,5 +8,6 @@ Restic now completely skips the attempt to fetch extended attributes for such volumes where it is not supported. https://github.com/restic/restic/pull/4980 +https://github.com/restic/restic/pull/4998 https://github.com/restic/restic/issues/4955 https://github.com/restic/restic/issues/4950 diff --git a/changelog/unreleased/pull-4998 b/changelog/unreleased/pull-4998 deleted file mode 100644 index 23ff3dbd2..000000000 --- a/changelog/unreleased/pull-4998 +++ /dev/null @@ -1,8 +0,0 @@ -Bugfix: Fix extended attributes handling for VSS snapshots - -Restic was failing to backup extended attributes for VSS snapshots -after the fix for https://github.com/restic/restic/pull/4980. -Restic now correctly handles extended attributes for VSS snapshots. - -https://github.com/restic/restic/pull/4998 -https://github.com/restic/restic/pull/4980 diff --git a/internal/fs/ea_windows_test.go b/internal/fs/ea_windows_test.go index b249f43c4..74afd7aa5 100644 --- a/internal/fs/ea_windows_test.go +++ b/internal/fs/ea_windows_test.go @@ -10,6 +10,7 @@ import ( "os" "path/filepath" "reflect" + "strings" "syscall" "testing" "unsafe" @@ -245,3 +246,78 @@ func testSetGetEA(t *testing.T, path string, handle windows.Handle, testEAs []Ex t.Fatalf("EAs read from path %s don't match", path) } } + +func TestPathSupportsExtendedAttributes(t *testing.T) { + testCases := []struct { + name string + path string + expected bool + }{ + { + name: "System drive", + path: os.Getenv("SystemDrive") + `\`, + expected: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + supported, err := PathSupportsExtendedAttributes(tc.path) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + if supported != tc.expected { + t.Errorf("Expected %v, got %v for path %s", tc.expected, supported, tc.path) + } + }) + } + + // Test with an invalid path + _, err := PathSupportsExtendedAttributes("Z:\\NonExistentPath-UAS664da5s4dyu56das45f5as") + if err == nil { + t.Error("Expected an error for non-existent path, but got nil") + } +} + +func TestGetVolumePathName(t *testing.T) { + tempDirVolume := filepath.VolumeName(os.TempDir()) + testCases := []struct { + name string + path string + expectedPrefix string + }{ + { + name: "Root directory", + path: os.Getenv("SystemDrive") + `\`, + expectedPrefix: os.Getenv("SystemDrive"), + }, + { + name: "Nested directory", + path: os.Getenv("SystemDrive") + `\Windows\System32`, + expectedPrefix: os.Getenv("SystemDrive"), + }, + { + name: "Temp directory", + path: os.TempDir() + `\`, + expectedPrefix: tempDirVolume, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + volumeName, err := GetVolumePathName(tc.path) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + if !strings.HasPrefix(volumeName, tc.expectedPrefix) { + t.Errorf("Expected volume name to start with %s, but got %s", tc.expectedPrefix, volumeName) + } + }) + } + + // Test with an invalid path + _, err := GetVolumePathName("Z:\\NonExistentPath") + if err == nil { + t.Error("Expected an error for non-existent path, but got nil") + } +} diff --git a/internal/restic/node_windows.go b/internal/restic/node_windows.go index 2ca7e42e6..bce01ccad 100644 --- a/internal/restic/node_windows.go +++ b/internal/restic/node_windows.go @@ -42,6 +42,7 @@ const ( extendedPathPrefix = `\\?\` uncPathPrefix = `\\?\UNC\` globalRootPrefix = `\\?\GLOBALROOT\` + volumeGUIDPrefix = `\\?\Volume{` ) // mknod is not supported on Windows. @@ -422,15 +423,21 @@ func checkAndStoreEASupport(path string) (isEASupportedVolume bool, err error) { } // If not found, check if EA is supported with manually prepared volume name isEASupportedVolume, err = fs.PathSupportsExtendedAttributes(volumeName + `\`) - // If the prepared volume name is not valid, we will next fetch the actual volume name. + // If the prepared volume name is not valid, we will fetch the actual volume name next. if err != nil && !errors.Is(err, windows.DNS_ERROR_INVALID_NAME) { - return false, err + debug.Log("Error checking if extended attributes are supported for prepared volume name %s: %v", volumeName, err) + // There can be multiple errors like path does not exist, bad network path, etc. + // We just gracefully disallow extended attributes for cases. + return false, nil } } // If an entry is not found, get the actual volume name using the GetVolumePathName function volumeNameActual, err := fs.GetVolumePathName(path) if err != nil { - return false, err + debug.Log("Error getting actual volume name %s for path %s: %v", volumeName, path, err) + // There can be multiple errors like path does not exist, bad network path, etc. + // We just gracefully disallow extended attributes for cases. + return false, nil } if volumeNameActual != volumeName { // If the actual volume name is different, check cache for the actual volume name @@ -441,11 +448,19 @@ func checkAndStoreEASupport(path string) (isEASupportedVolume bool, err error) { } // If the actual volume name is different and is not in the map, again check if the new volume supports extended attributes with the actual volume name isEASupportedVolume, err = fs.PathSupportsExtendedAttributes(volumeNameActual + `\`) + // Debug log for cases where the prepared volume name is not valid if err != nil { - return false, err + debug.Log("Error checking if extended attributes are supported for actual volume name %s: %v", volumeNameActual, err) + // There can be multiple errors like path does not exist, bad network path, etc. + // We just gracefully disallow extended attributes for cases. + return false, nil + } else { + debug.Log("Checking extended attributes. Prepared volume name: %s, actual volume name: %s, isEASupportedVolume: %v, err: %v", volumeName, volumeNameActual, isEASupportedVolume, err) } } - eaSupportedVolumesMap.Store(volumeNameActual, isEASupportedVolume) + if volumeNameActual != "" { + eaSupportedVolumesMap.Store(volumeNameActual, isEASupportedVolume) + } return isEASupportedVolume, err } @@ -460,17 +475,19 @@ func prepareVolumeName(path string) (volumeName string, err error) { volumeName = filepath.VolumeName(path) } } else { - if strings.HasPrefix(path, uncPathPrefix) { - // Convert \\?\UNC\ extended path to standard path to get the volume name correctly - path = `\\` + path[len(uncPathPrefix):] - } else if strings.HasPrefix(path, extendedPathPrefix) { - //Extended length path prefix needs to be trimmed to get the volume name correctly - path = path[len(extendedPathPrefix):] - } else { - // Use the absolute path - path, err = filepath.Abs(path) - if err != nil { - return "", fmt.Errorf("failed to get absolute path: %w", err) + if !strings.HasPrefix(path, volumeGUIDPrefix) { // Handle volume GUID path + if strings.HasPrefix(path, uncPathPrefix) { + // Convert \\?\UNC\ extended path to standard path to get the volume name correctly + path = `\\` + path[len(uncPathPrefix):] + } else if strings.HasPrefix(path, extendedPathPrefix) { + //Extended length path prefix needs to be trimmed to get the volume name correctly + path = path[len(extendedPathPrefix):] + } else { + // Use the absolute path + path, err = filepath.Abs(path) + if err != nil { + return "", fmt.Errorf("failed to get absolute path: %w", err) + } } } volumeName = filepath.VolumeName(path) diff --git a/internal/restic/node_windows_test.go b/internal/restic/node_windows_test.go index 4fd57bbb7..6ba25559b 100644 --- a/internal/restic/node_windows_test.go +++ b/internal/restic/node_windows_test.go @@ -12,6 +12,7 @@ import ( "strings" "syscall" "testing" + "time" "github.com/restic/restic/internal/errors" "github.com/restic/restic/internal/fs" @@ -329,3 +330,198 @@ func TestRestoreExtendedAttributes(t *testing.T) { } } } + +func TestPrepareVolumeName(t *testing.T) { + currentVolume := filepath.VolumeName(func() string { + // Get the current working directory + pwd, err := os.Getwd() + if err != nil { + t.Fatalf("Failed to get current working directory: %v", err) + } + return pwd + }()) + // Create a temporary directory for the test + tempDir, err := os.MkdirTemp("", "restic_test_"+time.Now().Format("20060102150405")) + if err != nil { + t.Fatalf("Failed to create temp directory: %v", err) + } + defer os.RemoveAll(tempDir) + + // Create a long file name + longFileName := `\Very\Long\Path\That\Exceeds\260\Characters\` + strings.Repeat(`\VeryLongFolderName`, 20) + `\\LongFile.txt` + longFilePath := filepath.Join(tempDir, longFileName) + + tempDirVolume := filepath.VolumeName(tempDir) + // Create the file + content := []byte("This is a test file with a very long name.") + err = os.MkdirAll(filepath.Dir(longFilePath), 0755) + test.OK(t, err) + if err != nil { + t.Fatalf("Failed to create long folder: %v", err) + } + err = os.WriteFile(longFilePath, content, 0644) + test.OK(t, err) + if err != nil { + t.Fatalf("Failed to create long file: %v", err) + } + osVolumeGUIDPath := getOSVolumeGUIDPath(t) + osVolumeGUIDVolume := filepath.VolumeName(osVolumeGUIDPath) + + testCases := []struct { + name string + path string + expectedVolume string + expectError bool + expectedEASupported bool + isRealPath bool + }{ + { + name: "Network drive path", + path: `Z:\Shared\Documents`, + expectedVolume: `Z:`, + expectError: false, + expectedEASupported: false, + }, + { + name: "Subst drive path", + path: `X:\Virtual\Folder`, + expectedVolume: `X:`, + expectError: false, + expectedEASupported: false, + }, + { + name: "Windows reserved path", + path: `\\.\` + os.Getenv("SystemDrive") + `\System32\drivers\etc\hosts`, + expectedVolume: `\\.\` + os.Getenv("SystemDrive"), + expectError: false, + expectedEASupported: true, + isRealPath: true, + }, + { + name: "Long UNC path", + path: `\\?\UNC\LongServerName\VeryLongShareName\DeepPath\File.txt`, + expectedVolume: `\\LongServerName\VeryLongShareName`, + expectError: false, + expectedEASupported: false, + }, + { + name: "Volume GUID path", + path: osVolumeGUIDPath, + expectedVolume: osVolumeGUIDVolume, + expectError: false, + expectedEASupported: true, + isRealPath: true, + }, + { + name: "Volume GUID path with subfolder", + path: osVolumeGUIDPath + `\Windows`, + expectedVolume: osVolumeGUIDVolume, + expectError: false, + expectedEASupported: true, + isRealPath: true, + }, + { + name: "Standard path", + path: os.Getenv("SystemDrive") + `\Users\`, + expectedVolume: os.Getenv("SystemDrive"), + expectError: false, + expectedEASupported: true, + isRealPath: true, + }, + { + name: "Extended length path", + path: longFilePath, + expectedVolume: tempDirVolume, + expectError: false, + expectedEASupported: true, + isRealPath: true, + }, + { + name: "UNC path", + path: `\\server\share\folder`, + expectedVolume: `\\server\share`, + expectError: false, + expectedEASupported: false, + }, + { + name: "Extended UNC path", + path: `\\?\UNC\server\share\folder`, + expectedVolume: `\\server\share`, + expectError: false, + expectedEASupported: false, + }, + { + name: "Volume Shadow Copy path", + path: `\\?\GLOBALROOT\Device\HarddiskVolumeShadowCopy1\Users\test`, + expectedVolume: `\\?\GLOBALROOT\Device\HarddiskVolumeShadowCopy1`, + expectError: false, + expectedEASupported: false, + }, + { + name: "Relative path", + path: `folder\subfolder`, + + expectedVolume: currentVolume, // Get current volume + expectError: false, + expectedEASupported: true, + }, + { + name: "Empty path", + path: ``, + expectedVolume: currentVolume, + expectError: false, + expectedEASupported: true, + isRealPath: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + isEASupported, err := checkAndStoreEASupport(tc.path) + test.OK(t, err) + test.Equals(t, tc.expectedEASupported, isEASupported) + + volume, err := prepareVolumeName(tc.path) + + if tc.expectError { + test.Assert(t, err != nil, "Expected an error, but got none") + } else { + test.OK(t, err) + } + test.Equals(t, tc.expectedVolume, volume) + + if tc.isRealPath { + isEASupportedVolume, err := fs.PathSupportsExtendedAttributes(volume + `\`) + // If the prepared volume name is not valid, we will next fetch the actual volume name. + test.OK(t, err) + + test.Equals(t, tc.expectedEASupported, isEASupportedVolume) + + actualVolume, err := fs.GetVolumePathName(tc.path) + test.OK(t, err) + test.Equals(t, tc.expectedVolume, actualVolume) + } + }) + } +} + +func getOSVolumeGUIDPath(t *testing.T) string { + // Get the path of the OS drive (usually C:\) + osDrive := os.Getenv("SystemDrive") + "\\" + + // Convert to a volume GUID path + volumeName, err := windows.UTF16PtrFromString(osDrive) + test.OK(t, err) + if err != nil { + return "" + } + + var volumeGUID [windows.MAX_PATH]uint16 + err = windows.GetVolumeNameForVolumeMountPoint(volumeName, &volumeGUID[0], windows.MAX_PATH) + test.OK(t, err) + if err != nil { + return "" + } + + return windows.UTF16ToString(volumeGUID[:]) +}