SFTP frontend fails when listing a directory containing a mutable file, because it relies on node.get_size() to be an integer

[davidsarah]

When the SFTP frontend lists a directory, it sometimes raises a struct.error exception:

ssh-connection on SSHServerTransport,11,127.0.0.1] OPENDIRECTORY /
2010-02-06 19:09:37-0700 [SSHChannel session (0) on SSHService
ssh-connection on SSHServerTransport,11,127.0.0.1] CONVERT  []
2010-02-06 19:09:37-0700 [SSHChannel session (0) on SSHService
ssh-connection on SSHServerTransport,11,127.0.0.1] ROOT <DirectoryNode
RW-MUT oen7k>
2010-02-06 19:09:37-0700 [SSHChannel session (0) on SSHService
ssh-connection on SSHServerTransport,11,127.0.0.1] PATH []
2010-02-06 19:09:37-0700 [SSHChannel session (0) on SSHService
ssh-connection on SSHServerTransport,11,127.0.0.1] Unhandled Error
        Traceback (most recent call last):
[...]
File "/usr/lib/python2.6/dist-packages/twisted/conch/ssh/filetransfer.py", line 312, in _cbSendDirectory
            data += self._packAttributes(attrs)
File "/usr/lib/python2.6/dist-packages/twisted/conch/ssh/filetransfer.py", line 95, in _packAttributes
            data += struct.pack('!Q', attrs['size'])
        struct.error: cannot convert argument to long

(full report by Jody Harris ​here).

This happens because childnode.get_size() does not always return an integer. At least for mutable files, it can't, because the size isn't immediately available and the return value would have to be a Deferred. (For directories, the size is reported as 1 byte.)

There are two places where sftpd.py can end up putting a result of get_size() in a field that expects an integer: attrs['size'] in _populate_attrs​, and s.st_size in openDirectory​.

It is possible to use get_current_size() instead of get_size(), which does return a Deferred -- but that would involve extra network round trips to get the size of each mutable file, significantly slowing down directory listings even though the sizes are not usually very important (as mentioned in ticket:677#comment:2).

This is essentially the same problem as ticket #680 for FTP. However the SFTP protocol, unlike FTP, has the option ​not to include the size in the attributes for each directory entry. We should try that first and see whether SFTP clients handle it correctly. (It appears that sshfs will treat the size as zero anyway -- see line 619 ​here -- but at least we're allowing other clients the opportunity to avoid showing the user an incorrect size.)

For stat requests that apply to a single file (SSH_FXP_STAT/LSTAT/FSTAT), we should get the current size -- this is similar to getting the current size for t=json webapi requests (#677).

It seems based on _populate_row in ftpd.py that FTP is likely to have the same issue. Probably all uses of get_size() in non-test code need to be checked.

(Thanks to Zooko and Brian for thorough IRC discussion of this bug and #680.)

[davidsarah]

Initial attempt at a fix (no tests, not ready to apply)

[davidsarah]

In the description I gave a link to the latest Internet Draft for SFTP, but the one that is relevant is ​the last one that describes SFTP version 3, which is what twisted.conch.ssl.filetransfer implements. I don't think that affects very much, though (that version ​still allows the size to be omitted).

[davidsarah]

Replying to davidsarah:

... SFTP version 3, which is what twisted.conch.ssl.filetransfer implements.

twisted.conch.ssh.filetransfer, of course.

[zooko]

We're not going to finish this for v1.6.1, but hopefully for v1.7.0!

(Note: the missing ingredient at this point is tests of SFTP functionality.)

[davidsarah]

This is intended to be fixed by the new SFTP implementation in #1037.

A directory listing will show the size of a mutable file as '?' -- this matches the behaviour of the CLI. (The option to omit the size field only applies to STAT requests, not directory listings.)

[davidsarah]

Confirmed fixed by ​tests, and manually with command-line sftp and WinSCP.

[davidsarah]

sftp's ls -l command shows ? for the size of a mutable file, but WinSCP shows 0. This does not appear to prevent WinSCP from correctly downloading the file. The fact that some clients may show 0 for mutable file sizes should be documented.