pkg / sftp

SFTP support for the go.crypto/ssh package
BSD 2-Clause "Simplified" License
1.52k stars 380 forks source link

request-server: implement Open method #377

Closed drakkan closed 4 years ago

drakkan commented 4 years ago

add an optional interface to FileWriter to allow to open a file for both writing and reading

Fixes #374

I noticed that an Open method is documented but not implemented, see here. I implemented this method. The change is backward compatible and implementing the new optional Filewriter interface the request server can handle both reading and writing to the same file fixing the corruption issue reported in #374

I know that we want something different for v2 and so I'll not push this change myself.

However I cannot wait until we implement v2 to fix a file corruption issue in SFTPGo, so if we don't agree to merge this change I'll maintain it in my fork until we have an alternative, thank you

drakkan commented 4 years ago

Hi,

after changing the code like this

func (fs *root) getFileForWrite(r *Request) (*memFile, error) {
    if fs.mockErr != nil {
        return nil, fs.mockErr
    }
    _ = r.WithContext(r.Context()) // initialize context for deadlock testing
    fs.filesLock.Lock()
    defer fs.filesLock.Unlock()
    file, err := fs.fetch(r.Filepath)
    if err == os.ErrNotExist {
        dir, err := fs.fetch(path.Dir(r.Filepath))
        if err != nil {
            return nil, err
        }
        if !dir.isdir {
            return nil, os.ErrInvalid
        }
        file = newMemFile(r.Filepath, false)
        fs.files[r.Filepath] = file
    }
    return file, nil
}

func (fs *root) Filewrite(r *Request) (io.WriterAt, error) {
    file, err := fs.getFileForWrite(r)
    if err != nil {
        return nil, err
    }
    return file.WriterAt()
}

func (fs *root) OpenFile(r *Request) (WriterAtReaderAt, error) {
    return fs.getFileForWrite(r)
}

I'm getting test failures like this one:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x58 pc=0x680b45]

goroutine 229 [running]:
github.com/pkg/sftp.(*memFile).TransferError(0x0, 0x7b8da0, 0xc000410140)
    /home/nicola/goprojects/sftp/request-example.go:345 +0x25
github.com/pkg/sftp.(*Request).transferError(0xc000720750, 0x7b8da0, 0xc000410140)
    /home/nicola/goprojects/sftp/request.go:195 +0x185
github.com/pkg/sftp.(*RequestServer).Serve(0xc000616a00, 0x0, 0x0)
    /home/nicola/goprojects/sftp/request-server.go:171 +0x259
github.com/pkg/sftp.clientRequestServerPair.func1(0xc000020ea0, 0xc000313380, 0xc000318020)
    /home/nicola/goprojects/sftp/request-server_test.go:55 +0x245
created by github.com/pkg/sftp.clientRequestServerPair
    /home/nicola/goprojects/sftp/request-server_test.go:40 +0xe5
exit status 2

for some reason now in code like this

if t, ok := wr.(TransferError); ok {
    t.TransferError(err)
}

wr implements the interface but is nil (probably because is a pointer to a struct, the struct implements the interface but it is nil). I got the same error in SFTPGo in the past when returning a pointer to a struct. What do you think to add something like this

if t, ok := wr.(TransferError); ok && t != nil && !reflect.ValueOf(t).IsNil() {
    t.TransferError(err)
}

to avoid this issue? Thank you

puellanivis commented 4 years ago

wr implements the interface but is nil (probably because is a pointer to a struct, the struct implements the interface but it is nil).

Yeah, this is a source of a lot of issues, where a (*ConcreteType)(nil) implements a method, but does not expect a nil-pointer.

drakkan commented 4 years ago

wr implements the interface but is nil (probably because is a pointer to a struct, the struct implements the interface but it is nil).

Yeah, this is a source of a lot of issues, where a (*ConcreteType)(nil) implements a method, but does not expect a nil-pointer.

OK, so is this PR ready to merge for you? Or should we document that the implementer should expect a null pointer?

Thank you

puellanivis commented 4 years ago

More preferably, the Request.open(…) handler should handle err != nil at each call site, and not assign any values into r.state if there is an error.

It shouldn’t be hard to copy-paste the if err != nil { return statusFromError(pkt, err) } into each of the funcitons, and implement them properly, rather than cheating with a “fallthrough” logic pattern…

drakkan commented 4 years ago

Great! I wish you could review all my code!

puellanivis commented 4 years ago

Great! I wish you could review all my code!

You say that. And then a month in, you’re just like, “but it works, right?” 😰

drakkan commented 4 years ago

Great! I wish you could review all my code!

You say that. And then a month in, you’re just like, “but it works, right?”

I don't think so :)

puellanivis commented 4 years ago

All good!