From f93a7b8457bf48660b836711051e316a97c7edc5 Mon Sep 17 00:00:00 2001 From: Aliaksandr Valialkin Date: Mon, 18 Sep 2023 16:10:16 +0200 Subject: [PATCH] lib/backup/common: consistently use canonical path with / directory separators at Part.Path Previously Part.Path could contain `\` directory separators on Windows OS, which could result in incorrect filepaths generation when making backups at object storage. Updates https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4704 This is a follow-up for f2df8ad480c649d25edc93d10836a2d8bfda5591 --- lib/backup/common/part.go | 23 +++++++++++++++++++ lib/backup/fscommon/fscommon.go | 5 ++-- lib/backup/fslocal/fslocal.go | 9 ++++---- lib/backup/fsremote/fsremote.go | 11 +++------ lib/backup/fsremote/normilize_path_bsd.go | 12 ---------- lib/backup/fsremote/normilize_path_darwin.go | 9 -------- lib/backup/fsremote/normilize_path_linux.go | 9 -------- lib/backup/fsremote/normilize_path_solaris.go | 9 -------- lib/backup/fsremote/normilize_path_windows.go | 11 --------- 9 files changed, 34 insertions(+), 64 deletions(-) delete mode 100644 lib/backup/fsremote/normilize_path_bsd.go delete mode 100644 lib/backup/fsremote/normilize_path_darwin.go delete mode 100644 lib/backup/fsremote/normilize_path_linux.go delete mode 100644 lib/backup/fsremote/normilize_path_solaris.go delete mode 100644 lib/backup/fsremote/normilize_path_windows.go diff --git a/lib/backup/common/part.go b/lib/backup/common/part.go index 72202472d..41e72d856 100644 --- a/lib/backup/common/part.go +++ b/lib/backup/common/part.go @@ -2,6 +2,7 @@ package common import ( "fmt" + "path/filepath" "regexp" "sort" "strconv" @@ -15,6 +16,9 @@ import ( // Each source file can be split into parts with up to MaxPartSize sizes. type Part struct { // Path is the path to file for backup. + // + // Path must consistently use `/` as directory separator. + // Use ToCanonicalPath() function for converting local directory separators to `/`. Path string // FileSize is the size of the whole file for the given part. @@ -51,11 +55,30 @@ func (p *Part) RemotePath(prefix string) string { return fmt.Sprintf("%s/%s/%016X_%016X_%016X", prefix, p.Path, p.FileSize, p.Offset, p.Size) } +// LocalPath returns local path for p at the given dir. +func (p *Part) LocalPath(dir string) string { + path := p.Path + if filepath.Separator != '/' { + path = strings.ReplaceAll(path, "/", string(filepath.Separator)) + } + return filepath.Join(dir, path) +} + +// ToCanonicalPath returns canonical path by replacing local directory separators with `/`. +func ToCanonicalPath(path string) string { + if filepath.Separator == '/' { + return path + } + return strings.ReplaceAll(path, string(filepath.Separator), "/") +} + var partNameRegexp = regexp.MustCompile(`^(.+)[/\\]([0-9A-F]{16})_([0-9A-F]{16})_([0-9A-F]{16})$`) // ParseFromRemotePath parses p from remotePath. // // Returns true on success. +// +// remotePath must be in canonical form received from ToCanonicalPath(). func (p *Part) ParseFromRemotePath(remotePath string) bool { tmp := partNameRegexp.FindStringSubmatch(remotePath) if len(tmp) != 5 { diff --git a/lib/backup/fscommon/fscommon.go b/lib/backup/fscommon/fscommon.go index 0ba3e856a..3d52bbcc9 100644 --- a/lib/backup/fscommon/fscommon.go +++ b/lib/backup/fscommon/fscommon.go @@ -10,9 +10,10 @@ import ( "github.com/VictoriaMetrics/VictoriaMetrics/lib/logger" ) -// AppendFiles appends all the files from dir to dst. +// AppendFiles appends paths to all the files from local dir to dst. // -// All the appended files will have dir prefix. +// All the appended filepaths will have dir prefix. +// The returned paths have local OS-specific directory separators. func AppendFiles(dst []string, dir string) ([]string, error) { d, err := os.Open(dir) if err != nil { diff --git a/lib/backup/fslocal/fslocal.go b/lib/backup/fslocal/fslocal.go index 1136cee74..d9d853da4 100644 --- a/lib/backup/fslocal/fslocal.go +++ b/lib/backup/fslocal/fslocal.go @@ -77,7 +77,7 @@ func (fs *FS) ListParts() ([]common.Part, error) { if err != nil { return nil, fmt.Errorf("cannot stat %q: %w", file, err) } - path := file[len(dir):] + path := common.ToCanonicalPath(file[len(dir):]) size := uint64(fi.Size()) if size == 0 { parts = append(parts, common.Part{ @@ -146,8 +146,9 @@ func (fs *FS) NewWriteCloser(p common.Part) (io.WriteCloser, error) { return blwc, nil } -// DeletePath deletes the given path from fs and returns the size -// for the deleted file. +// DeletePath deletes the given path from fs and returns the size for the deleted file. +// +// The path must be in canonical form, e.g. it must have `/` directory separators func (fs *FS) DeletePath(path string) (uint64, error) { p := common.Part{ Path: path, @@ -187,7 +188,7 @@ func (fs *FS) mkdirAll(filePath string) error { } func (fs *FS) path(p common.Part) string { - return filepath.Join(fs.Dir, p.Path) + return p.LocalPath(fs.Dir) } type limitedReadCloser struct { diff --git a/lib/backup/fsremote/fsremote.go b/lib/backup/fsremote/fsremote.go index cd2d81014..f52442adb 100644 --- a/lib/backup/fsremote/fsremote.go +++ b/lib/backup/fsremote/fsremote.go @@ -58,7 +58,8 @@ func (fs *FS) ListParts() ([]common.Part, error) { continue } var p common.Part - if !p.ParseFromRemotePath(file[len(dir):]) { + remotePath := common.ToCanonicalPath(file[len(dir):]) + if !p.ParseFromRemotePath(remotePath) { logger.Infof("skipping unknown file %s", file) continue } @@ -68,7 +69,6 @@ func (fs *FS) ListParts() ([]common.Part, error) { return nil, fmt.Errorf("cannot stat file %q for part %q: %w", file, p.Path, err) } p.ActualSize = uint64(fi.Size()) - p.Path = pathToCanonical(p.Path) parts = append(parts, p) } return parts, nil @@ -76,7 +76,6 @@ func (fs *FS) ListParts() ([]common.Part, error) { // DeletePart deletes the given part p from fs. func (fs *FS) DeletePart(p common.Part) error { - p.Path = canonicalPathToLocal(p.Path) path := fs.path(p) if err := os.Remove(path); err != nil { return fmt.Errorf("cannot remove %q: %w", path, err) @@ -97,7 +96,6 @@ func (fs *FS) CopyPart(srcFS common.OriginFS, p common.Part) error { if !ok { return fmt.Errorf("cannot perform server-side copying from %s to %s: both of them must be fsremote", srcFS, fs) } - p.Path = canonicalPathToLocal(p.Path) srcPath := src.path(p) dstPath := fs.path(p) if err := fs.mkdirAll(dstPath); err != nil { @@ -142,7 +140,6 @@ func (fs *FS) CopyPart(srcFS common.OriginFS, p common.Part) error { // DownloadPart download part p from fs to w. func (fs *FS) DownloadPart(p common.Part, w io.Writer) error { - p.Path = canonicalPathToLocal(p.Path) path := fs.path(p) r, err := os.Open(path) if err != nil { @@ -198,14 +195,13 @@ func (fs *FS) mkdirAll(filePath string) error { } func (fs *FS) path(p common.Part) string { - return filepath.Join(fs.Dir, p.Path, fmt.Sprintf("%016X_%016X_%016X", p.FileSize, p.Offset, p.Size)) + return filepath.Join(p.LocalPath(fs.Dir), fmt.Sprintf("%016X_%016X_%016X", p.FileSize, p.Offset, p.Size)) } // DeleteFile deletes filePath at fs. // // The function does nothing if the filePath doesn't exist. func (fs *FS) DeleteFile(filePath string) error { - filePath = canonicalPathToLocal(filePath) path := filepath.Join(fs.Dir, filePath) err := os.Remove(path) if err != nil && !os.IsNotExist(err) { @@ -247,6 +243,5 @@ func (fs *FS) HasFile(filePath string) (bool, error) { // ReadFile returns the content of filePath at fs. func (fs *FS) ReadFile(filePath string) ([]byte, error) { path := filepath.Join(fs.Dir, filePath) - return os.ReadFile(path) } diff --git a/lib/backup/fsremote/normilize_path_bsd.go b/lib/backup/fsremote/normilize_path_bsd.go deleted file mode 100644 index 4fb42fee9..000000000 --- a/lib/backup/fsremote/normilize_path_bsd.go +++ /dev/null @@ -1,12 +0,0 @@ -//go:build freebsd || openbsd || dragonfly || netbsd -// +build freebsd openbsd dragonfly netbsd - -package fsremote - -func pathToCanonical(path string) string { - return path -} - -func canonicalPathToLocal(path string) string { - return path -} diff --git a/lib/backup/fsremote/normilize_path_darwin.go b/lib/backup/fsremote/normilize_path_darwin.go deleted file mode 100644 index 5e9590684..000000000 --- a/lib/backup/fsremote/normilize_path_darwin.go +++ /dev/null @@ -1,9 +0,0 @@ -package fsremote - -func pathToCanonical(path string) string { - return path -} - -func canonicalPathToLocal(path string) string { - return path -} diff --git a/lib/backup/fsremote/normilize_path_linux.go b/lib/backup/fsremote/normilize_path_linux.go deleted file mode 100644 index 5e9590684..000000000 --- a/lib/backup/fsremote/normilize_path_linux.go +++ /dev/null @@ -1,9 +0,0 @@ -package fsremote - -func pathToCanonical(path string) string { - return path -} - -func canonicalPathToLocal(path string) string { - return path -} diff --git a/lib/backup/fsremote/normilize_path_solaris.go b/lib/backup/fsremote/normilize_path_solaris.go deleted file mode 100644 index 5e9590684..000000000 --- a/lib/backup/fsremote/normilize_path_solaris.go +++ /dev/null @@ -1,9 +0,0 @@ -package fsremote - -func pathToCanonical(path string) string { - return path -} - -func canonicalPathToLocal(path string) string { - return path -} diff --git a/lib/backup/fsremote/normilize_path_windows.go b/lib/backup/fsremote/normilize_path_windows.go deleted file mode 100644 index c1dfe71af..000000000 --- a/lib/backup/fsremote/normilize_path_windows.go +++ /dev/null @@ -1,11 +0,0 @@ -package fsremote - -import "strings" - -func pathToCanonical(path string) string { - return strings.ReplaceAll(path, "\\", "/") -} - -func canonicalPathToLocal(path string) string { - return strings.ReplaceAll(path, "/", "\\") -}