Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Memfile stuff #561

Open
wants to merge 30 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
b9d7f36
Use errors.Is
Oct 24, 2023
8513fed
Start tracking the file mode.
Oct 31, 2023
07a6b6a
Move memfile's Setstat function, add features.
Oct 31, 2023
7bf7a25
Make a new interface for direct SetStat access.
Oct 31, 2023
6b2b978
Add SetAttributes as an alternate option.
Oct 31, 2023
688c055
Rip out SetAttributes.
Nov 1, 2023
a0a4a52
Get rid of FileStatSetter.
Nov 1, 2023
8ee6563
Use errors.Is everywhere.
Nov 1, 2023
f215740
Merge SetStat back into Filecmd.
Nov 1, 2023
d41c7d4
Export ToFileMode and FromFileMode.
Nov 1, 2023
87fa218
Silence a linter warning.
Nov 1, 2023
a5d4316
memfile: Check file permissions.
Nov 1, 2023
4404cc8
Add some comments.
Nov 1, 2023
656a9db
Explictly forbid changing the mode of a symlink.
Nov 1, 2023
b1d53f7
Expose To/FromFileMode for plan9.
Nov 1, 2023
cbf7fcd
Add FileStat.MarshalTo and FAF.ForRequest.
Nov 2, 2023
4b73c93
Don't put the flags in the attrs.
Nov 2, 2023
82cd5ca
FileOpenFlags.ForRequest
Nov 2, 2023
5fd4ff4
Revert "Expose To/FromFileMode for plan9."
Nov 2, 2023
660c973
Revert "Export ToFileMode and FromFileMode."
Nov 2, 2023
6c452cc
Revert "Use errors.Is everywhere."
Nov 2, 2023
6563249
Revert "Silence a linter warning."
Nov 2, 2023
ad0d7df
Comment fixes.
Nov 2, 2023
7e1a518
Let Setstat follow symlinks, and rework comments.
Nov 2, 2023
d3a55c7
Shadow err to avoid the value leaking.
Nov 2, 2023
bff300b
Move the chmod logic to it's own method.
Nov 2, 2023
1d040d6
Make marshalFileInfo use fileStat.MarshalTo
Nov 2, 2023
706f4f1
Use 3 digit modes, not 4.
Nov 2, 2023
1ae2738
MarshalTo now takes a []byte argument.
Nov 3, 2023
3d7324a
Add warning comments around the Extended logic.
Nov 3, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions attrs.go
Expand Up @@ -32,7 +32,7 @@ func (fi *fileInfo) Name() string { return fi.name }
func (fi *fileInfo) Size() int64 { return int64(fi.stat.Size) }

// Mode returns file mode bits.
func (fi *fileInfo) Mode() os.FileMode { return toFileMode(fi.stat.Mode) }
func (fi *fileInfo) Mode() os.FileMode { return ToFileMode(fi.stat.Mode) }

