ipn/ipnlocal: use delete marker files to work around Windows delete problems

If DeleteFile fails on Windows due to another process (anti-virus,
probably) having our file open, instead leave a marker file that the
file is logically deleted, and remove it from API calls and clean it
up lazily later.

Updates tailscale/corp#1626

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>

ipn/ipnlocal/peerapi.go

@@ -11,6 +11,7 @@ import (
 	"hash/crc32"
 	"html"
 	"io"
+	"io/fs"
 	"net"
 	"net/http"
 	"net/url"
@@ -18,6 +19,7 @@ import (
 	"path"
 	"path/filepath"
 	"runtime"
+	"sort"
 	"strconv"
 	"strings"
 	"sync"
@@ -51,7 +53,17 @@ type peerAPIServer struct {
 	directFileMode bool
 }
 
-const partialSuffix = ".partial"
+const (
+	partialSuffix = ".partial"
+
+	// deletedSuffix is the suffix for a deleted marker file
+	// that's placed next to a file (without the suffix) that we
+	// tried to delete, but Windows wouldn't let us. These are
+	// only written on Windows (and in tests), but they're not
+	// permitted to be uploaded directly on any platform, like
+	// partial files.
+	deletedSuffix = ".deleted"
+)
 
 func validFilenameRune(r rune) bool {
 	switch r {
@@ -84,6 +96,7 @@ func (s *peerAPIServer) diskPath(baseName string) (fullPath string, ok bool) {
 	clean := path.Clean(baseName)
 	if clean != baseName ||
 		clean == "." || clean == ".." ||
+		strings.HasSuffix(clean, deletedSuffix) ||
 		strings.HasSuffix(clean, partialSuffix) {
 		return "", false
 	}
@@ -117,12 +130,29 @@ func (s *peerAPIServer) hasFilesWaiting() bool {
 	for {
 		des, err := f.ReadDir(10)
 		for _, de := range des {
-			if strings.HasSuffix(de.Name(), partialSuffix) {
+			name := de.Name()
+			if strings.HasSuffix(name, partialSuffix) {
+				continue
+			}
+			if strings.HasSuffix(name, deletedSuffix) { // for Windows + tests
+				// After we're done looping over files, then try
+				// to delete this file. Don't do it proactively,
+				// as the OS may return "foo.jpg.deleted" before "foo.jpg"
+				// and we don't want to delete the ".deleted" file before
+				// enumerating to the "foo.jpg" file.
+				defer tryDeleteAgain(filepath.Join(s.rootDir, strings.TrimSuffix(name, deletedSuffix)))
 				continue
 			}
 			if de.Type().IsRegular() {
+				_, err := os.Stat(filepath.Join(s.rootDir, name+deletedSuffix))
+				if os.IsNotExist(err) {
 					return true
 				}
+				if err == nil {
+					tryDeleteAgain(filepath.Join(s.rootDir, name))
+					continue
+				}
+			}
 		}
 		if err == io.EOF {
 			s.knownEmpty.Set(true)
@@ -134,6 +164,12 @@ func (s *peerAPIServer) hasFilesWaiting() bool {
 	return false
 }
 
+// WaitingFiles returns the list of files that have been sent by a
+// peer that are waiting in the buffered "pick up" directory owned by
+// the Tailscale daemon.
+//
+// As a side effect, it also does any lazy deletion of files as
+// required by Windows.
 func (s *peerAPIServer) WaitingFiles() (ret []apitype.WaitingFile, err error) {
 	if s.rootDir == "" {
 		return nil, errors.New("peerapi disabled; no storage configured")
@@ -146,6 +182,7 @@ func (s *peerAPIServer) WaitingFiles() (ret []apitype.WaitingFile, err error) {
 		return nil, err
 	}
 	defer f.Close()
+	var deleted map[string]bool // "foo.jpg" => true (if "foo.jpg.deleted" exists)
 	for {
 		des, err := f.ReadDir(10)
 		for _, de := range des {
@@ -153,6 +190,13 @@ func (s *peerAPIServer) WaitingFiles() (ret []apitype.WaitingFile, err error) {
 			if strings.HasSuffix(name, partialSuffix) {
 				continue
 			}
+			if strings.HasSuffix(name, deletedSuffix) { // for Windows + tests
+				if deleted == nil {
+					deleted = map[string]bool{}
+				}
+				deleted[strings.TrimSuffix(name, deletedSuffix)] = true
+				continue
+			}
 			if de.Type().IsRegular() {
 				fi, err := de.Info()
 				if err != nil {
@@ -171,9 +215,41 @@ func (s *peerAPIServer) WaitingFiles() (ret []apitype.WaitingFile, err error) {
 			return nil, err
 		}
 	}
+	if len(deleted) > 0 {
+		// Filter out any return values "foo.jpg" where a
+		// "foo.jpg.deleted" marker file exists on disk.
+		all := ret
+		ret = ret[:0]
+		for _, wf := range all {
+			if !deleted[wf.Name] {
+				ret = append(ret, wf)
+			}
+		}
+		// And do some opportunistic deleting while we're here.
+		// Maybe Windows is done virus scanning the file we tried
+		// to delete a long time ago and will let us delete it now.
+		for name := range deleted {
+			tryDeleteAgain(filepath.Join(s.rootDir, name))
+		}
+	}
+	sort.Slice(ret, func(i, j int) bool { return ret[i].Name < ret[j].Name })
 	return ret, nil
 }
 
+// tryDeleteAgain tries to delete path (and path+deletedSuffix) after
+// it failed earlier.  This happens on Windows when various anti-virus
+// tools hook into filesystem operations and have the file open still
+// while we're trying to delete it. In that case we instead mark it as
+// deleted (writing a "foo.jpg.deleted" marker file), but then we
+// later try to clean them up.
+//
+// fullPath is the full path to the file without the deleted suffix.
+func tryDeleteAgain(fullPath string) {
+	if err := os.Remove(fullPath); err == nil || os.IsNotExist(err) {
+		os.Remove(fullPath + deletedSuffix)
+	}
+}
+
 func (s *peerAPIServer) DeleteFile(baseName string) error {
 	if s.rootDir == "" {
 		return errors.New("peerapi disabled; no storage configured")
@@ -191,32 +267,28 @@ func (s *peerAPIServer) DeleteFile(baseName string) error {
 	for {
 		err := os.Remove(path)
 		if err != nil && !os.IsNotExist(err) {
-			if pe, ok := err.(*os.PathError); ok {
-				pe.Path = "redact"
-			}
+			err = redactErr(err)
 			// Put a retry loop around deletes on Windows. Windows
 			// file descriptor closes are effectively asynchronous,
 			// as a bunch of hooks run on/after close, and we can't
 			// necessarily delete the file for a while after close,
 			// as we need to wait for everybody to be done with
 			// it. (on Windows, unlike Unix, a file can't be deleted
-			// while open)
-			//
-			// TODO(bradfitz): we might instead want to just keep a
-			// map of logically deleted files and filter them out in
-			// WaitingFiles/OpenFile.  Then we can keep trying this
-			// delete in the background and/or in response to future
-			// WaitingFiles/OpenFile calls, and then remove from the
-			// logicallyDeleted map. But let's start with this retry
-			// loop.
+			// if it's open anywhere)
+			// So try a few times but ultimately just leave a
+			// "foo.jpg.deleted" marker file to note that it's
+			// deleted and we clean it up later.
 			if runtime.GOOS == "windows" {
 				if bo == nil {
 					bo = backoff.NewBackoff("delete-retry", logf, 1*time.Second)
 				}
-				if time.Since(t0) < 10*time.Second {
+				if time.Since(t0) < 5*time.Second {
 					bo.BackOff(context.Background(), err)
 					continue
 				}
+				if err := redactErr(touchFile(path + deletedSuffix)); err != nil {
+					logf("peerapi: failed to leave deleted marker: %v", err)
+				}
 			}
 			logf("peerapi: failed to DeleteFile: %v", err)
 			return err
@@ -225,6 +297,21 @@ func (s *peerAPIServer) DeleteFile(baseName string) error {
 	}
 }
 
+func redactErr(err error) error {
+	if pe, ok := err.(*os.PathError); ok {
+		pe.Path = "redacted"
+	}
+	return err
+}
+
+func touchFile(path string) error {
+	f, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE, 0666)
+	if err != nil {
+		return err
+	}
+	return f.Close()
+}
+
 func (s *peerAPIServer) OpenFile(baseName string) (rc io.ReadCloser, size int64, err error) {
 	if s.rootDir == "" {
 		return nil, 0, errors.New("peerapi disabled; no storage configured")
@@ -236,6 +323,10 @@ func (s *peerAPIServer) OpenFile(baseName string) (rc io.ReadCloser, size int64,
 	if !ok {
 		return nil, 0, errors.New("bad filename")
 	}
+	if fi, err := os.Stat(path + deletedSuffix); err == nil && fi.Mode().IsRegular() {
+		tryDeleteAgain(path)
+		return nil, 0, &fs.PathError{Op: "open", Path: path, Err: fs.ErrNotExist}
+	}
 	f, err := os.Open(path)
 	if err != nil {
 		return nil, 0, err

ipn/ipnlocal/peerapi_test.go

@@ -15,6 +15,7 @@ import (
 	"net/http/httptest"
 	"os"
 	"path/filepath"
+	"runtime"
 	"strings"
 	"testing"
 
@@ -235,6 +236,16 @@ func TestHandlePeerAPI(t *testing.T) {
 				bodyContains("bad filename"),
 			),
 		},
+		{
+			name:       "bad_filename_deleted",
+			isSelf:     true,
+			capSharing: true,
+			req:        httptest.NewRequest("PUT", "/v0/put/foo.deleted", nil),
+			checks: checks(
+				httpStatus(400),
+				bodyContains("bad filename"),
+			),
+		},
 		{
 			name:       "bad_filename_dot",
 			isSelf:     true,
@@ -476,3 +487,87 @@ func TestFileDeleteRace(t *testing.T) {
 		}
 	}
 }
+
+// Tests "foo.jpg.deleted" marks (for Windows).
+func TestDeletedMarkers(t *testing.T) {
+	dir := t.TempDir()
+	ps := &peerAPIServer{
+		b: &LocalBackend{
+			logf:           t.Logf,
+			capFileSharing: true,
+		},
+		rootDir: dir,
+	}
+
+	nothingWaiting := func() {
+		t.Helper()
+		ps.knownEmpty.Set(false)
+		if ps.hasFilesWaiting() {
+			t.Fatal("unexpected files waiting")
+		}
+	}
+	touch := func(base string) {
+		t.Helper()
+		if err := touchFile(filepath.Join(dir, base)); err != nil {
+			t.Fatal(err)
+		}
+	}
+	wantEmptyTempDir := func() {
+		t.Helper()
+		if fis, err := ioutil.ReadDir(dir); err != nil {
+			t.Fatal(err)
+		} else if len(fis) > 0 && runtime.GOOS != "windows" {
+			for _, fi := range fis {
+				t.Errorf("unexpected file in tempdir: %q", fi.Name())
+			}
+		}
+	}
+
+	nothingWaiting()
+	wantEmptyTempDir()
+
+	touch("foo.jpg.deleted")
+	nothingWaiting()
+	wantEmptyTempDir()
+
+	touch("foo.jpg.deleted")
+	touch("foo.jpg")
+	nothingWaiting()
+	wantEmptyTempDir()
+
+	touch("foo.jpg.deleted")
+	touch("foo.jpg")
+	wf, err := ps.WaitingFiles()
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(wf) != 0 {
+		t.Fatalf("WaitingFiles = %d; want 0", len(wf))
+	}
+	wantEmptyTempDir()
+
+	touch("foo.jpg.deleted")
+	touch("foo.jpg")
+	if rc, _, err := ps.OpenFile("foo.jpg"); err == nil {
+		rc.Close()
+		t.Fatal("unexpected foo.jpg open")
+	}
+	wantEmptyTempDir()
+
+	// And verify basics still work in non-deleted cases.
+	touch("foo.jpg")
+	touch("bar.jpg.deleted")
+	if wf, err := ps.WaitingFiles(); err != nil {
+		t.Error(err)
+	} else if len(wf) != 1 {
+		t.Errorf("WaitingFiles = %d; want 1", len(wf))
+	} else if wf[0].Name != "foo.jpg" {
+		t.Errorf("unexpected waiting file %+v", wf[0])
+	}
+	if rc, _, err := ps.OpenFile("foo.jpg"); err != nil {
+		t.Fatal(err)
+	} else {
+		rc.Close()
+	}
+
+}