lib/scanner, lib/fs: Don't create file infos with abs paths (fixes #4799) (#4800)

cmd/syncthing/main.go

@@ -770,21 +770,8 @@ func syncthingMain(runtimeOptions RuntimeOptions) {
 		miscDB.PutString("prevVersion", Version)
 	}
 
-	if cfg.RawCopy().OriginalVersion < 19 {
-		// Converts old symlink types to new in the entire database.
-		ldb.ConvertSymlinkTypes()
-	}
-	if cfg.RawCopy().OriginalVersion < 26 {
-		// Adds invalid (ignored) files to global list of files
-		changed := 0
-		for folderID, folderCfg := range folders {
-			changed += ldb.AddInvalidToGlobal([]byte(folderID), protocol.LocalDeviceID[:])
-			for _, deviceCfg := range folderCfg.Devices {
-				changed += ldb.AddInvalidToGlobal([]byte(folderID), deviceCfg.DeviceID[:])
-			}
-		}
-		l.Infof("Database update: Added %d ignored files to the global list", changed)
-	}
+	// Potential database transitions
+	ldb.UpdateSchema()
 
 	m := model.NewModel(cfg, myID, "syncthing", Version, ldb, protectedFiles)
 

lib/db/leveldb.go

@@ -15,6 +15,8 @@ import (
 	"github.com/syndtr/goleveldb/leveldb/opt"
 )
 
