Repository: pkg/sftp
Pull request #416 (2021-03-12 12:47:26)

Clean up matching/globbing code
scanChunk and matchChunk were unused leftovers from the old Match implementation, removed in 4f3e725e885c021085d2fb8a9cc26e30ea1a992f.

isPathSeparator contained a useless cast to string.

The documentation (copied from path/filepath) referred to a Separator constant, which is not present in the package.

greatroar: I just removed getEsc as well. It was only called by matchChunk. And I caught a bug in Glob, for which I added a fix and a test.

Patch:
diff --git a/client.go b/client.go
index e4e9886..81b2d6c 100644
--- a/client.go
+++ b/client.go
@@ -273,10 +273,7 @@ func (c *Client) recvVersion() error {
 		return &unexpectedPacketErr{sshFxpVersion, typ}
 	}
 
-	version, data, err := unmarshalUint32Safe(data)
-	if err != nil {
-		return err
-	}
+	version, data := unmarshalUint32(data)
 	if version != sftpProtocolVersion {
 		return &unexpectedVersionErr{sftpProtocolVersion, version}
 	}
@@ -563,12 +560,8 @@ func (c *Client) Chown(path string, uid, gid int) error {
 }
 
 // Chmod changes the permissions of the named file.
-//
-// Chmod does not apply a umask, because even retrieving the umask is not
-// possible in a portable way without causing a race condition. Callers
-// should mask off umask bits, if desired.
 func (c *Client) Chmod(path string, mode os.FileMode) error {
-	return c.setstat(path, sshFileXferAttrPermissions, toChmodPerm(mode))
+	return c.setstat(path, sshFileXferAttrPermissions, uint32(mode))
 }
 
 // Truncate sets the size of the named file. Although it may be safely assumed
