InMemHandler does not obey Truncate flag

[jimrobinson]

I've written a client that uses github.com/pkg/sftp and my tests were failing due to what I think is a bug in sftp.InMemHandler.

It looks to me as though sftp.InMemHandler does not truncate files when a client Create operation reuses an existing file.

I've attached an example as inmemsftp.tar.gz

The test client makes three distinct sftp.Client#OpenFile calls, specifying os.O_TRUNC:

fh, err := client.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC)

the intent being to overwrite the file and replace its contents with new ones.

In this example I use a python sftp server to show the client code does appear to do the correct thing when using it.

I haven't bundled the env files from the python test because I imagine that'd be considered a security concern, but the README.txt shows the steps I followed:

# we're using an sftp server impl in python to compare to inmem results
$ python -V
Python 3.7.8

# we setup an virtual environment for python
$ python -m venv env

# we activate the environment
$ source env/bin/activate

# within the environment we install sftpserver
(env) $ pip install --upgrade pip && pip install sftpserver
... output elided ...

# we start the sftp server
(env) $ sftpserver -k id_rsa  &
[1] 987395

# build the inmemsftp test client from main.go
(env) $ go build

# run the test client and we see expected output
# for our write operations
(env) $ ./inmemsftp
INFO:paramiko.transport:Connected (version 2.0, client Go)
INFO:paramiko.transport:Auth rejected (none).
INFO:paramiko.transport:Auth granted (password).
/test 000
/test 11
/test 2
ERROR:paramiko.transport:Socket exception: Connection reset by peer (104)

# kill the sftpserver
(env) $ kill %1

# now try again using the inmem sftp handlers and we
# we see that the truncate operation is not being
# satisfied
(env) $ ./inmemsftp -inmem
Listening on 127.0.0.1:3373
2020/09/16 18:34:44 login detected: user
/test 000
/test 110
/test 210
2020/09/16 18:34:44 sftp client exited session.

[drakkan]

Hi,

I have not tested your code, however sftp.InMemHandler seems to not respect any open file flag, it simply fetch the existing file. It should create a new in memory file if the TRUNCATE flag is requested. Do you want to provide a patch? Thank you

[puellanivis]

Yeah, was just going to add, “this sounds likely.” And then I got into checking. Yep. We just kind of ignore all the flags.

[puellanivis]

Hm… actually, I’m thinking I might want to do a rewrite/refactor. Getting it to respect all the appropriate flags that it should probably needs a fair amount of refactoring…

[jimrobinson]

[Apologies for the earlier close/reopen, I hit the close button by accident]

I don't see any good way to implement the SSH_FXF_APPEND handling w/o some heavy reworking.

For SSH_FXF_READ / SSH_FXF_WRITE / SSH_FXF_CREAT / SSH_FXF_TRUNC flags, would it be reasonable to just handle those at the level of the Fileread and Filewrite methods, something like the following?

https://github.com/pkg/sftp/compare/master...jimrobinson:ISSUE-383

[puellanivis]

There’s a bug in your proposed code. If you open with O_CREATE it will truncate the file, even if the file already exists. That should only happen when there is O_TRUNC.

This block here: https://github.com/pkg/sftp/compare/master...jimrobinson:ISSUE-383#diff-e17984159b645b7e2693b5a7b32a6dffR69-R72 is already covered by r.Pflags()

Could this issue be solved by a more minimally invasive change? Yes. But there are a lot of buggy behaviors beyond just “not supporting SSH_FX_TRUNC”, which is why I noted that I was wanting to refactor the file.

[jimrobinson]

Ok, thank you for looking into the wider refactoring.

[puellanivis]

Resolved in https://github.com/pkg/sftp/pull/384

Still left unimplemented is the Append flag, but as noted in the other PR, getting this working would require a pretty radical rewrite, which I have played with a bit already, but it has become clear working on it, that it would require a pretty drastic departure away from the scope of this project into providing an actual POSIX compliant in-memory file system. I might start a personal package to do so, which would then also be compatible with this project. I actually think supporting symlinks in the example package is precisely what causes the most confusing amount of code that elevates it above a simple piece of example code.

[drakkan]

Resolved in #384

Still left unimplemented is the Append flag,

Append should be fixed server side too.

pkg/sftp assumes that the client send correct offsets for append too.

Spec v3 say:

SSH_FXF_APPEND
      Force all writes to append data at the end of the file.

so it is not explicit. Specs v4 and forward:

SSH_FXF_APPEND
      Force all writes to append data at the end of the file.  The
      offset parameter to write will be ignored.

but as noted in the other PR, getting this working would require a pretty radical rewrite, which I have played with a bit already, but it has become clear working on it, that it would require a pretty drastic departure away from the scope of this project into providing an actual POSIX compliant in-memory file system. I might start a personal package to do so, which would then also be compatible with this project. I actually think supporting symlinks in the example package is precisely what causes the most confusing amount of code that elevates it above a simple piece of example code.