open with SSH_FXF_TRUNC without SSH_FXF_CREAT violates SFTP spec, but everyone does it anyway #1050

New Issue

tahoe-lafs · 2010-05-19T04:14:22Z

bj0 commented

2010-05-19 04:14:22 +00:00

It apparrently is against the SFTP spec (but not the POSIX) to open a file with SSH_FXF_TRUNC without including SSH_FXF_CREAT.

Programs like OpenSSH server and sshfs ignore this little detail, though, and programs like "cp" fail if it is followed.

workaround:
SFTP frontend should accept FXF_WRITE | FXF_TRUNC.

It apparrently is against the SFTP spec (but not the POSIX) to open a file with SSH_FXF_TRUNC without including SSH_FXF_CREAT. Programs like OpenSSH server and sshfs ignore this little detail, though, and programs like "cp" fail if it is followed. workaround: SFTP frontend should accept FXF_WRITE | FXF_TRUNC.

tahoe-lafs added the

labels 2010-05-19 04:14:22 +00:00

tahoe-lafs added this to the undecided milestone 2010-05-19 04:14:22 +00:00

davidsarah commented

2010-05-19 04:17:20 +00:00

See http://tools.ietf.org/html/draft-ietf-secsh-filexfer-02#section-6.3 for the relevant specification of FXF_TRUNC and FXF_CREAT, and http://www.opengroup.org/onlinepubs/000095399/functions/open.html for the POSIX open call.

sshfs appears to just pass the POSIX flags along with the obvious mappings.

See <http://tools.ietf.org/html/draft-ietf-secsh-filexfer-02#section-6.3> for the relevant specification of FXF_TRUNC and FXF_CREAT, and <http://www.opengroup.org/onlinepubs/000095399/functions/open.html> for the POSIX open call. sshfs appears to just pass the POSIX flags along with the obvious mappings.

tahoe-lafs added

code-frontend

and removed

unknown

labels 2010-05-19 04:17:20 +00:00

tahoe-lafs modified the milestone from undecided to 1.7.0

2010-05-19 04:17:20 +00:00

bj0 commented

2010-05-19 06:04:11 +00:00

the SFTP frontend now accepts WRITE | TRUNC, so

cp file1 file2

works.

but doing:

cat file1 > newfile

or

cat file1 >> newfile

when newfile doesn't already exist fails with a:

bash: newfile: No such file or directory

it does create a 'newfile' file, but it has zero length. (and repeating the command will succeed on the existing file)

from strace, it appears to be opening 'newfile' with the flags WRITE | CREAT | TRUNC.

the SFTP frontend now accepts WRITE | TRUNC, so ``` cp file1 file2 ``` works. but doing: ``` cat file1 > newfile ``` or ``` cat file1 >> newfile ``` when newfile doesn't already exist fails with a: ``` bash: newfile: No such file or directory ``` it does create a 'newfile' file, but it has zero length. (and repeating the command will succeed on the existing file) from strace, it appears to be opening 'newfile' with the flags WRITE | CREAT | TRUNC.

davidsarah commented

2010-05-20 04:56:35 +00:00

The problem in comment:77424 is due to the shell assuming that newfile already exists just after the open call. It does a stat which causes a getAttrs request, which fails with NoSuchChildError (FX_NO_SUCH_FILE). Then newfile is closed, but nothing has been written to it, so it is zero-length.

The ticket1037 branch has been changed to keep track of which paths correspond to open files. This allows us to return fake attributes from the getAttrs request (without having to write a zero-length file on the open call). zooko: please review this changeset, and bj0: please pull and re-test.

