ftp "ls" command is broken #2394

New Issue

warner · 2015-03-24T17:25:18Z

warner commented

2015-03-24 17:25:18 +00:00

While testing the fix for #2388, I noticed that FTP's "ls" command is broken. Apparently it was broken in 1.10.0 too, so I'm marking this as a release-blocking regression. I suspect it has to do with a change in Twisted, though, so maybe it worked at one point and we just got caught by an API change.

(ve)206:warner@brian-office-mini% ftp -P 8021 alice@localhost
Trying 127.0.0.1...
Connected to localhost.
220 Twisted 15.0.0 FTP Server
331 Password required for alice.
Password:
230 User logged in, proceed
Remote system type is UNIX.
Using binary mode to transfer files.
ftp> ls
227 Entering Passive Mode (127,0,0,1,220,56).
125 Data connection already open, starting transfer
<HANG>

twistd.log:

2015-03-24 10:16:13-0700 [-] Unexpected FTP error
2015-03-24 10:16:13-0700 [-] Unhandled Error
	Traceback (most recent call last):
	  File "/usr/local/lib/python2.7/site-packages/twisted/internet/base.py", line 824, in runUntilCurrent
	    call.func(*call.args, **call.kw)
	  File "/usr/local/Cellar/python/2.7.9/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/foolscap-0.7.0-py2.7.egg/foolscap/eventual.py", line 26, in _turn
	    cb(*args, **kwargs)
	  File "/usr/local/lib/python2.7/site-packages/twisted/internet/defer.py", line 383, in callback
	    self._startRunCallbacks(result)
	  File "/usr/local/lib/python2.7/site-packages/twisted/internet/defer.py", line 491, in _startRunCallbacks
	    self._runCallbacks()
	--- <exception caught here> ---
	  File "/usr/local/lib/python2.7/site-packages/twisted/internet/defer.py", line 578, in _runCallbacks
	    current.result = callback(current.result, *args, **kw)
	  File "/usr/local/lib/python2.7/site-packages/twisted/protocols/ftp.py", line 996, in gotListing
	    self.dtpInstance.sendListResponse(name, attrs)
	  File "/usr/local/lib/python2.7/site-packages/twisted/protocols/ftp.py", line 474, in sendListResponse
	    self.sendLine(self._formatOneListResponse(name, *response))
	  File "/usr/local/lib/python2.7/site-packages/twisted/protocols/ftp.py", line 464, in _formatOneListResponse
	    'permissions': permissions.shorthand(),
	exceptions.AttributeError: 'int' object has no attribute 'shorthand'

I'm guessing this involves allmydata.frontends.ftpd.Handler._populate_row, where it returns a fake (int) 0600 as the "permissions" key for all rows. I'm further guessing that Twisted's changed the API to require some sort of Permissions object.

We need to check the range of Twisteds with which we claim compatibility, look at the variety of ftp.IFTPShell interfaces required by that set, and find a way to be compatible with all of them.

While testing the fix for #2388, I noticed that FTP's "ls" command is broken. Apparently it was broken in 1.10.0 too, so I'm marking this as a release-blocking regression. I suspect it has to do with a change in Twisted, though, so maybe it worked at one point and we just got caught by an API change. ``` (ve)206:warner@brian-office-mini% ftp -P 8021 alice@localhost Trying 127.0.0.1... Connected to localhost. 220 Twisted 15.0.0 FTP Server 331 Password required for alice. Password: 230 User logged in, proceed Remote system type is UNIX. Using binary mode to transfer files. ftp> ls 227 Entering Passive Mode (127,0,0,1,220,56). 125 Data connection already open, starting transfer <HANG> ``` twistd.log: ``` 2015-03-24 10:16:13-0700 [-] Unexpected FTP error 2015-03-24 10:16:13-0700 [-] Unhandled Error Traceback (most recent call last): File "/usr/local/lib/python2.7/site-packages/twisted/internet/base.py", line 824, in runUntilCurrent call.func(*call.args, **call.kw) File "/usr/local/Cellar/python/2.7.9/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/foolscap-0.7.0-py2.7.egg/foolscap/eventual.py", line 26, in _turn cb(*args, **kwargs) File "/usr/local/lib/python2.7/site-packages/twisted/internet/defer.py", line 383, in callback self._startRunCallbacks(result) File "/usr/local/lib/python2.7/site-packages/twisted/internet/defer.py", line 491, in _startRunCallbacks self._runCallbacks() --- <exception caught here> --- File "/usr/local/lib/python2.7/site-packages/twisted/internet/defer.py", line 578, in _runCallbacks current.result = callback(current.result, *args, **kw) File "/usr/local/lib/python2.7/site-packages/twisted/protocols/ftp.py", line 996, in gotListing self.dtpInstance.sendListResponse(name, attrs) File "/usr/local/lib/python2.7/site-packages/twisted/protocols/ftp.py", line 474, in sendListResponse self.sendLine(self._formatOneListResponse(name, *response)) File "/usr/local/lib/python2.7/site-packages/twisted/protocols/ftp.py", line 464, in _formatOneListResponse 'permissions': permissions.shorthand(), exceptions.AttributeError: 'int' object has no attribute 'shorthand' ``` I'm guessing this involves `allmydata.frontends.ftpd.Handler._populate_row`, where it returns a fake (int) `0600` as the "permissions" key for all rows. I'm further guessing that Twisted's changed the API to require some sort of Permissions object. We need to check the range of Twisteds with which we claim compatibility, look at the variety of `ftp.IFTPShell` interfaces required by that set, and find a way to be compatible with all of them.

