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 28 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
44 changes: 44 additions & 0 deletions attrs.go
Expand Up @@ -56,6 +56,50 @@ type FileStat struct {
Extended []StatExtended
}

func (fs FileStat) MarshalTo(flags FileAttrFlags) []byte {
puellanivis marked this conversation as resolved.
Show resolved Hide resolved
// attributes variable struct, and also variable per protocol version
// spec version 3 attributes:
// uint32 flags
// uint64 size present only if flag SSH_FILEXFER_ATTR_SIZE
// uint32 uid present only if flag SSH_FILEXFER_ATTR_UIDGID
// uint32 gid present only if flag SSH_FILEXFER_ATTR_UIDGID
// uint32 permissions present only if flag SSH_FILEXFER_ATTR_PERMISSIONS
// uint32 atime present only if flag SSH_FILEXFER_ACMODTIME
// uint32 mtime present only if flag SSH_FILEXFER_ACMODTIME
// uint32 extended_count present only if flag SSH_FILEXFER_ATTR_EXTENDED
// string extended_type
// string extended_data
// ... more extended data (extended_type - extended_data pairs),
// so that number of pairs equals extended_count
var b []byte

if flags.Size {
b = marshalUint64(b, fs.Size)
}
if flags.UidGid {
b = marshalUint32(b, fs.UID)
b = marshalUint32(b, fs.GID)
}
if flags.Permissions {
b = marshalUint32(b, fs.Mode)
}
if flags.Acmodtime {
b = marshalUint32(b, fs.Atime)
b = marshalUint32(b, fs.Mtime)
}

if len(fs.Extended) > 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

And… 🤦‍♀️ we run into another bug of design.

The problem here is that we’re going to need to Marshal flags itself based on if ls(fs.Extended) is set or not. 😐

This split implementation is really going to drive me nuts, and might just drive me mad enough to go ahead and just roll out v2…

b = marshalUint32(b, uint32(len(fs.Extended)))

for _, attr := range fs.Extended {
b = marshalString(b, attr.ExtType)
b = marshalString(b, attr.ExtData)
}
}

return b
}

// StatExtended contains additional, extended information for a FileStat.
type StatExtended struct {
ExtType string
Expand Down
25 changes: 2 additions & 23 deletions packet.go
Expand Up @@ -54,31 +54,10 @@ func marshalFileInfo(b []byte, fi os.FileInfo) []byte {
// so that number of pairs equals extended_count

flags, fileStat := fileStatFromInfo(fi)
f := newFileAttrFlags(flags)

b = marshalUint32(b, flags)
Copy link
Collaborator

Choose a reason for hiding this comment

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

See? Think about what if flags & sshFileXferAttrExtended == sshFileXferAttrExtended and len(fileStat.Extended) == 0.

We’re going to encode a corrupted packet. 🤦‍♀️

Copy link
Author

Choose a reason for hiding this comment

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

Thankfully, that's not actually possible, but why it's not possible is subtle:

    // if fi implements FileInfoExtendedData, retrieve extended data from it
    if fiExt, ok := fi.(FileInfoExtendedData); ok {
        fileStat.Extended = fiExt.Extended()
        if len(fileStat.Extended) > 0 {
            flags |= sshFileXferAttrExtended
        }
    }

That's from fileStatFromInfo.

So it's not possible to have sshFileXferAttrExtended set while len(fileStat.Extended) == 0.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, for marshalFileInfo this might be true, but since FileStat.MarshalTo would be exported, more people can call it than just this code here, and the RequestServer code as well.

if flags&sshFileXferAttrSize != 0 {
b = marshalUint64(b, fileStat.Size)
}
if flags&sshFileXferAttrUIDGID != 0 {
b = marshalUint32(b, fileStat.UID)
b = marshalUint32(b, fileStat.GID)
}
if flags&sshFileXferAttrPermissions != 0 {
b = marshalUint32(b, fileStat.Mode)
}
if flags&sshFileXferAttrACmodTime != 0 {
b = marshalUint32(b, fileStat.Atime)
b = marshalUint32(b, fileStat.Mtime)
}

if flags&sshFileXferAttrExtended != 0 {
b = marshalUint32(b, uint32(len(fileStat.Extended)))

for _, attr := range fileStat.Extended {
b = marshalString(b, attr.ExtType)
b = marshalString(b, attr.ExtData)
}
}
b = append(b, fileStat.MarshalTo(f)...)

return b
}
Expand Down
40 changes: 40 additions & 0 deletions request-attrs.go
Expand Up @@ -22,6 +22,29 @@ func newFileOpenFlags(flags uint32) FileOpenFlags {
}
}

func (fof FileOpenFlags) ForRequest() (flags uint32) {
if fof.Read {
flags |= sshFxfRead
}
if fof.Write {
flags |= sshFxfWrite
}
if fof.Append {
flags |= sshFxfAppend
}
if fof.Creat {
flags |= sshFxfCreat
}
if fof.Trunc {
flags |= sshFxfTrunc
}
if fof.Excl {
flags |= sshFxfExcl
}

return flags
}

// Pflags converts the bitmap/uint32 from SFTP Open packet pflag values,
// into a FileOpenFlags struct with booleans set for flags set in bitmap.
func (r *Request) Pflags() FileOpenFlags {
Expand All @@ -35,6 +58,23 @@ type FileAttrFlags struct {
Size, UidGid, Permissions, Acmodtime bool
}

func (faf FileAttrFlags) ForRequest() (flags uint32) {
if faf.Size {
flags |= sshFileXferAttrSize
}
if faf.UidGid {
flags |= sshFileXferAttrUIDGID
}
if faf.Permissions {
flags |= sshFileXferAttrPermissions
}
if faf.Acmodtime {
flags |= sshFileXferAttrACmodTime
}

return flags
}

func newFileAttrFlags(flags uint32) FileAttrFlags {
return FileAttrFlags{
Size: (flags & sshFileXferAttrSize) != 0,
Expand Down
61 changes: 51 additions & 10 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 owner.
return fs.openFileModeCheck(r, 0o400)
}

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 owner.
return fs.openFileModeCheck(r, 0o200)
}

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

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 Down Expand Up @@ -108,8 +124,10 @@ func (fs *root) openfile(pathname string, flags uint32) (*memFile, error) {
link, err = fs.lfetch(pathname)
}

// The mode is currently hard coded because this library doesn't parse out the mode at file open time.
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 +169,30 @@ func (fs *root) Filecmd(r *Request) error {

switch r.Method {
case "Setstat":
// Some notes:
//
// openfile will follow symlinks, however as best as I can tell this is the correct POSIX behavior for chmod.
//
// openfile does not currently support opening a directory, and at this time we do not implement directory permissions.
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 {
return err
}
}
if flags.Permissions {
file.chmod(attrs.Mode)
}
// 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 +242,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 +540,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 +577,7 @@ type memFile struct {
modtime time.Time
symlink string
isdir bool
mode uint32

mu sync.RWMutex
content []byte
Expand All @@ -563,13 +597,15 @@ func (f *memFile) Size() int64 {
return f.size()
}
func (f *memFile) Mode() os.FileMode {
// At this time, we do not implement directory modes.
if f.isdir {
return os.FileMode(0755) | os.ModeDir
}
// Under POSIX, symlinks have a fixed mode which can not be changed.
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 Expand Up @@ -645,3 +681,8 @@ func (f *memFile) TransferError(err error) {

f.err = err
}

func (f *memFile) chmod(mode uint32) {
const mask = uint32(os.ModePerm | s_ISUID | s_ISGID | s_ISVTX)
f.mode = (f.mode &^ mask) | (mode & mask)
}