From bf165d680f38dcbf08fffd3a1bf2cd42da8c2217 Mon Sep 17 00:00:00 2001 From: Simon Frei Date: Mon, 12 Mar 2018 13:18:59 +0100 Subject: [PATCH] lib/scanner, lib/fs: Don't create file infos with abs paths (fixes #4799) (#4800) --- cmd/syncthing/main.go | 18 +---- lib/db/leveldb.go | 3 + lib/db/leveldb_dbinstance.go | 146 +++++++++++++---------------------- lib/fs/basicfs.go | 26 +------ lib/fs/basicfs_test.go | 37 ++++----- lib/fs/filesystem.go | 36 +++++++++ lib/fs/filesystem_test.go | 57 ++++++++++++++ lib/fs/walkfs.go | 7 +- lib/model/model.go | 7 +- lib/model/model_test.go | 6 ++ lib/scanner/walk_test.go | 24 ++++++ 11 files changed, 214 insertions(+), 153 deletions(-) diff --git a/cmd/syncthing/main.go b/cmd/syncthing/main.go index 0372b22f8..6f3bb44e9 100644 --- a/cmd/syncthing/main.go +++ b/cmd/syncthing/main.go @@ -752,21 +752,9 @@ func syncthingMain(runtimeOptions RuntimeOptions) { // have been incorrectly ignore filtered. ldb.DropDeltaIndexIDs() } - 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) diff --git a/lib/db/leveldb.go b/lib/db/leveldb.go index e30fd6a7b..e6ee43e66 100644 --- a/lib/db/leveldb.go +++ b/lib/db/leveldb.go @@ -15,6 +15,8 @@ import ( "github.com/syndtr/goleveldb/leveldb/opt" ) +const dbVersion = 1 + const ( KeyTypeDevice = iota KeyTypeGlobal @@ -26,6 +28,7 @@ const ( KeyTypeDeviceIdx KeyTypeIndexID KeyTypeFolderMeta + KeyTypeMiscData ) func (l VersionList) String() string { diff --git a/lib/db/leveldb_dbinstance.go b/lib/db/leveldb_dbinstance.go index 0c6a9ce0f..ce7b9fd1c 100644 --- a/lib/db/leveldb_dbinstance.go +++ b/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) @@ -531,21 +545,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 @@ -559,99 +591,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: diff --git a/lib/fs/basicfs.go b/lib/fs/basicfs.go index 133d032d7..8ee661c02 100644 --- a/lib/fs/basicfs.go +++ b/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 diff --git a/lib/fs/basicfs_test.go b/lib/fs/basicfs_test.go index 74313d62c..65f2d0c5b 100644 --- a/lib/fs/basicfs_test.go +++ b/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}, diff --git a/lib/fs/filesystem.go b/lib/fs/filesystem.go index 112d2f897..9731755dc 100644 --- a/lib/fs/filesystem.go +++ b/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 +} diff --git a/lib/fs/filesystem_test.go b/lib/fs/filesystem_test.go index 66514364a..46ca26716 100644 --- a/lib/fs/filesystem_test.go +++ b/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 + } + } +} diff --git a/lib/fs/walkfs.go b/lib/fs/walkfs.go index 005c7e262..e56555932 100644 --- a/lib/fs/walkfs.go +++ b/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 diff --git a/lib/model/model.go b/lib/model/model.go index 304f33e43..ad0785a51 100644 --- a/lib/model/model.go +++ b/lib/model/model.go @@ -2777,7 +2777,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 diff --git a/lib/model/model_test.go b/lib/model/model_test.go index 309210c97..4c38b41d4 100644 --- a/lib/model/model_test.go +++ b/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" { diff --git a/lib/scanner/walk_test.go b/lib/scanner/walk_test.go index e9121a6b4..3d9999843 100644 --- a/lib/scanner/walk_test.go +++ b/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 {