warner added the

code-frontend-ftp-sftp

normal

defect

1.10.0

labels 2015-03-24 17:25:18 +00:00

warner added this to the 1.10.1 milestone 2015-03-24 17:25:18 +00:00

daira commented

2015-03-24 18:25:41 +00:00

This looks like it might be due to https://twistedmatrix.com/trac/changeset/42473/trunk/twisted/protocols/ftp.py, which changed the permissions to be represented as a twisted.python.filepath.Permissions object.

This looks like it might be due to <https://twistedmatrix.com/trac/changeset/42473/trunk/twisted/protocols/ftp.py>, which changed the permissions to be represented as a `twisted.python.filepath.Permissions` object.

warner commented

2015-03-26 01:03:26 +00:00

Yeah, I think that's it. Twisted-14.0.2 expected an int, Twisted-15.0.0 expects a Permissions (and the docstring is still out of date: https://twistedmatrix.com/trac/ticket/7833).

I don't see any clean way to test which version of Twisted is calling us, so I think I'm going to use an ugly hack. The old Twisted uses this function to render the integer mode value (https://github.com/twisted/twisted/blob/twisted-14.0.2/twisted/protocols/ftp.py#L428):

def formatMode(mode):
    return ''.join([mode & (256 >> n) and 'rwx'[n % 3] or '-' for n in range(9)])

and the new Twisted uses this (https://github.com/twisted/twisted/blob/twisted-15.0.0/twisted/protocols/ftp.py#L464)

'permissions': permissions.shorthand(),

So I'm thinking we create a subclass of Permissions that overrides the *and* operator to return an int for the first form, but has a .shorthand() for the second form.

Evil! :-)

Yeah, I think that's it. Twisted-14.0.2 expected an int, Twisted-15.0.0 expects a `Permissions` (and the docstring is still out of date: <https://twistedmatrix.com/trac/ticket/7833>). I don't see any clean way to test which version of Twisted is calling us, so I think I'm going to use an ugly hack. The old Twisted uses this function to render the integer mode value (<https://github.com/twisted/twisted/blob/twisted-14.0.2/twisted/protocols/ftp.py#L428>): ``` def formatMode(mode): return ''.join([mode & (256 >> n) and 'rwx'[n % 3] or '-' for n in range(9)]) ``` and the new Twisted uses this (<https://github.com/twisted/twisted/blob/twisted-15.0.0/twisted/protocols/ftp.py#L464>) ``` 'permissions': permissions.shorthand(), ``` So I'm thinking we create a subclass of `Permissions` that overrides the `*and*` operator to return an int for the first form, but has a `.shorthand()` for the second form. Evil! :-)

warner commented

2015-03-26 01:10:47 +00:00

Hrm, we support back to Twisted-11.0.0 (on windows), which didn't have Permissions (it was introduced in 11.1.0).

Hrm, we support back to Twisted-11.0.0 (on windows), which didn't have `Permissions` (it was introduced in 11.1.0).

warner commented

2015-03-26 01:34:38 +00:00

(https://github.com/tahoe-lafs/tahoe-lafs/pull/148) has a branch for review. The evil hack seems to work, but if someone could also test it on a box with Twisted-11.0.0 (which will require windows, I think), I'd appreciate it.

warner self-assigned this 2015-03-26 01:34:38 +00:00

daira commented

2015-03-31 16:45:35 +00:00

We decided to simplify the hack by requiring Twisted >= 11.1.0 on Windows. This also simplifies the test in test_ftp.test_list, because we don't need to account for the permissions value being either an IntishPermissions or an int.

We decided to simplify the hack by requiring Twisted >= 11.1.0 on Windows. This also simplifies the test in `test_ftp.test_list`, because we don't need to account for the permissions value being either an `IntishPermissions` or an `int`.

warner commented

2015-03-31 18:11:14 +00:00

Landed in d7b763c

warner added the

fixed

label 2015-03-31 18:11:14 +00:00

warner closed this issue

2015-03-31 18:11:14 +00:00

Sign in to join this conversation.