diff --git a/feature/taildrop/delete.go b/feature/taildrop/delete.go index e9c8d7f1c..0b7259879 100644 --- a/feature/taildrop/delete.go +++ b/feature/taildrop/delete.go @@ -6,9 +6,7 @@ import ( "container/list" "context" - "io/fs" "os" - "path/filepath" "strings" "sync" "time" @@ -28,7 +26,6 @@ type fileDeleter struct { logf logger.Logf clock tstime.DefaultClock - dir string event func(string) // called for certain events; for testing only mu sync.Mutex @@ -39,6 +36,7 @@ type fileDeleter struct { group syncs.WaitGroup shutdownCtx context.Context shutdown context.CancelFunc + fs FileOps // must be used for all filesystem operations } // deleteFile is a specific file to delete after deleteDelay. @@ -50,15 +48,14 @@ type deleteFile struct { func (d *fileDeleter) Init(m *manager, eventHook func(string)) { d.logf = m.opts.Logf d.clock = m.opts.Clock - d.dir = m.opts.Dir d.event = eventHook + d.fs = m.opts.fileOps d.byName = make(map[string]*list.Element) d.emptySignal = make(chan struct{}) d.shutdownCtx, d.shutdown = context.WithCancel(context.Background()) // From a cold-start, load the list of partial and deleted files. - // // Only run this if we have ever received at least one file // to avoid ever touching the taildrop directory on systems (e.g., MacOS) // that pop up a security dialog window upon first access. @@ -71,38 +68,45 @@ func (d *fileDeleter) Init(m *manager, eventHook func(string)) { d.group.Go(func() { d.event("start full-scan") defer d.event("end full-scan") - rangeDir(d.dir, func(de fs.DirEntry) bool { + + if d.fs == nil { + d.logf("deleter: nil FileOps") + } + + files, err := d.fs.ListFiles() + if err != nil { + d.logf("deleter: ListDir error: %v", err) + return + } + for _, filename := range files { switch { case d.shutdownCtx.Err() != nil: - return false // terminate early - case !de.Type().IsRegular(): - return true - case strings.HasSuffix(de.Name(), partialSuffix): + return // terminate early + case strings.HasSuffix(filename, partialSuffix): // Only enqueue the file for deletion if there is no active put. - nameID := strings.TrimSuffix(de.Name(), partialSuffix) + nameID := strings.TrimSuffix(filename, partialSuffix) if i := strings.LastIndexByte(nameID, '.'); i > 0 { key := incomingFileKey{clientID(nameID[i+len("."):]), nameID[:i]} m.incomingFiles.LoadFunc(key, func(_ *incomingFile, loaded bool) { if !loaded { - d.Insert(de.Name()) + d.Insert(filename) } }) } else { - d.Insert(de.Name()) + d.Insert(filename) } - case strings.HasSuffix(de.Name(), deletedSuffix): + case strings.HasSuffix(filename, deletedSuffix): // Best-effort immediate deletion of deleted files. - name := strings.TrimSuffix(de.Name(), deletedSuffix) - if os.Remove(filepath.Join(d.dir, name)) == nil { - if os.Remove(filepath.Join(d.dir, de.Name())) == nil { - break + name := strings.TrimSuffix(filename, deletedSuffix) + if d.fs.Remove(name) == nil { + if d.fs.Remove(filename) == nil { + continue } } - // Otherwise, enqueue the file for later deletion. - d.Insert(de.Name()) + // Otherwise enqueue for later deletion. + d.Insert(filename) } - return true - }) + } }) } @@ -149,13 +153,13 @@ func (d *fileDeleter) waitAndDelete(wait time.Duration) { // Delete the expired file. if name, ok := strings.CutSuffix(file.name, deletedSuffix); ok { - if err := os.Remove(filepath.Join(d.dir, name)); err != nil && !os.IsNotExist(err) { + if err := d.fs.Remove(name); err != nil && !os.IsNotExist(err) { d.logf("could not delete: %v", redactError(err)) failed = append(failed, elem) continue } } - if err := os.Remove(filepath.Join(d.dir, file.name)); err != nil && !os.IsNotExist(err) { + if err := d.fs.Remove(file.name); err != nil && !os.IsNotExist(err) { d.logf("could not delete: %v", redactError(err)) failed = append(failed, elem) continue diff --git a/feature/taildrop/delete_test.go b/feature/taildrop/delete_test.go index 7a58de55c..36950f582 100644 --- a/feature/taildrop/delete_test.go +++ b/feature/taildrop/delete_test.go @@ -5,7 +5,6 @@ import ( "os" - "path/filepath" "slices" "testing" "time" @@ -20,11 +19,20 @@ func TestDeleter(t *testing.T) { dir := t.TempDir() - must.Do(touchFile(filepath.Join(dir, "foo.partial"))) - must.Do(touchFile(filepath.Join(dir, "bar.partial"))) - must.Do(touchFile(filepath.Join(dir, "fizz"))) - must.Do(touchFile(filepath.Join(dir, "fizz.deleted"))) - must.Do(touchFile(filepath.Join(dir, "buzz.deleted"))) // lacks a matching "buzz" file + var m manager + var fd fileDeleter + m.opts.Logf = t.Logf + m.opts.Clock = tstime.DefaultClock{Clock: tstest.NewClock(tstest.ClockOpts{ + Start: time.Date(2000, 1, 1, 0, 0, 0, 0, time.UTC), + })} + m.opts.State = must.Get(mem.New(nil, "")) + m.opts.fileOps, _ = newFileOps(dir) + + must.Do(m.touchFile("foo.partial")) + must.Do(m.touchFile("bar.partial")) + must.Do(m.touchFile("fizz")) + must.Do(m.touchFile("fizz.deleted")) + must.Do(m.touchFile("buzz.deleted")) // lacks a matching "buzz" file checkDirectory := func(want ...string) { t.Helper() @@ -69,12 +77,10 @@ func TestDeleter(t *testing.T) { } eventHook := func(event string) { eventsChan <- event } - var m manager - var fd fileDeleter m.opts.Logf = t.Logf m.opts.Clock = tstime.DefaultClock{Clock: clock} - m.opts.Dir = dir m.opts.State = must.Get(mem.New(nil, "")) + m.opts.fileOps, _ = newFileOps(dir) must.Do(m.opts.State.WriteState(ipn.TaildropReceivedKey, []byte{1})) fd.Init(&m, eventHook) defer fd.Shutdown() @@ -100,17 +106,17 @@ func TestDeleter(t *testing.T) { checkEvents("end waitAndDelete") checkDirectory() - must.Do(touchFile(filepath.Join(dir, "one.partial"))) + must.Do(m.touchFile("one.partial")) insert("one.partial") checkEvents("start waitAndDelete") advance(deleteDelay / 4) - must.Do(touchFile(filepath.Join(dir, "two.partial"))) + must.Do(m.touchFile("two.partial")) insert("two.partial") advance(deleteDelay / 4) - must.Do(touchFile(filepath.Join(dir, "three.partial"))) + must.Do(m.touchFile("three.partial")) insert("three.partial") advance(deleteDelay / 4) - must.Do(touchFile(filepath.Join(dir, "four.partial"))) + must.Do(m.touchFile("four.partial")) insert("four.partial") advance(deleteDelay / 4) @@ -145,8 +151,8 @@ func TestDeleterInitWithoutTaildrop(t *testing.T) { var m manager var fd fileDeleter m.opts.Logf = t.Logf - m.opts.Dir = t.TempDir() m.opts.State = must.Get(mem.New(nil, "")) + m.opts.fileOps, _ = newFileOps(t.TempDir()) fd.Init(&m, func(event string) { t.Errorf("unexpected event: %v", event) }) fd.Shutdown() } diff --git a/feature/taildrop/ext.go b/feature/taildrop/ext.go index c11fe3af4..f8f45b53f 100644 --- a/feature/taildrop/ext.go +++ b/feature/taildrop/ext.go @@ -10,7 +10,6 @@ "fmt" "io" "maps" - "os" "path/filepath" "runtime" "slices" @@ -75,7 +74,7 @@ type Extension struct { // FileOps abstracts platform-specific file operations needed for file transfers. // This is currently being used for Android to use the Storage Access Framework. - FileOps FileOps + fileOps FileOps nodeBackendForTest ipnext.NodeBackend // if non-nil, pretend we're this node state for tests @@ -89,30 +88,6 @@ type Extension struct { outgoingFiles map[string]*ipn.OutgoingFile } -// safDirectoryPrefix is used to determine if the directory is managed via SAF. -const SafDirectoryPrefix = "content://" - -// PutMode controls how Manager.PutFile writes files to storage. -// -// PutModeDirect – write files directly to a filesystem path (default). -// PutModeAndroidSAF – use Android’s Storage Access Framework (SAF), where -// the OS manages the underlying directory permissions. -type PutMode int - -const ( - PutModeDirect PutMode = iota - PutModeAndroidSAF -) - -// FileOps defines platform-specific file operations. -type FileOps interface { - OpenFileWriter(filename string) (io.WriteCloser, string, error) - - // RenamePartialFile finalizes a partial file. - // It returns the new SAF URI as a string and an error. - RenamePartialFile(partialUri, targetDirUri, targetName string) (string, error) -} - func (e *Extension) Name() string { return "taildrop" } @@ -176,23 +151,34 @@ func (e *Extension) onChangeProfile(profile ipn.LoginProfileView, _ ipn.PrefsVie return } - // If we have a netmap, create a taildrop manager. - fileRoot, isDirectFileMode := e.fileRoot(uid, activeLogin) - if fileRoot == "" { - e.logf("no Taildrop directory configured") - } - mode := PutModeDirect - if e.directFileRoot != "" && strings.HasPrefix(e.directFileRoot, SafDirectoryPrefix) { - mode = PutModeAndroidSAF + // Use the provided [FileOps] implementation (typically for SAF access on Android), + // or create an [fsFileOps] instance rooted at fileRoot. + // + // A non-nil [FileOps] also implies that we are in DirectFileMode. + fops := e.fileOps + isDirectFileMode := fops != nil + if fops == nil { + var fileRoot string + if fileRoot, isDirectFileMode = e.fileRoot(uid, activeLogin); fileRoot == "" { + e.logf("no Taildrop directory configured") + e.setMgrLocked(nil) + return + } + + var err error + if fops, err = newFileOps(fileRoot); err != nil { + e.logf("taildrop: cannot create FileOps: %v", err) + e.setMgrLocked(nil) + return + } } + e.setMgrLocked(managerOptions{ Logf: e.logf, Clock: tstime.DefaultClock{Clock: e.sb.Clock()}, State: e.stateStore, - Dir: fileRoot, DirectFileMode: isDirectFileMode, - FileOps: e.FileOps, - Mode: mode, + fileOps: fops, SendFileNotify: e.sendFileNotify, }.New()) } @@ -221,12 +207,7 @@ func (e *Extension) fileRoot(uid tailcfg.UserID, activeLogin string) (root strin baseDir := fmt.Sprintf("%s-uid-%d", strings.ReplaceAll(activeLogin, "@", "-"), uid) - dir := filepath.Join(varRoot, "files", baseDir) - if err := os.MkdirAll(dir, 0700); err != nil { - e.logf("Taildrop disabled; error making directory: %v", err) - return "", false - } - return dir, false + return filepath.Join(varRoot, "files", baseDir), false } // hasCapFileSharing reports whether the current node has the file sharing diff --git a/feature/taildrop/fileops.go b/feature/taildrop/fileops.go new file mode 100644 index 000000000..14f76067a --- /dev/null +++ b/feature/taildrop/fileops.go @@ -0,0 +1,41 @@ +// Copyright (c) Tailscale Inc & AUTHORS +// SPDX-License-Identifier: BSD-3-Clause + +package taildrop + +import ( + "io" + "io/fs" + "os" +) + +// FileOps abstracts over both local‐FS paths and Android SAF URIs. +type FileOps interface { + // OpenWriter creates or truncates a file named relative to the receiver's root, + // seeking to the specified offset. If the file does not exist, it is created with mode perm + // on platforms that support it. + // + // It returns an [io.WriteCloser] and the file's absolute path, or an error. + // This call may block. Callers should avoid holding locks when calling OpenWriter. + OpenWriter(name string, offset int64, perm os.FileMode) (wc io.WriteCloser, path string, err error) + + // Remove deletes a file or directory relative to the receiver's root. + // It returns [io.ErrNotExist] if the file or directory does not exist. + Remove(name string) error + + // Rename atomically renames oldPath to a new file named newName, + // returning the full new path or an error. + Rename(oldPath, newName string) (newPath string, err error) + + // ListFiles returns just the basenames of all regular files + // in the root directory. + ListFiles() ([]string, error) + + // Stat returns the FileInfo for the given name or an error. + Stat(name string) (fs.FileInfo, error) + + // OpenReader opens the given basename for the given name or an error. + OpenReader(name string) (io.ReadCloser, error) +} + +var newFileOps func(dir string) (FileOps, error) diff --git a/feature/taildrop/fileops_fs.go b/feature/taildrop/fileops_fs.go new file mode 100644 index 000000000..4fecbe4af --- /dev/null +++ b/feature/taildrop/fileops_fs.go @@ -0,0 +1,221 @@ +// Copyright (c) Tailscale Inc & AUTHORS +// SPDX-License-Identifier: BSD-3-Clause +//go:build !android + +package taildrop + +import ( + "bytes" + "crypto/sha256" + "errors" + "fmt" + "io" + "io/fs" + "os" + "path" + "path/filepath" + "strings" + "sync" + "unicode/utf8" +) + +var renameMu sync.Mutex + +// fsFileOps implements FileOps using the local filesystem rooted at a directory. +// It is used on non-Android platforms. +type fsFileOps struct{ rootDir string } + +func init() { + newFileOps = func(dir string) (FileOps, error) { + if dir == "" { + return nil, errors.New("rootDir cannot be empty") + } + if err := os.MkdirAll(dir, 0o700); err != nil { + return nil, fmt.Errorf("mkdir %q: %w", dir, err) + } + return fsFileOps{rootDir: dir}, nil + } +} + +func (f fsFileOps) OpenWriter(name string, offset int64, perm os.FileMode) (io.WriteCloser, string, error) { + path, err := joinDir(f.rootDir, name) + if err != nil { + return nil, "", err + } + if err = os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + return nil, "", err + } + fi, err := os.OpenFile(path, os.O_CREATE|os.O_RDWR, perm) + if err != nil { + return nil, "", err + } + if offset != 0 { + curr, err := fi.Seek(0, io.SeekEnd) + if err != nil { + fi.Close() + return nil, "", err + } + if offset < 0 || offset > curr { + fi.Close() + return nil, "", fmt.Errorf("offset %d out of range", offset) + } + if _, err := fi.Seek(offset, io.SeekStart); err != nil { + fi.Close() + return nil, "", err + } + if err := fi.Truncate(offset); err != nil { + fi.Close() + return nil, "", err + } + } + return fi, path, nil +} + +func (f fsFileOps) Remove(name string) error { + path, err := joinDir(f.rootDir, name) + if err != nil { + return err + } + return os.Remove(path) +} + +// Rename moves the partial file into its final name. +// newName must be a base name (not absolute or containing path separators). +// It will retry up to 10 times, de-dup same-checksum files, etc. +func (f fsFileOps) Rename(oldPath, newName string) (newPath string, err error) { + var dst string + if filepath.IsAbs(newName) || strings.ContainsRune(newName, os.PathSeparator) { + return "", fmt.Errorf("invalid newName %q: must not be an absolute path or contain path separators", newName) + } + + dst = filepath.Join(f.rootDir, newName) + + if err := os.MkdirAll(filepath.Dir(dst), 0o700); err != nil { + return "", err + } + + st, err := os.Stat(oldPath) + if err != nil { + return "", err + } + wantSize := st.Size() + + const maxRetries = 10 + for i := 0; i < maxRetries; i++ { + renameMu.Lock() + fi, statErr := os.Stat(dst) + // Atomically rename the partial file as the destination file if it doesn't exist. + // Otherwise, it returns the length of the current destination file. + // The operation is atomic. + if os.IsNotExist(statErr) { + err = os.Rename(oldPath, dst) + renameMu.Unlock() + if err != nil { + return "", err + } + return dst, nil + } + if statErr != nil { + renameMu.Unlock() + return "", statErr + } + gotSize := fi.Size() + renameMu.Unlock() + + // Avoid the final rename if a destination file has the same contents. + // + // Note: this is best effort and copying files from iOS from the Media Library + // results in processing on the iOS side which means the size and shas of the + // same file can be different. + if gotSize == wantSize { + sumP, err := sha256File(oldPath) + if err != nil { + return "", err + } + sumD, err := sha256File(dst) + if err != nil { + return "", err + } + if bytes.Equal(sumP[:], sumD[:]) { + if err := os.Remove(oldPath); err != nil { + return "", err + } + return dst, nil + } + } + + // Choose a new destination filename and try again. + dst = filepath.Join(filepath.Dir(dst), nextFilename(filepath.Base(dst))) + } + + return "", fmt.Errorf("too many retries trying to rename %q to %q", oldPath, newName) +} + +// sha256File computes the SHA‑256 of a file. +func sha256File(path string) (sum [sha256.Size]byte, _ error) { + f, err := os.Open(path) + if err != nil { + return sum, err + } + defer f.Close() + h := sha256.New() + if _, err := io.Copy(h, f); err != nil { + return sum, err + } + copy(sum[:], h.Sum(nil)) + return sum, nil +} + +func (f fsFileOps) ListFiles() ([]string, error) { + entries, err := os.ReadDir(f.rootDir) + if err != nil { + return nil, err + } + var names []string + for _, e := range entries { + if e.Type().IsRegular() { + names = append(names, e.Name()) + } + } + return names, nil +} + +func (f fsFileOps) Stat(name string) (fs.FileInfo, error) { + path, err := joinDir(f.rootDir, name) + if err != nil { + return nil, err + } + return os.Stat(path) +} + +func (f fsFileOps) OpenReader(name string) (io.ReadCloser, error) { + path, err := joinDir(f.rootDir, name) + if err != nil { + return nil, err + } + return os.Open(path) +} + +// joinDir is like [filepath.Join] but returns an error if baseName is too long, +// is a relative path instead of a basename, or is otherwise invalid or unsafe for incoming files. +func joinDir(dir, baseName string) (string, error) { + if !utf8.ValidString(baseName) || + strings.TrimSpace(baseName) != baseName || + len(baseName) > 255 { + return "", ErrInvalidFileName + } + // TODO: validate unicode normalization form too? Varies by platform. + clean := path.Clean(baseName) + if clean != baseName || clean == "." || clean == ".." { + return "", ErrInvalidFileName + } + for _, r := range baseName { + if !validFilenameRune(r) { + return "", ErrInvalidFileName + } + } + if !filepath.IsLocal(baseName) { + return "", ErrInvalidFileName + } + return filepath.Join(dir, baseName), nil +} diff --git a/feature/taildrop/paths.go b/feature/taildrop/paths.go index 22d01160c..79dc37d8f 100644 --- a/feature/taildrop/paths.go +++ b/feature/taildrop/paths.go @@ -21,7 +21,7 @@ func (e *Extension) SetDirectFileRoot(root string) { // SetFileOps sets the platform specific file operations. This is used // to call Android's Storage Access Framework APIs. func (e *Extension) SetFileOps(fileOps FileOps) { - e.FileOps = fileOps + e.fileOps = fileOps } func (e *Extension) setPlatformDefaultDirectFileRoot() { diff --git a/feature/taildrop/peerapi_test.go b/feature/taildrop/peerapi_test.go index 1a003b6ed..633997354 100644 --- a/feature/taildrop/peerapi_test.go +++ b/feature/taildrop/peerapi_test.go @@ -24,6 +24,7 @@ "tailscale.com/tstest" "tailscale.com/tstime" "tailscale.com/types/logger" + "tailscale.com/util/must" ) // peerAPIHandler serves the PeerAPI for a source specific client. @@ -93,7 +94,16 @@ func bodyContains(sub string) check { func fileHasSize(name string, size int) check { return func(t *testing.T, e *peerAPITestEnv) { - root := e.taildrop.Dir() + fsImpl, ok := e.taildrop.opts.fileOps.(*fsFileOps) + if !ok { + t.Skip("fileHasSize only supported on fsFileOps backend") + return + } + root := fsImpl.rootDir + if root == "" { + t.Errorf("no rootdir; can't check whether %q has size %v", name, size) + return + } if root == "" { t.Errorf("no rootdir; can't check whether %q has size %v", name, size) return @@ -109,12 +119,12 @@ func fileHasSize(name string, size int) check { func fileHasContents(name string, want string) check { return func(t *testing.T, e *peerAPITestEnv) { - root := e.taildrop.Dir() - if root == "" { - t.Errorf("no rootdir; can't check contents of %q", name) + fsImpl, ok := e.taildrop.opts.fileOps.(*fsFileOps) + if !ok { + t.Skip("fileHasContents only supported on fsFileOps backend") return } - path := filepath.Join(root, name) + path := filepath.Join(fsImpl.rootDir, name) got, err := os.ReadFile(path) if err != nil { t.Errorf("fileHasContents: %v", err) @@ -172,9 +182,10 @@ func TestHandlePeerAPI(t *testing.T) { reqs: []*http.Request{httptest.NewRequest("PUT", "/v0/put/foo", nil)}, checks: checks( httpStatus(http.StatusForbidden), - bodyContains("Taildrop disabled; no storage directory"), + bodyContains("Taildrop disabled"), ), }, + { name: "bad_method", isSelf: true, @@ -471,14 +482,18 @@ func(t *testing.T, env *peerAPITestEnv) { selfNode.CapMap = tailcfg.NodeCapMap{tailcfg.CapabilityDebug: nil} } var rootDir string + var fo FileOps if !tt.omitRoot { - rootDir = t.TempDir() + var err error + if fo, err = newFileOps(t.TempDir()); err != nil { + t.Fatalf("newFileOps: %v", err) + } } var e peerAPITestEnv e.taildrop = managerOptions{ - Logf: e.logBuf.Logf, - Dir: rootDir, + Logf: e.logBuf.Logf, + fileOps: fo, }.New() ext := &fakeExtension{ @@ -490,9 +505,7 @@ func(t *testing.T, env *peerAPITestEnv) { e.ph = &peerAPIHandler{ isSelf: tt.isSelf, selfNode: selfNode.View(), - peerNode: (&tailcfg.Node{ - ComputedName: "some-peer-name", - }).View(), + peerNode: (&tailcfg.Node{ComputedName: "some-peer-name"}).View(), } for _, req := range tt.reqs { e.rr = httptest.NewRecorder() @@ -526,8 +539,8 @@ func(t *testing.T, env *peerAPITestEnv) { func TestFileDeleteRace(t *testing.T) { dir := t.TempDir() taildropMgr := managerOptions{ - Logf: t.Logf, - Dir: dir, + Logf: t.Logf, + fileOps: must.Get(newFileOps(dir)), }.New() ph := &peerAPIHandler{ diff --git a/feature/taildrop/resume.go b/feature/taildrop/resume.go index 211a1ff6b..20ef527a6 100644 --- a/feature/taildrop/resume.go +++ b/feature/taildrop/resume.go @@ -9,7 +9,6 @@ "encoding/hex" "fmt" "io" - "io/fs" "os" "strings" ) @@ -51,19 +50,20 @@ func (cs *checksum) UnmarshalText(b []byte) error { // PartialFiles returns a list of partial files in [Handler.Dir] // that were sent (or is actively being sent) by the provided id. -func (m *manager) PartialFiles(id clientID) (ret []string, err error) { - if m == nil || m.opts.Dir == "" { +func (m *manager) PartialFiles(id clientID) ([]string, error) { + if m == nil || m.opts.fileOps == nil { return nil, ErrNoTaildrop } - suffix := id.partialSuffix() - if err := rangeDir(m.opts.Dir, func(de fs.DirEntry) bool { - if name := de.Name(); strings.HasSuffix(name, suffix) { - ret = append(ret, name) + files, err := m.opts.fileOps.ListFiles() + if err != nil { + return nil, redactError(err) + } + var ret []string + for _, filename := range files { + if strings.HasSuffix(filename, suffix) { + ret = append(ret, filename) } - return true - }); err != nil { - return ret, redactError(err) } return ret, nil } @@ -73,17 +73,13 @@ func (m *manager) PartialFiles(id clientID) (ret []string, err error) { // It returns (BlockChecksum{}, io.EOF) when the stream is complete. // It is the caller's responsibility to call close. func (m *manager) HashPartialFile(id clientID, baseName string) (next func() (blockChecksum, error), close func() error, err error) { - if m == nil || m.opts.Dir == "" { + if m == nil || m.opts.fileOps == nil { return nil, nil, ErrNoTaildrop } noopNext := func() (blockChecksum, error) { return blockChecksum{}, io.EOF } noopClose := func() error { return nil } - dstFile, err := joinDir(m.opts.Dir, baseName) - if err != nil { - return nil, nil, err - } - f, err := os.Open(dstFile + id.partialSuffix()) + f, err := m.opts.fileOps.OpenReader(baseName + id.partialSuffix()) if err != nil { if os.IsNotExist(err) { return noopNext, noopClose, nil diff --git a/feature/taildrop/resume_test.go b/feature/taildrop/resume_test.go index dac3c657b..4e59d401d 100644 --- a/feature/taildrop/resume_test.go +++ b/feature/taildrop/resume_test.go @@ -8,6 +8,7 @@ "io" "math/rand" "os" + "path/filepath" "testing" "testing/iotest" @@ -19,7 +20,9 @@ func TestResume(t *testing.T) { defer func() { blockSize = oldBlockSize }() blockSize = 256 - m := managerOptions{Logf: t.Logf, Dir: t.TempDir()}.New() + dir := t.TempDir() + + m := managerOptions{Logf: t.Logf, fileOps: must.Get(newFileOps(dir))}.New() defer m.Shutdown() rn := rand.New(rand.NewSource(0)) @@ -37,7 +40,7 @@ func TestResume(t *testing.T) { must.Do(close()) // Windows wants the file handle to be closed to rename it. must.Get(m.PutFile("", "foo", r, offset, -1)) - got := must.Get(os.ReadFile(must.Get(joinDir(m.opts.Dir, "foo")))) + got := must.Get(os.ReadFile(filepath.Join(dir, "foo"))) if !bytes.Equal(got, want) { t.Errorf("content mismatches") } @@ -66,7 +69,7 @@ func TestResume(t *testing.T) { t.Fatalf("too many iterations to complete the test") } } - got := must.Get(os.ReadFile(must.Get(joinDir(m.opts.Dir, "bar")))) + got := must.Get(os.ReadFile(filepath.Join(dir, "bar"))) if !bytes.Equal(got, want) { t.Errorf("content mismatches") } diff --git a/feature/taildrop/retrieve.go b/feature/taildrop/retrieve.go index 6fb975193..b048a1b3b 100644 --- a/feature/taildrop/retrieve.go +++ b/feature/taildrop/retrieve.go @@ -9,19 +9,19 @@ "io" "io/fs" "os" - "path/filepath" "runtime" "sort" "time" "tailscale.com/client/tailscale/apitype" "tailscale.com/logtail/backoff" + "tailscale.com/util/set" ) // HasFilesWaiting reports whether any files are buffered in [Handler.Dir]. // This always returns false when [Handler.DirectFileMode] is false. -func (m *manager) HasFilesWaiting() (has bool) { - if m == nil || m.opts.Dir == "" || m.opts.DirectFileMode { +func (m *manager) HasFilesWaiting() bool { + if m == nil || m.opts.fileOps == nil || m.opts.DirectFileMode { return false } @@ -30,64 +30,67 @@ func (m *manager) HasFilesWaiting() (has bool) { // has-files-or-not values as the macOS/iOS client might // in the future use+delete the files directly. So only // keep this negative cache. - totalReceived := m.totalReceived.Load() - if totalReceived == m.emptySince.Load() { + total := m.totalReceived.Load() + if total == m.emptySince.Load() { return false } - // Check whether there is at least one one waiting file. - err := rangeDir(m.opts.Dir, func(de fs.DirEntry) bool { - name := de.Name() - if isPartialOrDeleted(name) || !de.Type().IsRegular() { - return true - } - _, err := os.Stat(filepath.Join(m.opts.Dir, name+deletedSuffix)) - if os.IsNotExist(err) { - has = true - return false - } - return true - }) - - // If there are no more waiting files, record totalReceived as emptySince - // so that we can short-circuit the expensive directory traversal - // if no files have been received after the start of this call. - if err == nil && !has { - m.emptySince.Store(totalReceived) + files, err := m.opts.fileOps.ListFiles() + if err != nil { + return false } - return has + + // Build a set of filenames present in Dir + fileSet := set.Of(files...) + + for _, filename := range files { + if isPartialOrDeleted(filename) { + continue + } + if fileSet.Contains(filename + deletedSuffix) { + continue // already handled + } + // Found at least one downloadable file + return true + } + + // No waiting files → update negative‑result cache + m.emptySince.Store(total) + return false } // WaitingFiles returns the list of files that have been sent by a // peer that are waiting in [Handler.Dir]. // This always returns nil when [Handler.DirectFileMode] is false. -func (m *manager) WaitingFiles() (ret []apitype.WaitingFile, err error) { - if m == nil || m.opts.Dir == "" { +func (m *manager) WaitingFiles() ([]apitype.WaitingFile, error) { + if m == nil || m.opts.fileOps == nil { return nil, ErrNoTaildrop } if m.opts.DirectFileMode { return nil, nil } - if err := rangeDir(m.opts.Dir, func(de fs.DirEntry) bool { - name := de.Name() - if isPartialOrDeleted(name) || !de.Type().IsRegular() { - return true - } - _, err := os.Stat(filepath.Join(m.opts.Dir, name+deletedSuffix)) - if os.IsNotExist(err) { - fi, err := de.Info() - if err != nil { - return true - } - ret = append(ret, apitype.WaitingFile{ - Name: filepath.Base(name), - Size: fi.Size(), - }) - } - return true - }); err != nil { + names, err := m.opts.fileOps.ListFiles() + if err != nil { return nil, redactError(err) } + var ret []apitype.WaitingFile + for _, name := range names { + if isPartialOrDeleted(name) { + continue + } + // A corresponding .deleted marker means the file was already handled. + if _, err := m.opts.fileOps.Stat(name + deletedSuffix); err == nil { + continue + } + fi, err := m.opts.fileOps.Stat(name) + if err != nil { + continue + } + ret = append(ret, apitype.WaitingFile{ + Name: name, + Size: fi.Size(), + }) + } sort.Slice(ret, func(i, j int) bool { return ret[i].Name < ret[j].Name }) return ret, nil } @@ -95,21 +98,18 @@ func (m *manager) WaitingFiles() (ret []apitype.WaitingFile, err error) { // DeleteFile deletes a file of the given baseName from [Handler.Dir]. // This method is only allowed when [Handler.DirectFileMode] is false. func (m *manager) DeleteFile(baseName string) error { - if m == nil || m.opts.Dir == "" { + if m == nil || m.opts.fileOps == nil { return ErrNoTaildrop } if m.opts.DirectFileMode { return errors.New("deletes not allowed in direct mode") } - path, err := joinDir(m.opts.Dir, baseName) - if err != nil { - return err - } + var bo *backoff.Backoff logf := m.opts.Logf t0 := m.opts.Clock.Now() for { - err := os.Remove(path) + err := m.opts.fileOps.Remove(baseName) if err != nil && !os.IsNotExist(err) { err = redactError(err) // Put a retry loop around deletes on Windows. @@ -129,7 +129,7 @@ func (m *manager) DeleteFile(baseName string) error { bo.BackOff(context.Background(), err) continue } - if err := touchFile(path + deletedSuffix); err != nil { + if err := m.touchFile(baseName + deletedSuffix); err != nil { logf("peerapi: failed to leave deleted marker: %v", err) } m.deleter.Insert(baseName + deletedSuffix) @@ -141,35 +141,31 @@ func (m *manager) DeleteFile(baseName string) error { } } -func touchFile(path string) error { - f, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE, 0666) +func (m *manager) touchFile(name string) error { + wc, _, err := m.opts.fileOps.OpenWriter(name /* offset= */, 0, 0666) if err != nil { return redactError(err) } - return f.Close() + return wc.Close() } // OpenFile opens a file of the given baseName from [Handler.Dir]. // This method is only allowed when [Handler.DirectFileMode] is false. func (m *manager) OpenFile(baseName string) (rc io.ReadCloser, size int64, err error) { - if m == nil || m.opts.Dir == "" { + if m == nil || m.opts.fileOps == nil { return nil, 0, ErrNoTaildrop } if m.opts.DirectFileMode { return nil, 0, errors.New("opens not allowed in direct mode") } - path, err := joinDir(m.opts.Dir, baseName) - if err != nil { - return nil, 0, err + if _, err := m.opts.fileOps.Stat(baseName + deletedSuffix); err == nil { + return nil, 0, redactError(&fs.PathError{Op: "open", Path: baseName, Err: fs.ErrNotExist}) } - if _, err := os.Stat(path + deletedSuffix); err == nil { - return nil, 0, redactError(&fs.PathError{Op: "open", Path: path, Err: fs.ErrNotExist}) - } - f, err := os.Open(path) + f, err := m.opts.fileOps.OpenReader(baseName) if err != nil { return nil, 0, redactError(err) } - fi, err := f.Stat() + fi, err := m.opts.fileOps.Stat(baseName) if err != nil { f.Close() return nil, 0, redactError(err) diff --git a/feature/taildrop/send.go b/feature/taildrop/send.go index 59a1701da..32ba5f6f0 100644 --- a/feature/taildrop/send.go +++ b/feature/taildrop/send.go @@ -4,11 +4,8 @@ package taildrop import ( - "crypto/sha256" "fmt" "io" - "os" - "path/filepath" "sync" "time" @@ -73,9 +70,10 @@ func (f *incomingFile) Write(p []byte) (n int, err error) { // specific partial file. This allows the client to determine whether to resume // a partial file. While resuming, PutFile may be called again with a non-zero // offset to specify where to resume receiving data at. -func (m *manager) PutFile(id clientID, baseName string, r io.Reader, offset, length int64) (int64, error) { +func (m *manager) PutFile(id clientID, baseName string, r io.Reader, offset, length int64) (fileLength int64, err error) { + switch { - case m == nil || m.opts.Dir == "": + case m == nil || m.opts.fileOps == nil: return 0, ErrNoTaildrop case !envknob.CanTaildrop(): return 0, ErrNoTaildrop @@ -83,48 +81,48 @@ func (m *manager) PutFile(id clientID, baseName string, r io.Reader, offset, len return 0, ErrNotAccessible } - //Compute dstPath & avoid mid‑upload deletion - var dstPath string - if m.opts.Mode == PutModeDirect { - var err error - dstPath, err = joinDir(m.opts.Dir, baseName) - if err != nil { - return 0, err - } - } else { - // In SAF mode, we simply use the baseName as the destination "path" - // (the actual directory is managed by SAF). - dstPath = baseName + if err := validateBaseName(baseName); err != nil { + return 0, err } - m.deleter.Remove(filepath.Base(dstPath)) // avoid deleting the partial file while receiving - // Check whether there is an in-progress transfer for the file. - partialFileKey := incomingFileKey{id, baseName} - inFile, loaded := m.incomingFiles.LoadOrInit(partialFileKey, func() *incomingFile { - return &incomingFile{ - clock: m.opts.Clock, - started: m.opts.Clock.Now(), - size: length, - sendFileNotify: m.opts.SendFileNotify, - } - }) - if loaded { - return 0, ErrFileExists - } - defer m.incomingFiles.Delete(partialFileKey) + // and make sure we don't delete it while uploading: + m.deleter.Remove(baseName) - // Open writer & populate inFile paths - wc, partialPath, err := m.openWriterAndPaths(id, m.opts.Mode, inFile, baseName, dstPath, offset) + // Create (if not already) the partial file with read-write permissions. + partialName := baseName + id.partialSuffix() + wc, partialPath, err := m.opts.fileOps.OpenWriter(partialName, offset, 0o666) if err != nil { return 0, m.redactAndLogError("Create", err) } defer func() { wc.Close() if err != nil { - m.deleter.Insert(filepath.Base(partialPath)) // mark partial file for eventual deletion + m.deleter.Insert(partialName) // mark partial file for eventual deletion } }() + // Check whether there is an in-progress transfer for the file. + inFileKey := incomingFileKey{id, baseName} + inFile, loaded := m.incomingFiles.LoadOrInit(inFileKey, func() *incomingFile { + inFile := &incomingFile{ + clock: m.opts.Clock, + started: m.opts.Clock.Now(), + size: length, + sendFileNotify: m.opts.SendFileNotify, + } + if m.opts.DirectFileMode { + inFile.partialPath = partialPath + } + return inFile + }) + + inFile.w = wc + + if loaded { + return 0, ErrFileExists + } + defer m.incomingFiles.Delete(inFileKey) + // Record that we have started to receive at least one file. // This is used by the deleter upon a cold-start to scan the directory // for any files that need to be deleted. @@ -148,220 +146,26 @@ func (m *manager) PutFile(id clientID, baseName string, r io.Reader, offset, len return 0, m.redactAndLogError("Close", err) } - fileLength := offset + copyLength + fileLength = offset + copyLength inFile.mu.Lock() inFile.done = true inFile.mu.Unlock() - // Finalize rename - switch m.opts.Mode { - case PutModeDirect: - var finalDst string - finalDst, err = m.finalizeDirect(inFile, partialPath, dstPath, fileLength) - if err != nil { - return 0, m.redactAndLogError("Rename", err) - } - inFile.finalPath = finalDst - - case PutModeAndroidSAF: - if err = m.finalizeSAF(partialPath, baseName); err != nil { - return 0, m.redactAndLogError("Rename", err) - } + // 6) Finalize (rename/move) the partial into place via FileOps.Rename + finalPath, err := m.opts.fileOps.Rename(partialPath, baseName) + if err != nil { + return 0, m.redactAndLogError("Rename", err) } + inFile.finalPath = finalPath m.totalReceived.Add(1) m.opts.SendFileNotify() return fileLength, nil } -// openWriterAndPaths opens the correct writer, seeks/truncates if needed, -// and sets inFile.partialPath & inFile.finalPath for later cleanup/rename. -// The caller is responsible for closing the file on completion. -func (m *manager) openWriterAndPaths( - id clientID, - mode PutMode, - inFile *incomingFile, - baseName string, - dstPath string, - offset int64, -) (wc io.WriteCloser, partialPath string, err error) { - switch mode { - - case PutModeDirect: - partialPath = dstPath + id.partialSuffix() - f, err := os.OpenFile(partialPath, os.O_CREATE|os.O_RDWR, 0o666) - if err != nil { - return nil, "", m.redactAndLogError("Create", err) - } - if offset != 0 { - curr, err := f.Seek(0, io.SeekEnd) - if err != nil { - f.Close() - return nil, "", m.redactAndLogError("Seek", err) - } - if offset < 0 || offset > curr { - f.Close() - return nil, "", m.redactAndLogError("Seek", fmt.Errorf("offset %d out of range", offset)) - } - if _, err := f.Seek(offset, io.SeekStart); err != nil { - f.Close() - return nil, "", m.redactAndLogError("Seek", err) - } - if err := f.Truncate(offset); err != nil { - f.Close() - return nil, "", m.redactAndLogError("Truncate", err) - } - } - inFile.w = f - wc = f - inFile.partialPath = partialPath - inFile.finalPath = dstPath - return wc, partialPath, nil - - case PutModeAndroidSAF: - if m.opts.FileOps == nil { - return nil, "", m.redactAndLogError("Create (SAF)", fmt.Errorf("missing FileOps")) - } - writer, uri, err := m.opts.FileOps.OpenFileWriter(baseName) - if err != nil { - return nil, "", m.redactAndLogError("Create (SAF)", fmt.Errorf("failed to open file for writing via SAF")) - } - if writer == nil || uri == "" { - return nil, "", fmt.Errorf("invalid SAF writer or URI") - } - // SAF mode does not support resuming, so enforce offset == 0. - if offset != 0 { - writer.Close() - return nil, "", m.redactAndLogError("Seek", fmt.Errorf("resuming is not supported in SAF mode")) - } - inFile.w = writer - wc = writer - partialPath = uri - inFile.partialPath = uri - inFile.finalPath = baseName - return wc, partialPath, nil - - default: - return nil, "", fmt.Errorf("unsupported PutMode: %v", mode) - } -} - -// finalizeDirect atomically renames or dedups the partial file, retrying -// under new names up to 10 times. It returns the final path that succeeded. -func (m *manager) finalizeDirect( - inFile *incomingFile, - partialPath string, - initialDst string, - fileLength int64, -) (string, error) { - var ( - once sync.Once - cachedSum [sha256.Size]byte - cacheErr error - computeSum = func() ([sha256.Size]byte, error) { - once.Do(func() { cachedSum, cacheErr = sha256File(partialPath) }) - return cachedSum, cacheErr - } - ) - - dstPath := initialDst - const maxRetries = 10 - for i := 0; i < maxRetries; i++ { - // Atomically rename the partial file as the destination file if it doesn't exist. - // Otherwise, it returns the length of the current destination file. - // The operation is atomic. - lengthOnDisk, err := func() (int64, error) { - m.renameMu.Lock() - defer m.renameMu.Unlock() - fi, statErr := os.Stat(dstPath) - if os.IsNotExist(statErr) { - // dst missing → rename partial into place - return -1, os.Rename(partialPath, dstPath) - } - if statErr != nil { - return -1, statErr - } - return fi.Size(), nil - }() - if err != nil { - return "", err - } - if lengthOnDisk < 0 { - // successfully moved - inFile.finalPath = dstPath - return dstPath, nil - } - - // Avoid the final rename if a destination file has the same contents. - // - // Note: this is best effort and copying files from iOS from the Media Library - // results in processing on the iOS side which means the size and shas of the - // same file can be different. - if lengthOnDisk == fileLength { - partSum, err := computeSum() - if err != nil { - return "", err - } - dstSum, err := sha256File(dstPath) - if err != nil { - return "", err - } - if partSum == dstSum { - // same content → drop the partial - if err := os.Remove(partialPath); err != nil { - return "", err - } - inFile.finalPath = dstPath - return dstPath, nil - } - } - - // Choose a new destination filename and try again. - dstPath = nextFilename(dstPath) - } - - return "", fmt.Errorf("too many retries trying to rename a partial file %q", initialDst) -} - -// finalizeSAF retries RenamePartialFile up to 10 times, generating a new -// name on each failure until the SAF URI changes. -func (m *manager) finalizeSAF( - partialPath, finalName string, -) error { - if m.opts.FileOps == nil { - return fmt.Errorf("missing FileOps for SAF finalize") - } - const maxTries = 10 - name := finalName - for i := 0; i < maxTries; i++ { - newURI, err := m.opts.FileOps.RenamePartialFile(partialPath, m.opts.Dir, name) - if err != nil { - return err - } - if newURI != "" && newURI != name { - return nil - } - name = nextFilename(name) - } - return fmt.Errorf("failed to finalize SAF file after %d retries", maxTries) -} - func (m *manager) redactAndLogError(stage string, err error) error { err = redactError(err) m.opts.Logf("put %s error: %v", stage, err) return err } - -func sha256File(file string) (out [sha256.Size]byte, err error) { - h := sha256.New() - f, err := os.Open(file) - if err != nil { - return out, err - } - defer f.Close() - if _, err := io.Copy(h, f); err != nil { - return out, err - } - return [sha256.Size]byte(h.Sum(nil)), nil -} diff --git a/feature/taildrop/send_test.go b/feature/taildrop/send_test.go index 8edb70417..9ffa5fccc 100644 --- a/feature/taildrop/send_test.go +++ b/feature/taildrop/send_test.go @@ -4,123 +4,64 @@ package taildrop import ( - "bytes" - "fmt" - "io" "os" "path/filepath" + "strings" "testing" "tailscale.com/tstime" + "tailscale.com/util/must" ) -// nopWriteCloser is a no-op io.WriteCloser wrapping a bytes.Buffer. -type nopWriteCloser struct{ *bytes.Buffer } - -func (nwc nopWriteCloser) Close() error { return nil } - -// mockFileOps implements just enough of the FileOps interface for SAF tests. -type mockFileOps struct { - writes *bytes.Buffer - renameOK bool -} - -func (m *mockFileOps) OpenFileWriter(name string) (io.WriteCloser, string, error) { - m.writes = new(bytes.Buffer) - return nopWriteCloser{m.writes}, "uri://" + name + ".partial", nil -} - -func (m *mockFileOps) RenamePartialFile(partialPath, dir, finalName string) (string, error) { - if !m.renameOK { - m.renameOK = true - return "uri://" + finalName, nil - } - return "", io.ErrUnexpectedEOF -} - func TestPutFile(t *testing.T) { const content = "hello, world" tests := []struct { - name string - mode PutMode - setup func(t *testing.T) (*manager, string, *mockFileOps) - wantFile string + name string + directFileMode bool }{ - { - name: "PutModeDirect", - mode: PutModeDirect, - setup: func(t *testing.T) (*manager, string, *mockFileOps) { - dir := t.TempDir() - opts := managerOptions{ - Logf: t.Logf, - Clock: tstime.DefaultClock{}, - State: nil, - Dir: dir, - Mode: PutModeDirect, - DirectFileMode: true, - SendFileNotify: func() {}, - } - mgr := opts.New() - return mgr, dir, nil - }, - wantFile: "file.txt", - }, - { - name: "PutModeAndroidSAF", - mode: PutModeAndroidSAF, - setup: func(t *testing.T) (*manager, string, *mockFileOps) { - // SAF still needs a non-empty Dir to pass the guard. - dir := t.TempDir() - mops := &mockFileOps{} - opts := managerOptions{ - Logf: t.Logf, - Clock: tstime.DefaultClock{}, - State: nil, - Dir: dir, - Mode: PutModeAndroidSAF, - FileOps: mops, - DirectFileMode: true, - SendFileNotify: func() {}, - } - mgr := opts.New() - return mgr, dir, mops - }, - wantFile: "file.txt", - }, + {"DirectFileMode", true}, + {"NonDirectFileMode", false}, } - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - mgr, dir, mops := tc.setup(t) - id := clientID(fmt.Sprint(0)) - reader := bytes.NewReader([]byte(content)) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir := t.TempDir() + mgr := managerOptions{ + Logf: t.Logf, + Clock: tstime.DefaultClock{}, + State: nil, + fileOps: must.Get(newFileOps(dir)), + DirectFileMode: tt.directFileMode, + SendFileNotify: func() {}, + }.New() - n, err := mgr.PutFile(id, "file.txt", reader, 0, int64(len(content))) + id := clientID("0") + n, err := mgr.PutFile(id, "file.txt", strings.NewReader(content), 0, int64(len(content))) if err != nil { - t.Fatalf("PutFile(%s) error: %v", tc.name, err) + t.Fatalf("PutFile error: %v", err) } if n != int64(len(content)) { t.Errorf("wrote %d bytes; want %d", n, len(content)) } - switch tc.mode { - case PutModeDirect: - path := filepath.Join(dir, tc.wantFile) - data, err := os.ReadFile(path) - if err != nil { - t.Fatalf("ReadFile error: %v", err) - } - if got := string(data); got != content { - t.Errorf("file contents = %q; want %q", got, content) - } + path := filepath.Join(dir, "file.txt") - case PutModeAndroidSAF: - if mops.writes == nil { - t.Fatal("SAF writer was never created") - } - if got := mops.writes.String(); got != content { - t.Errorf("SAF writes = %q; want %q", got, content) + got, err := os.ReadFile(path) + if err != nil { + t.Fatalf("ReadFile %q: %v", path, err) + } + if string(got) != content { + t.Errorf("file contents = %q; want %q", string(got), content) + } + + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatal(err) + } + for _, entry := range entries { + if strings.Contains(entry.Name(), ".partial") { + t.Errorf("unexpected partial file left behind: %s", entry.Name()) } } }) diff --git a/feature/taildrop/taildrop.go b/feature/taildrop/taildrop.go index 2dfa415bb..6c3deaed1 100644 --- a/feature/taildrop/taildrop.go +++ b/feature/taildrop/taildrop.go @@ -12,8 +12,6 @@ import ( "errors" "hash/adler32" - "io" - "io/fs" "os" "path" "path/filepath" @@ -21,7 +19,6 @@ "sort" "strconv" "strings" - "sync" "sync/atomic" "unicode" "unicode/utf8" @@ -72,11 +69,6 @@ type managerOptions struct { Clock tstime.DefaultClock // may be nil State ipn.StateStore // may be nil - // Dir is the directory to store received files. - // This main either be the final location for the files - // or just a temporary staging directory (see DirectFileMode). - Dir string - // DirectFileMode reports whether we are writing files // directly to a download directory, rather than writing them to // a temporary staging directory. @@ -91,9 +83,10 @@ type managerOptions struct { // copy them out, and then delete them. DirectFileMode bool - FileOps FileOps - - Mode PutMode + // FileOps abstracts platform-specific file operations needed for file transfers. + // Android's implementation uses the Storage Access Framework, and other platforms + // use fsFileOps. + fileOps FileOps // SendFileNotify is called periodically while a file is actively // receiving the contents for the file. There is a final call @@ -111,9 +104,6 @@ type manager struct { // deleter managers asynchronous deletion of files. deleter fileDeleter - // renameMu is used to protect os.Rename calls so that they are atomic. - renameMu sync.Mutex - // totalReceived counts the cumulative total of received files. totalReceived atomic.Int64 // emptySince specifies that there were no waiting files @@ -137,11 +127,6 @@ func (opts managerOptions) New() *manager { return m } -// Dir returns the directory. -func (m *manager) Dir() string { - return m.opts.Dir -} - // Shutdown shuts down the Manager. // It blocks until all spawned goroutines have stopped running. func (m *manager) Shutdown() { @@ -172,57 +157,29 @@ func isPartialOrDeleted(s string) bool { return strings.HasSuffix(s, deletedSuffix) || strings.HasSuffix(s, partialSuffix) } -func joinDir(dir, baseName string) (fullPath string, err error) { - if !utf8.ValidString(baseName) { - return "", ErrInvalidFileName - } - if strings.TrimSpace(baseName) != baseName { - return "", ErrInvalidFileName - } - if len(baseName) > 255 { - return "", ErrInvalidFileName +func validateBaseName(name string) error { + if !utf8.ValidString(name) || + strings.TrimSpace(name) != name || + len(name) > 255 { + return ErrInvalidFileName } // TODO: validate unicode normalization form too? Varies by platform. - clean := path.Clean(baseName) - if clean != baseName || - clean == "." || clean == ".." || - isPartialOrDeleted(clean) { - return "", ErrInvalidFileName + clean := path.Clean(name) + if clean != name || clean == "." || clean == ".." { + return ErrInvalidFileName } - for _, r := range baseName { + if isPartialOrDeleted(name) { + return ErrInvalidFileName + } + for _, r := range name { if !validFilenameRune(r) { - return "", ErrInvalidFileName + return ErrInvalidFileName } } - if !filepath.IsLocal(baseName) { - return "", ErrInvalidFileName - } - return filepath.Join(dir, baseName), nil -} - -// rangeDir iterates over the contents of a directory, calling fn for each entry. -// It continues iterating while fn returns true. -// It reports the number of entries seen. -func rangeDir(dir string, fn func(fs.DirEntry) bool) error { - f, err := os.Open(dir) - if err != nil { - return err - } - defer f.Close() - for { - des, err := f.ReadDir(10) - for _, de := range des { - if !fn(de) { - return nil - } - } - if err != nil { - if err == io.EOF { - return nil - } - return err - } + if !filepath.IsLocal(name) { + return ErrInvalidFileName } + return nil } // IncomingFiles returns a list of active incoming files. diff --git a/feature/taildrop/taildrop_test.go b/feature/taildrop/taildrop_test.go index da0bd2f43..0d77273f0 100644 --- a/feature/taildrop/taildrop_test.go +++ b/feature/taildrop/taildrop_test.go @@ -4,40 +4,10 @@ package taildrop import ( - "path/filepath" "strings" "testing" ) -func TestJoinDir(t *testing.T) { - dir := t.TempDir() - tests := []struct { - in string - want string // just relative to m.Dir - wantOk bool - }{ - {"", "", false}, - {"foo", "foo", true}, - {"./foo", "", false}, - {"../foo", "", false}, - {"foo/bar", "", false}, - {"😋", "😋", true}, - {"\xde\xad\xbe\xef", "", false}, - {"foo.partial", "", false}, - {"foo.deleted", "", false}, - {strings.Repeat("a", 1024), "", false}, - {"foo:bar", "", false}, - } - for _, tt := range tests { - got, gotErr := joinDir(dir, tt.in) - got, _ = filepath.Rel(dir, got) - gotOk := gotErr == nil - if got != tt.want || gotOk != tt.wantOk { - t.Errorf("joinDir(%q) = (%v, %v), want (%v, %v)", tt.in, got, gotOk, tt.want, tt.wantOk) - } - } -} - func TestNextFilename(t *testing.T) { tests := []struct { in string @@ -67,3 +37,29 @@ func TestNextFilename(t *testing.T) { } } } + +func TestValidateBaseName(t *testing.T) { + tests := []struct { + in string + wantOk bool + }{ + {"", false}, + {"foo", true}, + {"./foo", false}, + {"../foo", false}, + {"foo/bar", false}, + {"😋", true}, + {"\xde\xad\xbe\xef", false}, + {"foo.partial", false}, + {"foo.deleted", false}, + {strings.Repeat("a", 1024), false}, + {"foo:bar", false}, + } + for _, tt := range tests { + err := validateBaseName(tt.in) + gotOk := err == nil + if gotOk != tt.wantOk { + t.Errorf("validateBaseName(%q) = %v, wantOk = %v", tt.in, err, tt.wantOk) + } + } +}