From 55027d40fb4832384e434db48e78b32646a73ad5 Mon Sep 17 00:00:00 2001 From: kari-ts Date: Wed, 6 Aug 2025 10:21:13 -0700 Subject: [PATCH] Revert "feature/taildrop: do not use m.opts.Dir for Android (#16316)" This reverts commit d6116ea41805ff7dff2aea5907e63941c7f274fa. --- feature/taildrop/delete.go | 52 +++--- feature/taildrop/delete_test.go | 34 ++-- feature/taildrop/ext.go | 67 +++++--- feature/taildrop/fileops.go | 41 ----- feature/taildrop/fileops_fs.go | 221 ------------------------ feature/taildrop/paths.go | 2 +- feature/taildrop/peerapi_test.go | 41 ++--- feature/taildrop/resume.go | 28 +-- feature/taildrop/resume_test.go | 9 +- feature/taildrop/retrieve.go | 118 ++++++------- feature/taildrop/send.go | 274 +++++++++++++++++++++++++----- feature/taildrop/send_test.go | 131 ++++++++++---- feature/taildrop/taildrop.go | 83 ++++++--- feature/taildrop/taildrop_test.go | 56 +++--- 14 files changed, 599 insertions(+), 558 deletions(-) delete mode 100644 feature/taildrop/fileops.go delete mode 100644 feature/taildrop/fileops_fs.go diff --git a/feature/taildrop/delete.go b/feature/taildrop/delete.go index 0b7259879..e9c8d7f1c 100644 --- a/feature/taildrop/delete.go +++ b/feature/taildrop/delete.go @@ -6,7 +6,9 @@ package taildrop import ( "container/list" "context" + "io/fs" "os" + "path/filepath" "strings" "sync" "time" @@ -26,6 +28,7 @@ const deleteDelay = time.Hour type fileDeleter struct { logf logger.Logf clock tstime.DefaultClock + dir string event func(string) // called for certain events; for testing only mu sync.Mutex @@ -36,7 +39,6 @@ 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. @@ -48,14 +50,15 @@ 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. @@ -68,45 +71,38 @@ func (d *fileDeleter) Init(m *manager, eventHook func(string)) { d.group.Go(func() { d.event("start full-scan") defer d.event("end full-scan") - - 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 { + rangeDir(d.dir, func(de fs.DirEntry) bool { switch { case d.shutdownCtx.Err() != nil: - return // terminate early - case strings.HasSuffix(filename, partialSuffix): + return false // terminate early + case !de.Type().IsRegular(): + return true + case strings.HasSuffix(de.Name(), partialSuffix): // Only enqueue the file for deletion if there is no active put. - nameID := strings.TrimSuffix(filename, partialSuffix) + nameID := strings.TrimSuffix(de.Name(), 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(filename) + d.Insert(de.Name()) } }) } else { - d.Insert(filename) + d.Insert(de.Name()) } - case strings.HasSuffix(filename, deletedSuffix): + case strings.HasSuffix(de.Name(), deletedSuffix): // Best-effort immediate deletion of deleted files. - name := strings.TrimSuffix(filename, deletedSuffix) - if d.fs.Remove(name) == nil { - if d.fs.Remove(filename) == nil { - continue + 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 } } - // Otherwise enqueue for later deletion. - d.Insert(filename) + // Otherwise, enqueue the file for later deletion. + d.Insert(de.Name()) } - } + return true + }) }) } @@ -153,13 +149,13 @@ func (d *fileDeleter) waitAndDelete(wait time.Duration) { // Delete the expired file. if name, ok := strings.CutSuffix(file.name, deletedSuffix); ok { - if err := d.fs.Remove(name); err != nil && !os.IsNotExist(err) { + if err := os.Remove(filepath.Join(d.dir, name)); err != nil && !os.IsNotExist(err) { d.logf("could not delete: %v", redactError(err)) failed = append(failed, elem) continue } } - if err := d.fs.Remove(file.name); err != nil && !os.IsNotExist(err) { + if err := os.Remove(filepath.Join(d.dir, 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 36950f582..7a58de55c 100644 --- a/feature/taildrop/delete_test.go +++ b/feature/taildrop/delete_test.go @@ -5,6 +5,7 @@ package taildrop import ( "os" + "path/filepath" "slices" "testing" "time" @@ -19,20 +20,11 @@ import ( func TestDeleter(t *testing.T) { dir := t.TempDir() - 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 + 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 checkDirectory := func(want ...string) { t.Helper() @@ -77,10 +69,12 @@ 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() @@ -106,17 +100,17 @@ func TestDeleter(t *testing.T) { checkEvents("end waitAndDelete") checkDirectory() - must.Do(m.touchFile("one.partial")) + must.Do(touchFile(filepath.Join(dir, "one.partial"))) insert("one.partial") checkEvents("start waitAndDelete") advance(deleteDelay / 4) - must.Do(m.touchFile("two.partial")) + must.Do(touchFile(filepath.Join(dir, "two.partial"))) insert("two.partial") advance(deleteDelay / 4) - must.Do(m.touchFile("three.partial")) + must.Do(touchFile(filepath.Join(dir, "three.partial"))) insert("three.partial") advance(deleteDelay / 4) - must.Do(m.touchFile("four.partial")) + must.Do(touchFile(filepath.Join(dir, "four.partial"))) insert("four.partial") advance(deleteDelay / 4) @@ -151,8 +145,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 f8f45b53f..c11fe3af4 100644 --- a/feature/taildrop/ext.go +++ b/feature/taildrop/ext.go @@ -10,6 +10,7 @@ import ( "fmt" "io" "maps" + "os" "path/filepath" "runtime" "slices" @@ -74,7 +75,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 @@ -88,6 +89,30 @@ 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" } @@ -151,34 +176,23 @@ func (e *Extension) onChangeProfile(profile ipn.LoginProfileView, _ ipn.PrefsVie return } - // 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 - } + // 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 } - e.setMgrLocked(managerOptions{ Logf: e.logf, Clock: tstime.DefaultClock{Clock: e.sb.Clock()}, State: e.stateStore, + Dir: fileRoot, DirectFileMode: isDirectFileMode, - fileOps: fops, + FileOps: e.FileOps, + Mode: mode, SendFileNotify: e.sendFileNotify, }.New()) } @@ -207,7 +221,12 @@ func (e *Extension) fileRoot(uid tailcfg.UserID, activeLogin string) (root strin baseDir := fmt.Sprintf("%s-uid-%d", strings.ReplaceAll(activeLogin, "@", "-"), uid) - return filepath.Join(varRoot, "files", baseDir), false + 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 } // hasCapFileSharing reports whether the current node has the file sharing diff --git a/feature/taildrop/fileops.go b/feature/taildrop/fileops.go deleted file mode 100644 index 14f76067a..000000000 --- a/feature/taildrop/fileops.go +++ /dev/null @@ -1,41 +0,0 @@ -// 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 deleted file mode 100644 index 4fecbe4af..000000000 --- a/feature/taildrop/fileops_fs.go +++ /dev/null @@ -1,221 +0,0 @@ -// 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 79dc37d8f..22d01160c 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 633997354..1a003b6ed 100644 --- a/feature/taildrop/peerapi_test.go +++ b/feature/taildrop/peerapi_test.go @@ -24,7 +24,6 @@ import ( "tailscale.com/tstest" "tailscale.com/tstime" "tailscale.com/types/logger" - "tailscale.com/util/must" ) // peerAPIHandler serves the PeerAPI for a source specific client. @@ -94,16 +93,7 @@ func bodyContains(sub string) check { func fileHasSize(name string, size int) check { return func(t *testing.T, e *peerAPITestEnv) { - 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 - } + root := e.taildrop.Dir() if root == "" { t.Errorf("no rootdir; can't check whether %q has size %v", name, size) return @@ -119,12 +109,12 @@ func fileHasSize(name string, size int) check { func fileHasContents(name string, want string) check { return func(t *testing.T, e *peerAPITestEnv) { - fsImpl, ok := e.taildrop.opts.fileOps.(*fsFileOps) - if !ok { - t.Skip("fileHasContents only supported on fsFileOps backend") + root := e.taildrop.Dir() + if root == "" { + t.Errorf("no rootdir; can't check contents of %q", name) return } - path := filepath.Join(fsImpl.rootDir, name) + path := filepath.Join(root, name) got, err := os.ReadFile(path) if err != nil { t.Errorf("fileHasContents: %v", err) @@ -182,10 +172,9 @@ func TestHandlePeerAPI(t *testing.T) { reqs: []*http.Request{httptest.NewRequest("PUT", "/v0/put/foo", nil)}, checks: checks( httpStatus(http.StatusForbidden), - bodyContains("Taildrop disabled"), + bodyContains("Taildrop disabled; no storage directory"), ), }, - { name: "bad_method", isSelf: true, @@ -482,18 +471,14 @@ func TestHandlePeerAPI(t *testing.T) { selfNode.CapMap = tailcfg.NodeCapMap{tailcfg.CapabilityDebug: nil} } var rootDir string - var fo FileOps if !tt.omitRoot { - var err error - if fo, err = newFileOps(t.TempDir()); err != nil { - t.Fatalf("newFileOps: %v", err) - } + rootDir = t.TempDir() } var e peerAPITestEnv e.taildrop = managerOptions{ - Logf: e.logBuf.Logf, - fileOps: fo, + Logf: e.logBuf.Logf, + Dir: rootDir, }.New() ext := &fakeExtension{ @@ -505,7 +490,9 @@ func TestHandlePeerAPI(t *testing.T) { 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() @@ -539,8 +526,8 @@ func TestHandlePeerAPI(t *testing.T) { func TestFileDeleteRace(t *testing.T) { dir := t.TempDir() taildropMgr := managerOptions{ - Logf: t.Logf, - fileOps: must.Get(newFileOps(dir)), + Logf: t.Logf, + Dir: dir, }.New() ph := &peerAPIHandler{ diff --git a/feature/taildrop/resume.go b/feature/taildrop/resume.go index 20ef527a6..211a1ff6b 100644 --- a/feature/taildrop/resume.go +++ b/feature/taildrop/resume.go @@ -9,6 +9,7 @@ import ( "encoding/hex" "fmt" "io" + "io/fs" "os" "strings" ) @@ -50,20 +51,19 @@ 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) ([]string, error) { - if m == nil || m.opts.fileOps == nil { +func (m *manager) PartialFiles(id clientID) (ret []string, err error) { + if m == nil || m.opts.Dir == "" { return nil, ErrNoTaildrop } + suffix := id.partialSuffix() - 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) + if err := rangeDir(m.opts.Dir, func(de fs.DirEntry) bool { + if name := de.Name(); strings.HasSuffix(name, suffix) { + ret = append(ret, name) } + return true + }); err != nil { + return ret, redactError(err) } return ret, nil } @@ -73,13 +73,17 @@ func (m *manager) PartialFiles(id clientID) ([]string, 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.fileOps == nil { + if m == nil || m.opts.Dir == "" { return nil, nil, ErrNoTaildrop } noopNext := func() (blockChecksum, error) { return blockChecksum{}, io.EOF } noopClose := func() error { return nil } - f, err := m.opts.fileOps.OpenReader(baseName + id.partialSuffix()) + dstFile, err := joinDir(m.opts.Dir, baseName) + if err != nil { + return nil, nil, err + } + f, err := os.Open(dstFile + 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 4e59d401d..dac3c657b 100644 --- a/feature/taildrop/resume_test.go +++ b/feature/taildrop/resume_test.go @@ -8,7 +8,6 @@ import ( "io" "math/rand" "os" - "path/filepath" "testing" "testing/iotest" @@ -20,9 +19,7 @@ func TestResume(t *testing.T) { defer func() { blockSize = oldBlockSize }() blockSize = 256 - dir := t.TempDir() - - m := managerOptions{Logf: t.Logf, fileOps: must.Get(newFileOps(dir))}.New() + m := managerOptions{Logf: t.Logf, Dir: t.TempDir()}.New() defer m.Shutdown() rn := rand.New(rand.NewSource(0)) @@ -40,7 +37,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(filepath.Join(dir, "foo"))) + got := must.Get(os.ReadFile(must.Get(joinDir(m.opts.Dir, "foo")))) if !bytes.Equal(got, want) { t.Errorf("content mismatches") } @@ -69,7 +66,7 @@ func TestResume(t *testing.T) { t.Fatalf("too many iterations to complete the test") } } - got := must.Get(os.ReadFile(filepath.Join(dir, "bar"))) + got := must.Get(os.ReadFile(must.Get(joinDir(m.opts.Dir, "bar")))) if !bytes.Equal(got, want) { t.Errorf("content mismatches") } diff --git a/feature/taildrop/retrieve.go b/feature/taildrop/retrieve.go index b048a1b3b..6fb975193 100644 --- a/feature/taildrop/retrieve.go +++ b/feature/taildrop/retrieve.go @@ -9,19 +9,19 @@ import ( "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() bool { - if m == nil || m.opts.fileOps == nil || m.opts.DirectFileMode { +func (m *manager) HasFilesWaiting() (has bool) { + if m == nil || m.opts.Dir == "" || m.opts.DirectFileMode { return false } @@ -30,67 +30,64 @@ func (m *manager) HasFilesWaiting() 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. - total := m.totalReceived.Load() - if total == m.emptySince.Load() { + totalReceived := m.totalReceived.Load() + if totalReceived == m.emptySince.Load() { return false } - files, err := m.opts.fileOps.ListFiles() - if err != nil { - return false - } - - // Build a set of filenames present in Dir - fileSet := set.Of(files...) - - for _, filename := range files { - if isPartialOrDeleted(filename) { - continue + // 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 } - if fileSet.Contains(filename + deletedSuffix) { - continue // already handled + _, err := os.Stat(filepath.Join(m.opts.Dir, name+deletedSuffix)) + if os.IsNotExist(err) { + has = true + return false } - // Found at least one downloadable file return true - } + }) - // No waiting files → update negative‑result cache - m.emptySince.Store(total) - return false + // 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) + } + return has } // 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() ([]apitype.WaitingFile, error) { - if m == nil || m.opts.fileOps == nil { +func (m *manager) WaitingFiles() (ret []apitype.WaitingFile, err error) { + if m == nil || m.opts.Dir == "" { return nil, ErrNoTaildrop } if m.opts.DirectFileMode { return nil, nil } - names, err := m.opts.fileOps.ListFiles() - if err != 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 { 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 } @@ -98,18 +95,21 @@ func (m *manager) WaitingFiles() ([]apitype.WaitingFile, 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.fileOps == nil { + if m == nil || m.opts.Dir == "" { 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 := m.opts.fileOps.Remove(baseName) + err := os.Remove(path) 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 := m.touchFile(baseName + deletedSuffix); err != nil { + if err := touchFile(path + deletedSuffix); err != nil { logf("peerapi: failed to leave deleted marker: %v", err) } m.deleter.Insert(baseName + deletedSuffix) @@ -141,31 +141,35 @@ func (m *manager) DeleteFile(baseName string) error { } } -func (m *manager) touchFile(name string) error { - wc, _, err := m.opts.fileOps.OpenWriter(name /* offset= */, 0, 0666) +func touchFile(path string) error { + f, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE, 0666) if err != nil { return redactError(err) } - return wc.Close() + return f.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.fileOps == nil { + if m == nil || m.opts.Dir == "" { return nil, 0, ErrNoTaildrop } if m.opts.DirectFileMode { return nil, 0, errors.New("opens not allowed in direct mode") } - if _, err := m.opts.fileOps.Stat(baseName + deletedSuffix); err == nil { - return nil, 0, redactError(&fs.PathError{Op: "open", Path: baseName, Err: fs.ErrNotExist}) + path, err := joinDir(m.opts.Dir, baseName) + if err != nil { + return nil, 0, err } - f, err := m.opts.fileOps.OpenReader(baseName) + 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) if err != nil { return nil, 0, redactError(err) } - fi, err := m.opts.fileOps.Stat(baseName) + fi, err := f.Stat() if err != nil { f.Close() return nil, 0, redactError(err) diff --git a/feature/taildrop/send.go b/feature/taildrop/send.go index 32ba5f6f0..59a1701da 100644 --- a/feature/taildrop/send.go +++ b/feature/taildrop/send.go @@ -4,8 +4,11 @@ package taildrop import ( + "crypto/sha256" "fmt" "io" + "os" + "path/filepath" "sync" "time" @@ -70,10 +73,9 @@ 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) (fileLength int64, err error) { - +func (m *manager) PutFile(id clientID, baseName string, r io.Reader, offset, length int64) (int64, error) { switch { - case m == nil || m.opts.fileOps == nil: + case m == nil || m.opts.Dir == "": return 0, ErrNoTaildrop case !envknob.CanTaildrop(): return 0, ErrNoTaildrop @@ -81,48 +83,48 @@ func (m *manager) PutFile(id clientID, baseName string, r io.Reader, offset, len return 0, ErrNotAccessible } - if err := validateBaseName(baseName); err != nil { - return 0, err + //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 } + m.deleter.Remove(filepath.Base(dstPath)) // avoid deleting the partial file while receiving - // and make sure we don't delete it while uploading: - m.deleter.Remove(baseName) + // 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) - // 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) + // Open writer & populate inFile paths + wc, partialPath, err := m.openWriterAndPaths(id, m.opts.Mode, inFile, baseName, dstPath, offset) if err != nil { return 0, m.redactAndLogError("Create", err) } defer func() { wc.Close() if err != nil { - m.deleter.Insert(partialName) // mark partial file for eventual deletion + m.deleter.Insert(filepath.Base(partialPath)) // 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. @@ -146,26 +148,220 @@ 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() - // 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) + // 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) + } } - 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 9ffa5fccc..8edb70417 100644 --- a/feature/taildrop/send_test.go +++ b/feature/taildrop/send_test.go @@ -4,64 +4,123 @@ 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 - directFileMode bool + name string + mode PutMode + setup func(t *testing.T) (*manager, string, *mockFileOps) + wantFile string }{ - {"DirectFileMode", true}, - {"NonDirectFileMode", false}, + { + 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", + }, } - 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() + 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)) - id := clientID("0") - n, err := mgr.PutFile(id, "file.txt", strings.NewReader(content), 0, int64(len(content))) + n, err := mgr.PutFile(id, "file.txt", reader, 0, int64(len(content))) if err != nil { - t.Fatalf("PutFile error: %v", err) + t.Fatalf("PutFile(%s) error: %v", tc.name, err) } if n != int64(len(content)) { t.Errorf("wrote %d bytes; want %d", n, len(content)) } - path := filepath.Join(dir, "file.txt") + 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) + } - 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()) + 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) } } }) diff --git a/feature/taildrop/taildrop.go b/feature/taildrop/taildrop.go index 6c3deaed1..2dfa415bb 100644 --- a/feature/taildrop/taildrop.go +++ b/feature/taildrop/taildrop.go @@ -12,6 +12,8 @@ package taildrop import ( "errors" "hash/adler32" + "io" + "io/fs" "os" "path" "path/filepath" @@ -19,6 +21,7 @@ import ( "sort" "strconv" "strings" + "sync" "sync/atomic" "unicode" "unicode/utf8" @@ -69,6 +72,11 @@ 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. @@ -83,10 +91,9 @@ type managerOptions struct { // copy them out, and then delete them. DirectFileMode bool - // 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 + FileOps FileOps + + Mode PutMode // SendFileNotify is called periodically while a file is actively // receiving the contents for the file. There is a final call @@ -104,6 +111,9 @@ 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 @@ -127,6 +137,11 @@ 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() { @@ -157,29 +172,57 @@ func isPartialOrDeleted(s string) bool { return strings.HasSuffix(s, deletedSuffix) || strings.HasSuffix(s, partialSuffix) } -func validateBaseName(name string) error { - if !utf8.ValidString(name) || - strings.TrimSpace(name) != name || - len(name) > 255 { - return ErrInvalidFileName +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 } // TODO: validate unicode normalization form too? Varies by platform. - clean := path.Clean(name) - if clean != name || clean == "." || clean == ".." { - return ErrInvalidFileName + clean := path.Clean(baseName) + if clean != baseName || + clean == "." || clean == ".." || + isPartialOrDeleted(clean) { + return "", ErrInvalidFileName } - if isPartialOrDeleted(name) { - return ErrInvalidFileName - } - for _, r := range name { + for _, r := range baseName { if !validFilenameRune(r) { - return ErrInvalidFileName + return "", ErrInvalidFileName } } - if !filepath.IsLocal(name) { - 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 + } } - 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 0d77273f0..da0bd2f43 100644 --- a/feature/taildrop/taildrop_test.go +++ b/feature/taildrop/taildrop_test.go @@ -4,10 +4,40 @@ 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 @@ -37,29 +67,3 @@ 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) - } - } -}