// ModTime returns the last modification time of the file.
func (fi *fileInfo) ModTime() time.Time { return time.Unix(int64(fi.stat.Mtime), 0) }
Expand Down Expand Up @@ -92,7 +92,7 @@ func fileStatFromInfo(fi os.FileInfo) (uint32, *FileStat) {

fileStat := &FileStat{
Size: uint64(fi.Size()),
Mode: fromFileMode(fi.Mode()),
Mode: FromFileMode(fi.Mode()),
Mtime: uint32(mtime),
Atime: uint32(atime),
}
Expand Down
14 changes: 7 additions & 7 deletions client.go
Expand Up @@ -275,7 +275,7 @@ func (c *Client) nextID() uint32 {
func (c *Client) recvVersion() error {
typ, data, err := c.recvPacket(0)
if err != nil {
if err == io.EOF {
if errors.Is(err, io.EOF) {
zelch marked this conversation as resolved.
Show resolved Hide resolved
return fmt.Errorf("server unexpectedly closed connection: %w", io.ErrUnexpectedEOF)
}

Expand Down Expand Up @@ -368,7 +368,7 @@ func (c *Client) ReadDir(p string) ([]os.FileInfo, error) {
return nil, unimplementedPacketErr(typ)
}
}
if err == io.EOF {
if errors.Is(err, io.EOF) {
err = nil
}
return attrs, err
Expand Down Expand Up @@ -1238,7 +1238,7 @@ func (f *File) writeToSequential(w io.Writer) (written int64, err error) {
}

if err != nil {
if err == io.EOF {
if errors.Is(err, io.EOF) {
return written, nil // return nil explicitly.
}

Expand Down Expand Up @@ -1435,7 +1435,7 @@ func (f *File) WriteTo(w io.Writer) (written int64, err error) {
}

if packet.err != nil {
if packet.err == io.EOF {
if errors.Is(packet.err, io.EOF) {
return written, nil
}

Expand Down Expand Up @@ -1726,7 +1726,7 @@ func (f *File) ReadFromWithConcurrency(r io.Reader, concurrency int) (read int64
}

if err != nil {
if err != io.EOF {
if !errors.Is(err, io.EOF) {
errCh <- rwErr{off, err}
}
return
Expand Down Expand Up @@ -1878,7 +1878,7 @@ func (f *File) ReadFrom(r io.Reader) (int64, error) {
}

if err != nil {
if err == io.EOF {
if errors.Is(err, io.EOF) {
return read, nil // return nil explicitly.
}

Expand Down Expand Up @@ -2009,7 +2009,7 @@ func flags(f int) uint32 {

// toChmodPerm converts Go permission bits to POSIX permission bits.
//
// This differs from fromFileMode in that we preserve the POSIX versions of
// 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) {
Expand Down
6 changes: 3 additions & 3 deletions client_integration_test.go
Expand Up @@ -1240,7 +1240,7 @@ func TestClientReadSimple(t *testing.T) {
defer f2.Close()
stuff := make([]byte, 32)
n, err := f2.Read(stuff)
if err != nil && err != io.EOF {
if err != nil && !errors.Is(err, io.EOF) {
t.Fatalf("err: %v", err)
}
if n != 5 {
Expand Down Expand Up @@ -2152,7 +2152,7 @@ func TestMatch(t *testing.T) {
pattern := tt.pattern
s := tt.s
ok, err := Match(pattern, s)
if ok != tt.match || err != tt.err {
if ok != tt.match || !errors.Is(err, tt.err) {
t.Errorf("Match(%#q, %#q) = %v, %q want %v, %q", pattern, s, ok, errp(err), tt.match, errp(tt.err))
}
}
Expand Down Expand Up @@ -2411,7 +2411,7 @@ func benchmarkRead(b *testing.B, bufsize int, delay time.Duration) {
for offset < size {
n, err := io.ReadFull(f2, buf)
offset += n
if err == io.ErrUnexpectedEOF && offset != size {
if errors.Is(err, io.ErrUnexpectedEOF) && offset != size {
b.Fatalf("read too few bytes! want: %d, got: %d", size, n)
}

Expand Down
3 changes: 2 additions & 1 deletion examples/go-sftp-server/main.go
Expand Up @@ -4,6 +4,7 @@
package main

import (
"errors"
"flag"
"fmt"
"io"
Expand Down Expand Up @@ -136,7 +137,7 @@ func main() {
if err != nil {
log.Fatal(err)
}
if err := server.Serve(); err == io.EOF {
if err := server.Serve(); errors.Is(err, io.EOF) {
server.Close()
log.Print("sftp client exited session.")
} else if err != nil {
Expand Down
3 changes: 2 additions & 1 deletion examples/request-server/main.go
Expand Up @@ -4,6 +4,7 @@
package main

import (
"errors"
"flag"
"fmt"
"io"
Expand Down Expand Up @@ -120,7 +121,7 @@ func main() {

root := sftp.InMemHandler()
server := sftp.NewRequestServer(channel, root)
if err := server.Serve(); err == io.EOF {
if err := server.Serve(); errors.Is(err, io.EOF) {
server.Close()
log.Print("sftp client exited session.")
} else if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion ls_formatting.go
Expand Up @@ -47,7 +47,7 @@ func runLs(idLookup NameLookupFileLister, dirent os.FileInfo) string {
// format:
// {directory / char device / etc}{rwxrwxrwx} {number of links} owner group size month day [time (this year) | year (otherwise)] name

symPerms := sshfx.FileMode(fromFileMode(dirent.Mode())).String()
symPerms := sshfx.FileMode(FromFileMode(dirent.Mode())).String()

var numLinks uint64 = 1
uid, gid := "0", "0"
Expand Down
2 changes: 1 addition & 1 deletion packet.go
Expand Up @@ -292,7 +292,7 @@ func recvPacket(r io.Reader, alloc *allocator, orderID uint32) (uint8, []byte, e
if _, err := io.ReadFull(r, b[:length]); err != nil {
// ReadFull only returns EOF if it has read no bytes.
// In this case, that means a partial packet, and thus unexpected.
if err == io.EOF {
if errors.Is(err, io.EOF) {
err = io.ErrUnexpectedEOF
}
debug("recv packet %d bytes: err %v", length, err)
Expand Down
67 changes: 56 additions & 11 deletions request-example.go
Expand Up @@ -37,7 +37,8 @@ func (fs *root) Fileread(r *Request) (io.ReaderAt, error) {
return nil, os.ErrInvalid
}

return fs.OpenFile(r)
// Needs to be readable by the user.
puellanivis marked this conversation as resolved.
Show resolved Hide resolved
return fs.openFileModeCheck(r, 0o0400)
zelch marked this conversation as resolved.
Show resolved Hide resolved
}

func (fs *root) Filewrite(r *Request) (io.WriterAt, error) {
Expand All @@ -47,10 +48,16 @@ func (fs *root) Filewrite(r *Request) (io.WriterAt, error) {
return nil, os.ErrInvalid
}

return fs.OpenFile(r)
// Needs to be writable by the user.
return fs.openFileModeCheck(r, 0o0200)
}

func (fs *root) OpenFile(r *Request) (WriterAtReaderAt, error) {
// Needs to be readable and writable by the user.
return fs.openFileModeCheck(r, 0o0200|0o0400)
}

func (fs *root) openFileModeCheck(r *Request, mode uint32) (WriterAtReaderAt, error) {
if fs.mockErr != nil {
return nil, fs.mockErr
}
Expand All @@ -59,7 +66,16 @@ func (fs *root) OpenFile(r *Request) (WriterAtReaderAt, error) {
fs.mu.Lock()
defer fs.mu.Unlock()

return fs.openfile(r.Filepath, r.Flags)
f, err := fs.openfile(r.Filepath, r.Flags)
if err != nil {
return nil, err
}

if f.mode&mode != mode {
return nil, os.ErrPermission
}

return f, nil
}

func (fs *root) putfile(pathname string, file *memFile) error {
Expand All @@ -72,7 +88,7 @@ func (fs *root) putfile(pathname string, file *memFile) error {
return os.ErrInvalid
}

if _, err := fs.lfetch(pathname); err != os.ErrNotExist {
if _, err := fs.lfetch(pathname); !errors.Is(err, os.ErrNotExist) {
return os.ErrExist
}

Expand All @@ -86,7 +102,7 @@ func (fs *root) openfile(pathname string, flags uint32) (*memFile, error) {
pflags := newFileOpenFlags(flags)

file, err := fs.fetch(pathname)
if err == os.ErrNotExist {
if errors.Is(err, os.ErrNotExist) {
if !pflags.Creat {
return nil, os.ErrNotExist
}
Expand All @@ -108,8 +124,11 @@ func (fs *root) openfile(pathname string, flags uint32) (*memFile, error) {
link, err = fs.lfetch(pathname)
}

// The mode is hard coded because the sftp protocol does not specify a
// mode at file open time.
zelch marked this conversation as resolved.
Show resolved Hide resolved
file := &memFile{
modtime: time.Now(),
mode: 0644,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not be defaulting this value. Please, pass it as a parameter, and document in the godoc that it is ignored if the creation flag is not set.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, I had defaulted to the value that we previously hard coded. I'll take a crack at getting it from the initial request.

}

if err := fs.putfile(pathname, file); err != nil {
Expand Down Expand Up @@ -151,15 +170,38 @@ func (fs *root) Filecmd(r *Request) error {

switch r.Method {
case "Setstat":
// We explicitly do not support changing the mode of symlinks.
// Nor do we want to change the mode of whatever the symlink is pointed
// at.
zelch marked this conversation as resolved.
Show resolved Hide resolved
link, err := fs.Readlink(r.Filepath)
if link != "" || err == nil || !errors.Is(err, os.ErrInvalid) {
return err
}

// Note: fs.openfile does not support opening a directory.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Memfile is intended to be a stripped-feature POSIX compatible filesystem.

It turns out, this behavior is a bug. It just wasn’t apparent, because I wasn’t implementing permissions on the filesystem, and we do not stress test this package for POSIX compatibility.

Any feature expansion to support permission should comply with POSIX compatibility of permissions, and this includes directories having permissions, and the behavior of those permissions.

When files don’t have permissions, it does not violate the principle of least astonishment that directories do not have permissions as well. But if files have permissions, it does violate the principle of least astonishment that directories do not have permissions.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is awkward, and I think that we might be better off documenting where we are not POSIX compliant.

Otherwise we're left checking the permissions of every directory under a given file. That wouldn't necessarily be the end of the world, but it definitely makes all the file open cases more complicated.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fortunately, opening both files and directories goes through either fetch or lfetch.

And yeah. :( POSIX is really hard, but our testing code kind of relies upon it being as POSIX-compliant that we can get. Last I touched this, I basically had to do a complete overhaul to correctly support open flags properly. … 🤔 I think I might have avoided working on permissions and ownership precisely because things were getting out-of-hand, and it would require an even further restructure/refactor to more properly emulate inodes in their entirety.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fortunately, this is all still passing the testing infrastructure, so I'm still very inclined to just keep permissions on directories as 'unimplemented' for right this moment.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s passing the testing infrastructure merely because we’re not sufficiently testing it. :blobsweat:

I think if we’re going to start implementing permissions for files, but not for directories, then we’re going to need to start documenting what is covered and what is not. Right now attempting to change permissions on anything won’t do anything, and users are likely to say, “oh… I guess no permissions?” (Somewhat typical situation because of Windows.) Or changing ownership yields, “oh, I guess no ownership.”

But “wait, but file permissions work, and dir permissions don’t?!” is going to lead to questions that we can answer with documentation. If you’re not willing to put in the effort of :blobsweat: implementing the full permissions system. (I don’t blame you. I clearly stopped short.)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with implementing directory permissions is that I can't even remember the esoteric rules on what happens when one of read or execute is set without the other set, let alone the special bits. Especially since we don't even have a chdir at that level, just get of specific full paths. And I definitely recall that it gets weird when you have that be the case for an intermediate directory in the path, but you have full permissions for a subdirectory.

Only, now you've made me look at the POSIX specification with a migraine, ugh.

Writing test cases for this is going to be painful, but I'll take a look in my free time. Sadly, having gotten stuff working 'well enough' for the work application, it's harder to spend work time on the remaining bits.

// So there is currently no way to set the mode of a directory.
// That's a good thing, because it means that we don't have to do
// permissions checks on parent directories.
flags := r.AttrFlags()
attrs := r.Attributes()
file, err := fs.openfile(r.Filepath, sshFxfWrite)
if err != nil {
return err
}

if r.AttrFlags().Size {
return file.Truncate(int64(r.Attributes().Size))
if flags.Size {
if err = file.Truncate(int64(attrs.Size)); err != nil {
zelch marked this conversation as resolved.
Show resolved Hide resolved
return err
}
}
if flags.Permissions {
const mask = uint32(os.ModePerm | s_ISUID | s_ISGID | s_ISVTX)
file.mode = (file.mode &^ mask) | (attrs.Mode & mask)
zelch marked this conversation as resolved.
Show resolved Hide resolved
}
// We only have mtime, not atime.
if flags.Acmodtime {
file.modtime = time.Unix(int64(attrs.Mtime), 0)
}

return nil

case "Rename":
Expand Down Expand Up @@ -209,7 +251,7 @@ func (fs *root) rename(oldpath, newpath string) error {
}

target, err := fs.lfetch(newpath)
if err != os.ErrNotExist {
if !errors.Is(err, os.ErrNotExist) {
if target == file {
// IEEE 1003.1: if oldpath and newpath are the same directory entry,
// then return no error, and perform no further action.
Expand Down Expand Up @@ -507,7 +549,7 @@ func (fs *root) exists(path string) bool {

_, err = fs.lfetch(path)

return err != os.ErrNotExist
return !errors.Is(err, os.ErrNotExist)
}

func (fs *root) fetch(pathname string) (*memFile, error) {
Expand Down Expand Up @@ -544,6 +586,7 @@ type memFile struct {
modtime time.Time
symlink string
isdir bool
mode uint32

mu sync.RWMutex
content []byte
Expand All @@ -563,13 +606,15 @@ func (f *memFile) Size() int64 {
return f.size()
}
func (f *memFile) Mode() os.FileMode {
// Hardcoded values, because we do not even try to support changing the
// mode of directories or symlinks.
if f.isdir {
return os.FileMode(0755) | os.ModeDir
}
if f.symlink != "" {
return os.FileMode(0777) | os.ModeSymlink
}
return os.FileMode(0644)
return os.FileMode(f.mode)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function also now needs to that directories might have non-default modes.

}
func (f *memFile) ModTime() time.Time { return f.modtime }
func (f *memFile) IsDir() bool { return f.isdir }
Expand Down
2 changes: 1 addition & 1 deletion request-server.go
Expand Up @@ -189,7 +189,7 @@ func (rs *RequestServer) Serve() error {
// make sure all open requests are properly closed
// (eg. possible on dropped connections, client crashes, etc.)
for handle, req := range rs.openRequests {
if err == io.EOF {
if errors.Is(err, io.EOF) {
err = io.ErrUnexpectedEOF
}
req.transferError(err)
Expand Down
3 changes: 2 additions & 1 deletion request-server_test.go
Expand Up @@ -2,6 +2,7 @@ package sftp

import (
"context"
"errors"
"fmt"
"io"
"io/ioutil"
Expand Down Expand Up @@ -240,7 +241,7 @@ func TestRequestJustRead(t *testing.T) {
defer rf.Close()
contents := make([]byte, 5)
n, err := rf.Read(contents)
if err != nil && err != io.EOF {
if err != nil && !errors.Is(err, io.EOF) {
t.Fatalf("err: %v", err)
}
assert.Equal(t, 5, n)
Expand Down