The problem in [comment:77424](/tahoe-lafs/trac-2024-07-25/issues/1050#issuecomment-77424) is due to the shell assuming that newfile already exists just after the open call. It does a `stat` which causes a `getAttrs` request, which fails with `NoSuchChildError` (`FX_NO_SUCH_FILE`). Then newfile is closed, but nothing has been written to it, so it is zero-length. The ticket1037 branch has been changed to keep track of which paths correspond to open files. This allows us to return fake attributes from the `getAttrs` request (without having to write a zero-length file on the open call). zooko: please review [this changeset](http://tahoe-lafs.org/trac/tahoe-lafs-ticket1037/changeset/4304), and bj0: please pull and re-test.

davidsarah commented

2010-05-20 05:00:45 +00:00

Please also test 'touch newfile' before and after pulling the changes. Based on https://bugzilla.gnome.org/show_bug.cgi?id=522532 , I think it might have the same issue.

Please also test '`touch newfile`' before and after pulling the changes. Based on <https://bugzilla.gnome.org/show_bug.cgi?id=522532> , I think it might have the same issue.

bj0 commented

2010-05-20 07:14:00 +00:00

I just checked and

touch newfile

Does work before the latest patches, although it takes like 20s before the new file appears in ls

After the patch, it shows up in ls instantly. Also:

cat test2 > newfile

works.

I just checked and ``` touch newfile ``` Does work before the latest patches, although it takes like 20s before the new file appears in ls After the patch, it shows up in ls instantly. Also: ``` cat test2 > newfile ``` works.

davidsarah commented

2010-05-20 16:01:47 +00:00

Thanks for the testing.

The current code is not quite right: it indexes the global_open_files dict by canonical path, but this does not include the root node. So, paths for different user accounts could be confused. It should instead index by the directory entry, i.e. parent cap and child name.

The test coverage also needs to be improved to cover cases where the same directory entry is opened more than once, and where files are still open when a connection is logged out.

Thanks for the testing. The current code is not quite right: it indexes the `global_open_files` dict by canonical path, but this does not include the root node. So, paths for different user accounts could be confused. It should instead index by the directory entry, i.e. parent cap and child name. The test coverage also needs to be improved to cover cases where the same directory entry is opened more than once, and where files are still open when a connection is logged out.

davidsarah commented

2010-05-22 04:22:51 +00:00

Replying to davidsarah:

The current code is not quite right: it indexes the global_open_files dict by canonical path, but this does not include the root node. So, paths for different user accounts could be confused. It should instead index by the directory entry, i.e. parent cap and child name.

This is now fixed. Also, we now try to support renaming immutable files that have been opened for writing or creation, while they are still open. Many apps depend on this: they create a temporary file, rename it to the destination file, and then close it.

bj0: please re-test both the previous tests, and saving files with gedit and OpenOffice.

Replying to [davidsarah](/tahoe-lafs/trac-2024-07-25/issues/1050#issuecomment-77428): > The current code is not quite right: it indexes the `global_open_files` dict by canonical path, but this does not include the root node. So, paths for different user accounts could be confused. It should instead index by the directory entry, i.e. parent cap and child name. This is now fixed. Also, we now try to support renaming immutable files that have been opened for writing or creation, while they are still open. Many apps depend on this: they create a temporary file, rename it to the destination file, and then close it. bj0: please re-test both the previous tests, and saving files with `gedit` and [OpenOffice](wiki/OpenOffice).

bj0 commented

2010-05-25 04:20:02 +00:00

I tried, and got no errors with:
cp
mv
cat >
cat >>
gedit
OpenOffice

gedit and OO both saved fine it seemed.

One odd thing i noticed was that when i open a file with vi, then close it, the .swp file isn't deleted. I took a quick look at the flog and it looks like it called removeFile and got a SUCCESS, but the file is still there.

I tried, and got no errors with: cp mv cat > cat >> gedit [OpenOffice](wiki/OpenOffice) gedit and OO both saved fine it seemed. One odd thing i noticed was that when i open a file with vi, then close it, the .swp file isn't deleted. I took a quick look at the flog and it looks like it called removeFile and got a SUCCESS, but the file is still there.

davidsarah commented

2010-06-08 03:34:12 +00:00

Assigning to bj0 to check whether the remaining bug in comment:77430 still exists. (It should have been fixed by the changes to handle remove/close race conditions.)

Assigning to bj0 to check whether the remaining bug in [comment:77430](/tahoe-lafs/trac-2024-07-25/issues/1050#issuecomment-77430) still exists. (It should have been fixed by the changes to handle remove/close race conditions.)

davidsarah commented

2010-06-12 23:48:13 +00:00

Fixed according to bj0 on IRC.

tahoe-lafs added the

fixed

label 2010-06-12 23:48:13 +00:00

tahoe-lafs closed this issue

2010-06-12 23:48:13 +00:00

warner added

code-frontend-ftp-sftp

and removed

code-frontend