From 59ecedacaf8d7273e374da4330d1b2f606af73e4 Mon Sep 17 00:00:00 2001 From: Aneesh Nireshwalia Date: Tue, 31 Jan 2023 17:35:18 -0700 Subject: [PATCH] Fix review comments for temp file creation Add rclone copyright notice for smb files. Change temp file creation code to match sftp code. Remove fastrand dependency. --- go.mod | 1 - go.sum | 2 - internal/backend/smb/conpool.go | 22 +++++- internal/backend/smb/smb.go | 129 +++++++++----------------------- 4 files changed, 58 insertions(+), 96 deletions(-) diff --git a/go.mod b/go.mod index ae7511074..76beb6846 100644 --- a/go.mod +++ b/go.mod @@ -26,7 +26,6 @@ require ( github.com/restic/chunker v0.4.0 github.com/spf13/cobra v1.6.1 github.com/spf13/pflag v1.0.5 - github.com/valyala/fastrand v1.1.0 golang.org/x/crypto v0.5.0 golang.org/x/net v0.5.0 golang.org/x/oauth2 v0.4.0 diff --git a/go.sum b/go.sum index 1a12f025c..3561bbdf8 100644 --- a/go.sum +++ b/go.sum @@ -169,8 +169,6 @@ github.com/stretchr/testify v1.8.1 h1:w7B6lhMri9wdJUVmEZPGGhZzrYTPvgJArz7wNPgYKs github.com/stretchr/testify v1.8.1/go.mod h1:w2LPCIKwWwSfY2zedu0+kehJoqGctiVI29o6fzry7u4= github.com/tv42/httpunix v0.0.0-20191220191345-2ba4b9c3382c h1:u6SKchux2yDvFQnDHS3lPnIRmfVJ5Sxy3ao2SIdysLQ= github.com/tv42/httpunix v0.0.0-20191220191345-2ba4b9c3382c/go.mod h1:hzIxponao9Kjc7aWznkXaL4U4TWaDSs8zcsY4Ka08nM= -github.com/valyala/fastrand v1.1.0 h1:f+5HkLW4rsgzdNoleUOB69hyT9IlD2ZQh9GyDMfb5G8= -github.com/valyala/fastrand v1.1.0/go.mod h1:HWqCzkrkg6QXT8V2EXWvXCoow7vLwOFN002oeRzjapQ= github.com/yuin/goldmark v1.1.27/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74= go.opencensus.io v0.24.0 h1:y73uSU6J157QMP2kn2r30vwW1A2W2WFwSCGnAVxeaD0= go.opencensus.io v0.24.0/go.mod h1:vNK8G9p7aAivkbmorf4v+7Hgx+Zs0yY+0fOtgBfjQKo= diff --git a/internal/backend/smb/conpool.go b/internal/backend/smb/conpool.go index b6cbbf769..5ab981f7d 100644 --- a/internal/backend/smb/conpool.go +++ b/internal/backend/smb/conpool.go @@ -1,3 +1,23 @@ +// Parts of this code have been copied from Rclone (https://github.com/rclone) +// Copyright (C) 2012 by Nick Craig-Wood http://www.craig-wood.com/nick/ + +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: + +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. + +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. package smb import ( @@ -63,7 +83,7 @@ func (b *Backend) getSessions() int32 { // dial starts a client connection to the given SMB server. It is a // convenience function that connects to the given network address, -// initiates the SMB handshake, and then sets up a Client. +// initiates the SMB handshake, and then returns a session for SMB communication. func (b *Backend) dial(ctx context.Context, network, addr string) (*conn, error) { dialer := net.Dialer{} tconn, err := dialer.Dial(network, addr) diff --git a/internal/backend/smb/smb.go b/internal/backend/smb/smb.go index b8f8ab2a1..a2a1d9f03 100644 --- a/internal/backend/smb/smb.go +++ b/internal/backend/smb/smb.go @@ -1,14 +1,34 @@ +// Parts of this code have been copied from Rclone (https://github.com/rclone) +// Copyright (C) 2012 by Nick Craig-Wood http://www.craig-wood.com/nick/ + +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: + +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. + +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. package smb import ( "context" + "crypto/rand" + "encoding/hex" "hash" "io" - "io/fs" "os" "path" "path/filepath" - "strconv" "sync" "syscall" "time" @@ -21,8 +41,6 @@ import ( "github.com/restic/restic/internal/debug" "github.com/restic/restic/internal/errors" "github.com/restic/restic/internal/restic" - - "github.com/valyala/fastrand" ) // Backend stores data on an SMB endpoint. @@ -160,8 +178,9 @@ func (b *Backend) Save(ctx context.Context, h restic.Handle, rd restic.RewindRea return backoff.Permanent(err) } - finalname := b.Filename(h) - dir := filepath.Dir(finalname) + filename := b.Filename(h) + tmpFilename := filename + "-restic-temp-" + tempSuffix() + dir := filepath.Dir(tmpFilename) defer func() { // Mark non-retriable errors as such @@ -173,9 +192,6 @@ func (b *Backend) Save(ctx context.Context, h restic.Handle, rd restic.RewindRea b.sem.GetToken() defer b.sem.ReleaseToken() - // Create new file with a temporary name. - tmpname := filepath.Base(finalname) + "-tmp-" - b.addSession() // Show session in use defer b.removeSession() @@ -185,7 +201,8 @@ func (b *Backend) Save(ctx context.Context, h restic.Handle, rd restic.RewindRea } defer b.putConnection(&cn) - f, err := b.CreateTemp(cn, dir, tmpname) + // create new file + f, err := cn.smbShare.OpenFile(tmpFilename, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0600) if b.IsNotExist(err) { debug.Log("error %v: creating dir", err) @@ -196,7 +213,7 @@ func (b *Backend) Save(ctx context.Context, h restic.Handle, rd restic.RewindRea debug.Log("error creating dir %v: %v", dir, mkdirErr) } else { // try again - f, err = b.CreateTemp(cn, dir, tmpname) + f, err = cn.smbShare.OpenFile(tmpFilename, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0600) } } @@ -237,14 +254,14 @@ func (b *Backend) Save(ctx context.Context, h restic.Handle, rd restic.RewindRea if err = f.Close(); err != nil { return errors.WithStack(err) } - if err = cn.smbShare.Rename(f.Name(), finalname); err != nil { + if err = cn.smbShare.Rename(f.Name(), filename); err != nil { return errors.WithStack(err) } // try to mark file as read-only to avoid accidential modifications // ignore if the operation fails as some filesystems don't allow the chmod call // e.g. exfat and network file systems with certain mount options - err = cn.setFileReadonly(finalname, b.Modes.File) + err = cn.setFileReadonly(filename, b.Modes.File) if err != nil && !os.IsPermission(err) { return errors.WithStack(err) } @@ -473,85 +490,13 @@ func (b *Backend) Close() error { return err } -const ( - pathSeparator = '/' // Always using '/' for SMB -) - -// CreateTemp creates a new temporary file in the directory dir, -// opens the file for reading and writing, and returns the resulting file. -// The filename is generated by taking pattern and adding a random string to the end. -// If pattern includes a "*", the random string replaces the last "*". -// If dir is the empty string, CreateTemp uses the default directory for temporary files, as returned by TempDir. -// Multiple programs or goroutines calling CreateTemp simultaneously will not choose the same file. -// The caller can use the file's Name method to find the pathname of the file. -// It is the caller's responsibility to remove the file when it is no longer needed. -func (b *Backend) CreateTemp(cn *conn, dir, pattern string) (*smb2.File, error) { - if dir == "" { - dir = os.TempDir() - } - - prefix, suffix, err := prefixAndSuffix(pattern) +// tempSuffix generates a random string suffix that should be sufficiently long +// to avoid accidental conflicts. +func tempSuffix() string { + var nonce [16]byte + _, err := rand.Read(nonce[:]) if err != nil { - return nil, &fs.PathError{Op: "createtemp", Path: pattern, Err: err} - } - prefix = joinPath(dir, prefix) - - try := 0 - for { - name := prefix + nextRandom() + suffix - f, err := cn.smbShare.OpenFile(name, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0600) - - if os.IsExist(err) { - if try++; try < 10000 { - continue - } - return nil, &fs.PathError{Op: "createtemp", Path: prefix + "*" + suffix, Err: fs.ErrExist} - } - return f, err + panic(err) } -} - -var errPatternHasSeparator = errors.New("pattern contains path separator") - -// prefixAndSuffix splits pattern by the last wildcard "*", if applicable, -// returning prefix as the part before "*" and suffix as the part after "*". -func prefixAndSuffix(pattern string) (prefix, suffix string, err error) { - for i := 0; i < len(pattern); i++ { - if IsPathSeparator(pattern[i]) { - return "", "", errPatternHasSeparator - } - } - if pos := lastIndex(pattern, '*'); pos != -1 { - prefix, suffix = pattern[:pos], pattern[pos+1:] - } else { - prefix = pattern - } - return prefix, suffix, nil -} - -// LastIndexByte from the strings package. -func lastIndex(s string, sep byte) int { - for i := len(s) - 1; i >= 0; i-- { - if s[i] == sep { - return i - } - } - return -1 -} - -func nextRandom() string { - return strconv.FormatUint(uint64(fastrand.Uint32()), 10) -} - -func joinPath(dir, name string) string { - if len(dir) > 0 && IsPathSeparator(dir[len(dir)-1]) { - return dir + name - } - return dir + string(pathSeparator) + name -} - -// IsPathSeparator reports whether c is a directory separator character. -func IsPathSeparator(c uint8) bool { - // NOTE: Windows accepts / as path separator. - return c == '\\' || c == '/' + return hex.EncodeToString(nonce[:]) }