+const dbVersion = 1
+
 const (
 	KeyTypeDevice = iota
 	KeyTypeGlobal

lib/db/leveldb_dbinstance.go

@@ -83,6 +83,20 @@ func newDBInstance(db *leveldb.DB, location string) *Instance {
 	return i
 }
 
+// UpdateSchema does transitions to the current db version if necessary
+func (db *Instance) UpdateSchema() {
+	miscDB := NewNamespacedKV(db, string(KeyTypeMiscData))
+	prevVersion, _ := miscDB.Int64("dbVersion")
+
+	if prevVersion == 0 {
+		db.updateSchema0to1()
+	}
+
+	if prevVersion != dbVersion {
+		miscDB.PutInt64("dbVersion", dbVersion)
+	}
+}
+
 // Committed returns the number of items committed to the database since startup
 func (db *Instance) Committed() int64 {
 	return atomic.LoadInt64(&db.committed)
@@ -525,21 +539,39 @@ func (db *Instance) checkGlobals(folder []byte, meta *metadataTracker) {
 	l.Debugf("db check completed for %q", folder)
 }
 
-// ConvertSymlinkTypes should be run once only on an old database. It
-// changes SYMLINK_FILE and SYMLINK_DIRECTORY types to the current SYMLINK
-// type (previously SYMLINK_UNKNOWN). It does this for all devices, both
-// local and remote, and does not reset delta indexes. It shouldn't really
-// matter what the symlink type is, but this cleans it up for a possible
-// future when SYMLINK_FILE and SYMLINK_DIRECTORY are no longer understood.
-func (db *Instance) ConvertSymlinkTypes() {
+func (db *Instance) updateSchema0to1() {
 	t := db.newReadWriteTransaction()
 	defer t.close()
 
 	dbi := t.NewIterator(util.BytesPrefix([]byte{KeyTypeDevice}), nil)
 	defer dbi.Release()
 
-	conv := 0
+	symlinkConv := 0
+	changedFolders := make(map[string]struct{})
+	ignAdded := 0
+	meta := newMetadataTracker() // dummy metadata tracker
+
 	for dbi.Next() {
+		folder := db.deviceKeyFolder(dbi.Key())
+		device := db.deviceKeyDevice(dbi.Key())
+		name := string(db.deviceKeyName(dbi.Key()))
+
+		// Remove files with absolute path (see #4799)
+		if strings.HasPrefix(name, "/") {
+			if _, ok := changedFolders[string(folder)]; !ok {
+				changedFolders[string(folder)] = struct{}{}
+			}
+			t.removeFromGlobal(folder, device, nil, nil)
+			t.Delete(dbi.Key())
+			t.checkFlush()
+			continue
+		}
+
+		// Change SYMLINK_FILE and SYMLINK_DIRECTORY types to the current SYMLINK
+		// type (previously SYMLINK_UNKNOWN). It does this for all devices, both
+		// local and remote, and does not reset delta indexes. It shouldn't really
+		// matter what the symlink type is, but this cleans it up for a possible
+		// future when SYMLINK_FILE and SYMLINK_DIRECTORY are no longer understood.
 		var f protocol.FileInfo
 		if err := f.Unmarshal(dbi.Value()); err != nil {
 			// probably can't happen
@@ -553,99 +585,25 @@ func (db *Instance) ConvertSymlinkTypes() {
 			}
 			t.Put(dbi.Key(), bs)
 			t.checkFlush()
-			conv++
+			symlinkConv++
 		}
-	}
 
-	l.Infof("Updated symlink type for %d index entries", conv)
-}
-
-// AddInvalidToGlobal searches for invalid files and adds them to the global list.
-// Invalid files exist in the db if they once were not ignored and subsequently
-// ignored. In the new system this is still valid, but invalid files must also be
-// in the global list such that they cannot be mistaken for missing files.
-func (db *Instance) AddInvalidToGlobal(folder, device []byte) int {
-	t := db.newReadWriteTransaction()
-	defer t.close()
-
-	dbi := t.NewIterator(util.BytesPrefix(db.deviceKey(folder, device, nil)[:keyPrefixLen+keyFolderLen+keyDeviceLen]), nil)
-	defer dbi.Release()
-
-	changed := 0
-	for dbi.Next() {
-		var file protocol.FileInfo
-		if err := file.Unmarshal(dbi.Value()); err != nil {
-			// probably can't happen
-			continue
-		}
-		if file.Invalid {
-			changed++
-
-			l.Debugf("add invalid to global; folder=%q device=%v file=%q version=%v", folder, protocol.DeviceIDFromBytes(device), file.Name, file.Version)
-
-			// this is an adapted version of readWriteTransaction.updateGlobal
-			name := []byte(file.Name)
-			gk := t.db.globalKey(folder, name)
-
-			var fl VersionList
-			if svl, err := t.Get(gk, nil); err == nil {
-				fl.Unmarshal(svl) // skip error, range handles success case
-			}
-
-			nv := FileVersion{
-				Device:  device,
-				Version: file.Version,
-				Invalid: file.Invalid,
-			}
-
-			inserted := false
-			// Find a position in the list to insert this file. The file at the front
-			// of the list is the newer, the "global".
-		insert:
-			for i := range fl.Versions {
-				switch fl.Versions[i].Version.Compare(file.Version) {
-				case protocol.Equal:
-					// Invalid files should go after a valid file of equal version
-					if nv.Invalid {
-						continue insert
-					}
-					fallthrough
-
-				case protocol.Lesser:
-					// The version at this point in the list is equal to or lesser
-					// ("older") than us. We insert ourselves in front of it.
-					fl.Versions = insertVersion(fl.Versions, i, nv)
-					inserted = true
-					break insert
-
-				case protocol.ConcurrentLesser, protocol.ConcurrentGreater:
-					// The version at this point is in conflict with us. We must pull
-					// the actual file metadata to determine who wins. If we win, we
-					// insert ourselves in front of the loser here. (The "Lesser" and
-					// "Greater" in the condition above is just based on the device
-					// IDs in the version vector, which is not the only thing we use
-					// to determine the winner.)
-					//
-					// A surprise missing file entry here is counted as a win for us.
-					of, ok := t.getFile(folder, fl.Versions[i].Device, name)
-					if !ok || file.WinsConflict(of) {
-						fl.Versions = insertVersion(fl.Versions, i, nv)
-						inserted = true
-						break insert
-					}
+		// Add invalid files to global list
+		if f.Invalid {
+			if t.updateGlobal(folder, device, f, meta) {
+				if _, ok := changedFolders[string(folder)]; !ok {
+					changedFolders[string(folder)] = struct{}{}
 				}
+				ignAdded++
 			}
-
-			if !inserted {
-				// We didn't find a position for an insert above, so append to the end.
-				fl.Versions = append(fl.Versions, nv)
-			}
-
-			t.Put(gk, mustMarshal(&fl))
 		}
 	}
 
-	return changed
+	for folder := range changedFolders {
+		db.dropFolderMeta([]byte(folder))
+	}
+
+	l.Infof("Updated symlink type for %d index entries and added %d invalid files to global list", symlinkConv, ignAdded)
 }
 
 // deviceKey returns a byte slice encoding the following information:

lib/fs/basicfs.go

@@ -88,28 +88,10 @@ func (f *BasicFilesystem) rooted(rel string) (string, error) {
 		expectedPrefix += pathSep
 	}
 
-	// The relative path should be clean from internal dotdots and similar
-	// funkyness.
-	rel = filepath.FromSlash(rel)
-	if filepath.Clean(rel) != rel {
-		return "", ErrInvalidFilename
-	}
-
-	// It is not acceptable to attempt to traverse upwards.
-	switch rel {
-	case "..", pathSep:
-		return "", ErrNotRelative
-	}
-	if strings.HasPrefix(rel, ".."+pathSep) {
-		return "", ErrNotRelative
-	}
-
-	if strings.HasPrefix(rel, pathSep+pathSep) {
-		// The relative path may pretend to be an absolute path within the
-		// root, but the double path separator on Windows implies something
-		// else. It would get cleaned by the Join below, but it's out of
-		// spec anyway.
-		return "", ErrNotRelative
+	var err error
+	rel, err = Canonicalize(rel)
+	if err != nil {
+		return "", err
 	}
 
 	// The supposedly correct path is the one filepath.Join will return, as

lib/fs/basicfs_test.go

@@ -364,17 +364,17 @@ func TestRooted(t *testing.T) {
 		{"baz/foo/", "bar/baz", "baz/foo/bar/baz", true},
 		{"baz/foo/", "/bar/baz", "baz/foo/bar/baz", true},
 
-		// Not escape attempts, but oddly formatted relative paths. Disallowed.
-		{"foo", "./bar", "", false},
-		{"baz/foo", "./bar", "", false},
-		{"foo", "./bar/baz", "", false},
-		{"baz/foo", "./bar/baz", "", false},
-		{"baz/foo", "bar/../baz", "", false},
-		{"baz/foo", "/bar/../baz", "", false},
-		{"baz/foo", "./bar/../baz", "", false},
-		{"baz/foo", "bar/../baz", "", false},
-		{"baz/foo", "/bar/../baz", "", false},
-		{"baz/foo", "./bar/../baz", "", false},
+		// Not escape attempts, but oddly formatted relative paths.
+		{"foo", "", "foo/", true},
+		{"foo", "/", "foo/", true},
+		{"foo", "/..", "foo/", true},
+		{"foo", "./bar", "foo/bar", true},
+		{"baz/foo", "./bar", "baz/foo/bar", true},
+		{"foo", "./bar/baz", "foo/bar/baz", true},
+		{"baz/foo", "./bar/baz", "baz/foo/bar/baz", true},
+		{"baz/foo", "bar/../baz", "baz/foo/baz", true},
+		{"baz/foo", "/bar/../baz", "baz/foo/baz", true},
+		{"baz/foo", "./bar/../baz", "baz/foo/baz", true},
 
 		// Results in an allowed path, but does it by probing. Disallowed.
 		{"foo", "../foo", "", false},
@@ -385,10 +385,7 @@ func TestRooted(t *testing.T) {
 		{"baz/foo", "bar/../../../baz/foo/bar", "", false},
 
 		// Escape attempts.
-		{"foo", "", "", false},
-		{"foo", "/", "", false},
 		{"foo", "..", "", false},
-		{"foo", "/..", "", false},
 		{"foo", "../", "", false},
 		{"foo", "../bar", "", false},
 		{"foo", "../foobar", "", false},
@@ -413,8 +410,8 @@ func TestRooted(t *testing.T) {
 		{"/", "/foo", "/foo", true},
 		{"/", "../foo", "", false},
 		{"/", "..", "", false},
-		{"/", "/", "", false},
-		{"/", "", "", false},
+		{"/", "/", "/", true},
+		{"/", "", "/", true},
 
 		// special case for filesystems to be able to MkdirAll('.') for example
 		{"/", ".", "/", true},
@@ -427,11 +424,11 @@ func TestRooted(t *testing.T) {
 			{`c:\`, `\foo`, `c:\foo`, true},
 			{`\\?\c:\`, `\foo`, `\\?\c:\foo`, true},
 			{`c:\`, `\\foo`, ``, false},
-			{`c:\`, ``, ``, false},
-			{`c:\`, `\`, ``, false},
+			{`c:\`, ``, `c:\`, true},
+			{`c:\`, `\`, `c:\`, true},
 			{`\\?\c:\`, `\\foo`, ``, false},
-			{`\\?\c:\`, ``, ``, false},
-			{`\\?\c:\`, `\`, ``, false},
+			{`\\?\c:\`, ``, `\\?\c:\`, true},
+			{`\\?\c:\`, `\`, `\\?\c:\`, true},
 
 			// makes no sense, but will be treated simply as a bad filename
 			{`c:\foo`, `d:\bar`, `c:\foo\d:\bar`, true},

lib/fs/filesystem.go

@@ -198,3 +198,39 @@ func IsInternal(file string) bool {
 	}
 	return false
 }
+
+// Canonicalize checks that the file path is valid and returns it in the "canonical" form:
+// - /foo/bar -> foo/bar
+// - / -> "."
+func Canonicalize(file string) (string, error) {
+	pathSep := string(PathSeparator)
+
+	if strings.HasPrefix(file, pathSep+pathSep) {
+		// The relative path may pretend to be an absolute path within
+		// the root, but the double path separator on Windows implies
+		// something else and is out of spec.
+		return "", ErrNotRelative
+	}
+
+	// The relative path should be clean from internal dotdots and similar
+	// funkyness.
+	file = filepath.Clean(file)
+
+	// It is not acceptable to attempt to traverse upwards.
+	switch file {
+	case "..":
+		return "", ErrNotRelative
+	}
+	if strings.HasPrefix(file, ".."+pathSep) {
+		return "", ErrNotRelative
+	}
+
+	if strings.HasPrefix(file, pathSep) {
+		if file == pathSep {
+			return ".", nil
+		}
+		return file[1:], nil
+	}
+
+	return file, nil
+}

lib/fs/filesystem_test.go

@@ -41,3 +41,60 @@ func TestIsInternal(t *testing.T) {
 		}
 	}
 }
+
+func TestCanonicalize(t *testing.T) {
+	type testcase struct {
+		path     string
+		expected string
+		ok       bool
+	}
+	cases := []testcase{
+		// Valid cases
+		{"/bar", "bar", true},
+		{"/bar/baz", "bar/baz", true},
+		{"bar", "bar", true},
+		{"bar/baz", "bar/baz", true},
+
+		// Not escape attempts, but oddly formatted relative paths
+		{"", ".", true},
+		{"/", ".", true},
+		{"/..", ".", true},
+		{"./bar", "bar", true},
+		{"./bar/baz", "bar/baz", true},
+		{"bar/../baz", "baz", true},
+		{"/bar/../baz", "baz", true},
+		{"./bar/../baz", "baz", true},
+
+		// Results in an allowed path, but does it by probing. Disallowed.
+		{"../foo", "", false},
+		{"../foo/bar", "", false},
+		{"../foo/bar", "", false},
+		{"../../baz/foo/bar", "", false},
+		{"bar/../../foo/bar", "", false},
+		{"bar/../../../baz/foo/bar", "", false},
+
+		// Escape attempts.
+		{"..", "", false},
+		{"../", "", false},
+		{"../bar", "", false},
+		{"../foobar", "", false},
+		{"bar/../../quux/baz", "", false},
+	}
+
+	for _, tc := range cases {
+		res, err := Canonicalize(tc.path)
+		if tc.ok {
+			if err != nil {
+				t.Errorf("Unexpected error for Canonicalize(%q): %v", tc.path, err)
+				continue
+			}
+			exp := filepath.FromSlash(tc.expected)
+			if res != exp {
+				t.Errorf("Unexpected result for Canonicalize(%q): %q != expected %q", tc.path, res, exp)
+			}
+		} else if err == nil {
+			t.Errorf("Unexpected pass for Canonicalize(%q) => %q", tc.path, res)
+			continue
+		}
+	}
+}

lib/fs/walkfs.go

@@ -38,7 +38,12 @@ func NewWalkFilesystem(next Filesystem) Filesystem {
 
 // walk recursively descends path, calling walkFn.
 func (f *walkFilesystem) walk(path string, info FileInfo, walkFn WalkFunc) error {
-	err := walkFn(path, info, nil)
+	path, err := Canonicalize(path)
+	if err != nil {
+		return err
+	}
+
+	err = walkFn(path, info, nil)
 	if err != nil {
 		if info.IsDir() && err == SkipDir {
 			return nil

lib/model/model.go

@@ -2784,7 +2784,12 @@ func unifySubs(dirs []string, exists func(dir string) bool) []string {
 	}
 	prev := "./" // Anything that can't be parent of a clean path
 	for i := 0; i < len(dirs); {
-		dir := filepath.Clean(dirs[i])
+		dir, err := fs.Canonicalize(dirs[i])
+		if err != nil {
+			l.Debugf("Skipping %v for scan: %s", dirs[i], err)
+			dirs = append(dirs[:i], dirs[i+1:]...)
+			continue
+		}
 		if dir == prev || strings.HasPrefix(dir, prev+string(fs.PathSeparator)) {
 			dirs = append(dirs[:i], dirs[i+1:]...)
 			continue

lib/model/model_test.go

@@ -2159,6 +2159,12 @@ func unifySubsCases() []unifySubsCase {
 			[]string{"foo"},
 			nil,
 		},
+		{
+			// 10. absolute path
+			[]string{"/foo"},
+			[]string{"foo"},
+			[]string{"foo"},
+		},
 	}
 
 	if runtime.GOOS == "windows" {

lib/scanner/walk_test.go

@@ -498,6 +498,30 @@ func TestStopWalk(t *testing.T) {
 	}
 }
 
+func TestIssue4799(t *testing.T) {
+	tmp, err := ioutil.TempDir("", "")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer os.RemoveAll(tmp)
+
+	fs := fs.NewFilesystem(fs.FilesystemTypeBasic, tmp)
+
+	fd, err := fs.Create("foo")
+	if err != nil {
+		t.Fatal(err)
+	}
+	fd.Close()
+
+	files, err := walkDir(fs, "/foo")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(files) != 1 || files[0].Name != "foo" {
+		t.Error(`Received unexpected file infos when walking "/foo"`, files)
+	}
+}
+
 // Verify returns nil or an error describing the mismatch between the block
 // list and actual reader contents
 func verify(r io.Reader, blocksize int, blocks []protocol.BlockInfo) error {