@@ -871,12 +864,12 @@ func (c *Client) MkdirAll(path string) error {
 
 	// Slow path: make sure parent exists and then call Mkdir for path.
 	i := len(path)
-	for i > 0 && path[i-1] == '/' { // Skip trailing path separator.
+	for i > 0 && os.IsPathSeparator(path[i-1]) { // Skip trailing path separator.
 		i--
 	}
 
 	j := i
-	for j > 0 && path[j-1] != '/' { // Scan backward over element.
+	for j > 0 && !os.IsPathSeparator(path[j-1]) { // Scan backward over element.
 		j--
 	}
 
@@ -1239,7 +1232,11 @@ func (f *File) WriteTo(w io.Writer) (written int64, err error) {
 		}
 	}()
 
-	pool := newBufPool(concurrency, f.c.maxPacket)
+	pool := sync.Pool{
+		New: func() interface{} {
+			return make([]byte, f.c.maxPacket)
+		},
+	}
 
 	wg.Add(concurrency)
 	for i := 0; i < concurrency; i++ {
@@ -1250,7 +1247,7 @@ func (f *File) WriteTo(w io.Writer) (written int64, err error) {
 			ch := make(chan result, 1) // reusable channel
 
 			for readWork := range readCh {
-				b := pool.Get()
+				b := pool.Get().([]byte)
 
 				n, err := f.readChunkAt(ch, b, readWork.off)
 				if n < 0 {
@@ -1519,13 +1516,12 @@ func (f *File) readFromConcurrent(r io.Reader, remain int64) (read int64, err er
 	}
 	errCh := make(chan rwErr)
 
-	concurrency := int(remain/int64(f.c.maxPacket) + 1) // a bad guess, but better than no guess
-	if concurrency > f.c.maxConcurrentRequests {
-		concurrency = f.c.maxConcurrentRequests
+	pool := sync.Pool{
+		New: func() interface{} {
+			return make([]byte, f.c.maxPacket)
+		},
 	}
 
-	pool := newBufPool(concurrency, f.c.maxPacket)
-
 	// Slice: cut up the Read into any number of buffers of length <= f.c.maxPacket, and at appropriate offsets.
 	go func() {
 		defer close(workCh)
@@ -1533,7 +1529,7 @@ func (f *File) readFromConcurrent(r io.Reader, remain int64) (read int64, err er
 		off := f.offset
 
 		for {
-			b := pool.Get()
+			b := pool.Get().([]byte)
 
 			n, err := r.Read(b)
 			if n > 0 {
@@ -1558,6 +1554,11 @@ func (f *File) readFromConcurrent(r io.Reader, remain int64) (read int64, err er
 		}
 	}()
 
+	concurrency := int(remain/int64(f.c.maxPacket) + 1) // a bad guess, but better than no guess
+	if concurrency > f.c.maxConcurrentRequests {
+		concurrency = f.c.maxConcurrentRequests
+	}
+
 	var wg sync.WaitGroup
 	wg.Add(concurrency)
 	for i := 0; i < concurrency; i++ {
@@ -1718,8 +1719,6 @@ func (f *File) Chown(uid, gid int) error {
 }
 
 // Chmod changes the permissions of the current file.
-//
-// See Client.Chmod for details.
 func (f *File) Chmod(mode os.FileMode) error {
 	return f.c.Chmod(f.path, mode)
 }
@@ -1831,25 +1830,3 @@ func flags(f int) uint32 {
 	}
 	return out
 }
-
-// toChmodPerm converts Go permission bits to POSIX permission bits.
-//
-// This differs from fromFileMode in that we preserve the POSIX versions of
-// setuid, setgid and sticky in m, because we've historically supported those
-// bits, and we mask off any non-permission bits.
-func toChmodPerm(m os.FileMode) (perm uint32) {
-	const mask = os.ModePerm | s_ISUID | s_ISGID | s_ISVTX
-	perm = uint32(m & mask)
-
-	if m&os.ModeSetuid != 0 {
-		perm |= s_ISUID
-	}
-	if m&os.ModeSetgid != 0 {
-		perm |= s_ISGID
-	}
-	if m&os.ModeSticky != 0 {
-		perm |= s_ISVTX
-	}
-
-	return perm
-}
diff --git a/match.go b/match.go
index e2f2ba4..875006a 100644
--- a/match.go
+++ b/match.go
@@ -3,198 +3,39 @@ package sftp
 import (
 	"path"
 	"strings"
-	"unicode/utf8"
 )
 
 // ErrBadPattern indicates a globbing pattern was malformed.
 var ErrBadPattern = path.ErrBadPattern
 
-// Unix separator
-const separator = "/"
-
-// Match reports whether name matches the shell file name pattern.
-// The pattern syntax is:
-//
-//	pattern:
-//		{ term }
-//	term:
-//		'*'         matches any sequence of non-Separator characters
-//		'?'         matches any single non-Separator character
-//		'[' [ '^' ] { character-range } ']'
-//		            character class (must be non-empty)
-//		c           matches character c (c != '*', '?', '\\', '[')
-//		'\\' c      matches character c
-//
-//	character-range:
-//		c           matches character c (c != '\\', '-', ']')
-//		'\\' c      matches character c
-//		lo '-' hi   matches character c for lo <= c <= hi
-//
-// Match requires pattern to match all of name, not just a substring.
-// The only possible returned error is ErrBadPattern, when pattern
-// is malformed.
-//
+// Match reports whether name matches the shell pattern.
 //
+// This is an alias for path.Match from the standard library,
+// offered so that callers need not import the path package.
+// For details, see https://golang.org/pkg/path/#Match.
 func Match(pattern, name string) (matched bool, err error) {
 	return path.Match(pattern, name)
 }
 
 // detect if byte(char) is path separator
 func isPathSeparator(c byte) bool {
-	return string(c) == "/"
-}
-
-// scanChunk gets the next segment of pattern, which is a non-star string
-// possibly preceded by a star.
-func scanChunk(pattern string) (star bool, chunk, rest string) {
-	for len(pattern) > 0 && pattern[0] == '*' {
-		pattern = pattern[1:]
-		star = true
-	}
-	inrange := false
-	var i int
-Scan:
-	for i = 0; i < len(pattern); i++ {
-		switch pattern[i] {
-		case '\\':
-
-			// error check handled in matchChunk: bad pattern.
-			if i+1 < len(pattern) {
-				i++
-			}
-		case '[':
-			inrange = true
-		case ']':
-			inrange = false
-		case '*':
-			if !inrange {
-				break Scan
-			}
-		}
-	}
-	return star, pattern[0:i], pattern[i:]
+	return c == '/'
 }
 
-// matchChunk checks whether chunk matches the beginning of s.
-// If so, it returns the remainder of s (after the match).
-// Chunk is all single-character operators: literals, char classes, and ?.
-func matchChunk(chunk, s string) (rest string, ok bool, err error) {
-	for len(chunk) > 0 {
-		if len(s) == 0 {
-			return
-		}
-		switch chunk[0] {
-		case '[':
-			// character class
-			r, n := utf8.DecodeRuneInString(s)
-			s = s[n:]
-			chunk = chunk[1:]
-			// We can't end right after '[', we're expecting at least
-			// a closing bracket and possibly a caret.
-			if len(chunk) == 0 {
-				err = ErrBadPattern
-				return
-			}
-			// possibly negated
-			negated := chunk[0] == '^'
-			if negated {
-				chunk = chunk[1:]
-			}
-			// parse all ranges
-			match := false
-			nrange := 0
-			for {
-				if len(chunk) > 0 && chunk[0] == ']' && nrange > 0 {
-					chunk = chunk[1:]
-					break
-				}
-				var lo, hi rune
-				if lo, chunk, err = getEsc(chunk); err != nil {
-					return
-				}
-				hi = lo
-				if chunk[0] == '-' {
-					if hi, chunk, err = getEsc(chunk[1:]); err != nil {
-						return
-					}
-				}
-				if lo <= r && r <= hi {
-					match = true
-				}
-				nrange++
-			}
-			if match == negated {
-				return
-			}
-
-		case '?':
-			if isPathSeparator(s[0]) {
-				return
-			}
-			_, n := utf8.DecodeRuneInString(s)
-			s = s[n:]
-			chunk = chunk[1:]
-
-		case '\\':
-			chunk = chunk[1:]
-			if len(chunk) == 0 {
-				err = ErrBadPattern
-				return
-			}
-			fallthrough
-
-		default:
-			if chunk[0] != s[0] {
-				return
-			}
-			s = s[1:]
-			chunk = chunk[1:]
-		}
-	}
-	return s, true, nil
-}
-
-// getEsc gets a possibly-escaped character from chunk, for a character class.
-func getEsc(chunk string) (r rune, nchunk string, err error) {
-	if len(chunk) == 0 || chunk[0] == '-' || chunk[0] == ']' {
-		err = ErrBadPattern
-		return
-	}
-	if chunk[0] == '\\' {
-		chunk = chunk[1:]
-		if len(chunk) == 0 {
-			err = ErrBadPattern
-			return
-		}
-	}
-	r, n := utf8.DecodeRuneInString(chunk)
-	if r == utf8.RuneError && n == 1 {
-		err = ErrBadPattern
-	}
-	nchunk = chunk[n:]
-	if len(nchunk) == 0 {
-		err = ErrBadPattern
-	}
-	return
-}
-
-// Split splits path immediately following the final Separator,
+// Split splits the path p immediately following the final slash,
 // separating it into a directory and file name component.
-// If there is no Separator in path, Split returns an empty dir
-// and file set to path.
-// The returned values have the property that path = dir+file.
-func Split(path string) (dir, file string) {
-	i := len(path) - 1
-	for i >= 0 && !isPathSeparator(path[i]) {
-		i--
-	}
-	return path[:i+1], path[i+1:]
+//
+// This is an alias for path.Split from the standard library,
+// offered so that callers need not import the path package.
+// For details, see https://golang.org/pkg/path/#Split.
+func Split(p string) (dir, file string) {
+	return path.Split(p)
 }
 
 // Glob returns the names of all files matching pattern or nil
 // if there is no matching file. The syntax of patterns is the same
 // as in Match. The pattern may describe hierarchical names such as
-// /usr/*/bin/ed (assuming the Separator is '/').
+// /usr/*/bin/ed.
 //
 // Glob ignores file system errors such as I/O errors reading directories.
 // The only possible returned error is ErrBadPattern, when pattern
@@ -241,8 +82,7 @@ func cleanGlobPath(path string) string {
 	switch path {
 	case "":
 		return "."
-	case string(separator):
-		// do nothing to the path
+	case "/":
 		return path
 	default:
 		return path[0 : len(path)-1] // chop off trailing separator
@@ -280,9 +120,12 @@ func (c *Client) glob(dir, pattern string, matches []string) (m []string, e erro
 	return
 }
 
-// Join joins any number of path elements into a single path, adding
-// a Separator if necessary.
-// all empty strings are ignored.
+// Join joins any number of path elements into a single path, separating
+// them with slashes.
+//
+// This is an alias for path.Join from the standard library,
+// offered so that callers need not import the path package.
+// For details, see https://golang.org/pkg/path/#Join.
 func Join(elem ...string) string {
 	return path.Join(elem...)
 }
@@ -290,6 +133,5 @@ func Join(elem ...string) string {
 // hasMeta reports whether path contains any of the magic characters
 // recognized by Match.
 func hasMeta(path string) bool {
-	// TODO(niemeyer): Should other magic characters be added here?
-	return strings.ContainsAny(path, "*?[")
+	return strings.ContainsAny(path, "\\*?[")
 }
diff --git a/pool.go b/pool.go
deleted file mode 100644
index c074637..0000000
--- a/pool.go
+++ /dev/null
@@ -1,29 +0,0 @@
-package sftp
-
-type bufPool struct {
-	ch   chan []byte
-	blen int
-}
-
-func newBufPool(depth, bufLen int) *bufPool {
-	return &bufPool{
-		ch:   make(chan []byte, depth),
-		blen: bufLen,
-	}
-}
-
-func (p *bufPool) Get() []byte {
-	select {
-	case b := <-p.ch:
-		return b
-	default:
-		return make([]byte, p.blen)
-	}
-}
-
-func (p *bufPool) Put(b []byte) {
-	select {
-	case p.ch <- b:
-	default:
-	}
-}
diff --git a/stat_plan9.go b/stat_plan9.go
index 25074fe..32f17e0 100644
--- a/stat_plan9.go
+++ b/stat_plan9.go
@@ -94,11 +94,3 @@ func fromFileMode(mode os.FileMode) uint32 {
 
 	return ret
 }
-
-// Plan 9 doesn't have setuid, setgid or sticky, but a Plan 9 client should
-// be able to send these bits to a POSIX server.
-const (
-	s_ISUID = 04000
-	s_ISGID = 02000
-	S_ISVTX = 01000
-)
diff --git a/stat_posix.go b/stat_posix.go
index 71080ac..38ba26a 100644
--- a/stat_posix.go
+++ b/stat_posix.go
@@ -114,9 +114,3 @@ func fromFileMode(mode os.FileMode) uint32 {
 
 	return ret
 }
-
-const (
-	s_ISUID = syscall.S_ISUID
-	s_ISGID = syscall.S_ISGID
-	s_ISVTX = syscall.S_ISVTX
-)


Test patch:
diff --git a/client_integration_test.go b/client_integration_test.go
index cca1078..d916a2a 100644
--- a/client_integration_test.go
+++ b/client_integration_test.go
@@ -873,59 +873,6 @@ func TestClientChmodReadonly(t *testing.T) {
 	}
 }
 
-func TestClientSetuid(t *testing.T) {
-	skipIfWindows(t) // No UNIX permissions.
-	if *testServerImpl {
-		t.Skipf("skipping with -testserver")
-	}
-
-	sftp, cmd := testClient(t, READWRITE, NODELAY)
-	defer cmd.Wait()
-	defer sftp.Close()
-
-	f, err := ioutil.TempFile("", "sftptest-setuid")
-	if err != nil {
-		t.Fatal(err)
-	}
-	defer os.Remove(f.Name())
-	f.Close()
-
-	const allPerm = os.ModePerm | os.ModeSetuid | os.ModeSetgid | os.ModeSticky |
-		s_ISUID | s_ISGID | s_ISVTX
-
-	for _, c := range []struct {
-		goPerm    os.FileMode
-		posixPerm uint32
-	}{
-		{os.ModeSetuid, s_ISUID},
-		{os.ModeSetgid, s_ISGID},
-		{os.ModeSticky, s_ISVTX},
-		{os.ModeSetuid | os.ModeSticky, s_ISUID | s_ISVTX},
-	} {
-		goPerm := 0700 | c.goPerm
-		posixPerm := 0700 | c.posixPerm
-
-		err = sftp.Chmod(f.Name(), goPerm)
-		require.NoError(t, err)
-
-		info, err := sftp.Stat(f.Name())
-		require.NoError(t, err)
-		require.Equal(t, goPerm, info.Mode()&allPerm)
-
-		err = sftp.Chmod(f.Name(), 0700) // Reset funny bits.
-		require.NoError(t, err)
-
-		// For historical reasons, we also support literal POSIX mode bits in
-		// Chmod. Stat should still translate these to Go os.FileMode bits.
-		err = sftp.Chmod(f.Name(), os.FileMode(posixPerm))
-		require.NoError(t, err)
-
-		info, err = sftp.Stat(f.Name())
-		require.NoError(t, err)
-		require.Equal(t, goPerm, info.Mode()&allPerm)
-	}
-}
-
 func TestClientChown(t *testing.T) {
 	skipIfWindows(t) // No UNIX permissions.
 	sftp, cmd := testClient(t, READWRITE, NODELAY)
@@ -1935,6 +1882,7 @@ var globTests = []struct {
 	{"match.go", "match.go"},
 	{"mat?h.go", "match.go"},
 	{"ma*ch.go", "match.go"},
+	{`\m\a\t\c\h\.\g\o`, "match.go"},
 	{"../*/match.go", "../sftp/match.go"},
 }
 
diff --git a/client_test.go b/client_test.go
index 05b885c..b2eacbc 100644
--- a/client_test.go
+++ b/client_test.go
@@ -228,14 +228,3 @@ func TestClientZeroLengthPacket(t *testing.T) {
 		c.Close()
 	}
 }
-
-func TestClientShortPacket(t *testing.T) {
-	// init packet too short.
-	packet := []byte{0, 0, 0, 1, 2}
-
-	r := bytes.NewReader(packet)
-	_, err := NewClientPipe(r, &sink{})
-	if !errors.Is(err, errShortPacket) {
-		t.Fatalf("expected error: %v, got: %v", errShortPacket, err)
-	}
-}