ftp_NLST and ftp_LIST should only send data as "str"

[oberstet]

When doing a DIR from a FTP client to a Twisted FTP based server, I got a traceback

        --- <exception caught here> ---
          File "/home/autobahn/python/lib/python2.7/site-packages/Twisted-11.1.0_r33225-py2.7-freebsd-8.2-RELEASE-p3-i386.egg/twisted/internet/defer.py", line 545, in _runCallbacks
            current.result = callback(current.result, *args, **kw)
          File "/home/autobahn/python/lib/python2.7/site-packages/Twisted-11.1.0_r33225-py2.7-freebsd-8.2-RELEASE-p3-i386.egg/twisted/protocols/ftp.py", line 907, in gotListing
            self.dtpInstance.sendListResponse(name, attrs)
          File "/home/autobahn/python/lib/python2.7/site-packages/Twisted-11.1.0_r33225-py2.7-freebsd-8.2-RELEASE-p3-i386.egg/twisted/protocols/ftp.py", line 421, in sendListResponse
            self.sendLine(self._formatOneListResponse(name, *response))
          File "/home/autobahn/python/lib/python2.7/site-packages/Twisted-11.1.0_r33225-py2.7-freebsd-8.2-RELEASE-p3-i386.egg/twisted/protocols/ftp.py", line 385, in sendLine
            self.transport.write(line + '\r\n')
          File "/home/autobahn/python/lib/python2.7/site-packages/Twisted-11.1.0_r33225-py2.7-freebsd-8.2-RELEASE-p3-i386.egg/twisted/internet/_newtls.py", line 180, in write
            FileDescriptor.write(self, bytes)
          File "/home/autobahn/python/lib/python2.7/site-packages/Twisted-11.1.0_r33225-py2.7-freebsd-8.2-RELEASE-p3-i386.egg/twisted/internet/abstract.py", line 300, in write
            raise TypeError("Data must not be unicode")
        exceptions.TypeError: Data must not be unicode

This is Twisted trunk (r33225) on FreeBSD i386 / Python 2.7.2

The Unicode data is coming from the FTP directory listing formatting function

ftp.DTP. _formatOneListResponse

[therve]

Your patch needs tests to be able to be accepted.

Also, can you give some sample code which would allow to reproduce the problem? In particular, having the FTPShell that you're using would be interesting.

Thanks!

[oberstet]

Found a different solution: the ultimate trigger of the error was passing a Unicode string (which actually was plain ASCII) as userHome to FTPRealm:

p = Portal(FTPRealm(anonymousRoot = '/dev/null', userHome = self.homedir), [FtpUserDb(self)])

Making userHome type str solves it.

So I guess the issue is none.

[exarkun]

From the IFTPShell.list documentation:

@return: A Deferred which fires with a list of (name, list), where the name is the name of the entry as a unicode string and each list contains values corresponding to the requested keys.

So either the documentation should be fixed or the implementation should be.

[exarkun]

RFC 3659, section 2.2 discusses the encoding of filenames. It says:

Various FTP commands take pathnames as arguments, or return pathnames in responses. When the MLST command is supported, as indicated in the response to the FEAT command, pathnames are to be transferred in one of the following two formats.

pathname = utf-8-name / raw
utf-8-name = <a UTF-8 encoded Unicode string>
raw = <any string that is not a valid UTF-8 encoding>

Which format is used is at the option of the user-PI or server-PI sending the pathname.

[exarkun]

So either the documentation should be fixed or the implementation should be.

I'll take a bit more of a stand. The documented behavior is better than the implemented behavior. If IFTPShell.list returns a unicode, encode it to UTF-8 and send the result.

Separately, we may want to eliminate the str case. This is nice for reasons of idealized correctness: the wire format is up to the protocol, and text encoding is part of the wire format. However, since Which format is used is at the option of the user-PI or server-PI sending the pathname, and because several mainstream platforms still exist which have filenames made of bytes, not text, the str case may still be necessary.

So I suggest leaving support for str for the time being, not deprecating or removing it.

[adiroiban]

Both ftp_NLST and ftp_LIST are affected.

---

As suggested by exarkun my plan is to implement this by allowing FTPShell.list to return both str or unicode, and then, if requried, convert to str before sending the data on the "wire".

The attached patch, implements "unicode" - > "str" conversion in DTP.sendLine()

Please advice if you think that this is ok.

Thanks!

[exarkun]

Hi adiroiban. Thanks for your work on this. I think the approach taken isn't quite right, though. This change will encode any unicode strings sent over the DTP using UTF-8. From what I can tell from reading the RFC, this UTF-8 encoding is only appropriate for filenames.

Thus, I think the necessary change is in callers of IFTPShell.list - ftp_LIST and ftp_NLST - to make them check the type of the filename the shell has given back and perform the encoding at that point.

The proposed patch here mostly accomplishes the same thing, but I'm worried about the implicit UTF-8 encoding it adds for the responses to all other commands. This seems like it will encourage people to be sloppy about their types in other cases where it isn't appropriate. In fact, perhaps it would even make sense for sendLine to check for unicode and warn about it (later to be changed to an error, after applications have had time to fix any mistakes they were making).

Thanks again.

[adiroiban]

Hi,

Many thanks for the review.

I have attached a new diff which adds a warning in sendLine and converts to utf-8 on ftp_LIST and ftp_NLST level.

A live version of the diff is available here:

 https://github.com/chevah/twisted/compare/chevah:master...chevah:5411-ftp-list-unicode

[exarkun]

(In [37043]) Branching to 'ftp-list-response-encoding-5411'

[exarkun]

(In [37044]) Apply 5411-2.diff

refs #5411

[exarkun]

Thanks.

1. twisted/test/test_ftp.py
   1. I try to reserve use of TestCase.patch with globals (eg classes or methods on classes) for circumstances where there's no way to do the monkey patching in a more localized way. In this list case which interests test_LISTUnicode and test_NLSTUnicode, there are two more localized options. The ideal is to substitute a different kind of IFTPShell implementation entirely, within the authentication configuration, so that no patching is required at all. A second approach, which is easier if not quite as nice, is to patch the instance instead of the class. The instance is available at self.serverProtocol.shell after login, so the test can reach it easily. This isn't a huge deal, but the more localized such hacks can be, the better.
   2. There are some typos or spelling errors:
      1. pached should be patched
      2. Whend / When
      3. autoamtically / automatically
   3. Also, Twisted naming convention prefers fooBar over foo_bar
2. twisted/protocols/ftp.py
   1. Pretty much the only warning Twisted uses is DeprecationWarning (use tentatively outlined on CompatibilityPolicy). Also, warnings should be "pointed at" the code which triggered them, not the code which is emitting them. This usually means stacklevel=2 is needed as a warnings.warn argument (though stack-level based reporting is woefully error prone and sometimes more complicated schemes are necessary).
   2. The deprecated unicode support for the line argument shouldn't be advertised in the sendLine docstring. It should insist on str - or, in fact, bytes now, to avoid Python 3 induced ambiguity - so as to avoid confusing people about what they should be passing.
   3. Since the stack-based system is so poor, warning text itself needs to clearly identify the source of the warning.
   4. Use L{} to link to Python names, not I{}.
   5. epytext requires that @param and related markup go at the end, after any freeform text.
   6. Same comment again about fooBar vs foo_bar - name_encoded should be nameEncoded.
   7. I think a parallel warning is needed in the case where IFTPShell.list returns non-ASCII byte string. Previously this worked, even though it disagreed with the interface documentation; the change proposed here will break it, as bytes.encode('utf-8') first decodes bytes using the ASCII codec. The code should handle a bytes return type by warning it should be unicode instead and skipping the encode.
3. Should have a news fragment as well

[exarkun]

 Build results

[adiroiban]

Hi,

I am sorry for the silly errors from the patch.

I have attached a new patch based on trunk. It is not a diff on to for 5411-2.diff

I live diff is here:  https://github.com/chevah/twisted/compare/chevah:master...chevah:5411-ftp-list-unicode

1. twisted/test/test_ftp.py

1. I patched the FTPShell instance.

2. I fixed the typos. Sorry for that.

3. I also fixed the naming convention. It is hard to do it right when not used with this convention.

1. twisted/protocols/ftp.py

1. I fixed the link to Python names.

2. epytext is placed at the end.

3. line argument is advertised in the doc as 'bytes'

4. style of the variables names was updated.

5. The warning now included the name of the method from where it is called.

6. I switched to using the DeprecationWarning ... but I have no idea what I'm doing there :)

7. Not sure what to do about the parallel warning for IFTPShell.list when non-ASCII byte string are returned. What is a "non-ASCII byte string"? I tried to implement something basef of what I think should be done. I put the code in a different function, to have a clean test. Let me know if you think an integration test with ftp_NLST and ftp_LIST is required.

8. [unsolved] When testing the code with stacklevel=2, the call is made from twisted/twisted/trial/_synctest.py. Please advise how should I proceed to get the filename for _synctest module.

[exarkun]

Hi adiroiban,

Thanks for your work on this ticket again. Sorry I wasn't clear the last time I touched this ticket, I actually went ahead and addressed my own review feedback in the branch (r37045 - which trac should have linked to, but it appears to fail to have done so), which is why I re-submitted it for review shortly afterwards.

You should be able to find answers to your two questions in that changeset, at least. To summarize briefly:

• By "non-ASCII byte string" I meant a string like "my resum\xc3\xa9". Looking at your version of the fix, it looks like you interpreted this correctly.
• twisted.python.deprecate.warnAboutFunction is a useful helper for non-stack-based warnings, which is what's necessary to warn about the return value of a function you just called being wrong (since the function is necessarily no longer on the stack after it has returned).

[adiroiban]

Hi. No problem.

I saw the linked branch but did not bother to check its content. I thought it is just there to prepare the branch on which my patch will be applied.

I also so the "review" keyword, but I ignored it. So it is my fault.

I was expecting a comment informing that the you have fixed the issues and they are waiting for a review.

---

I have reviewed your changes and they look good.

My comment is whether

try: 
    return name.encode('utf-8')
except UnicodeEncodeError:

is better than:

isinstance(name, unicode): 
    return name.encode('utf-8') 

[exarkun]

My comment is whether {{{ try: return name.encode('utf-8') except UnicodeEncodeError: }}} is better than: {{{ isinstance(name, unicode): return name.encode('utf-8') }}}

They're different, certainly. :) Consider these cases:

1. name is str containing only ASCII-encodable bytes
2. name is str containing non-ASCII encodable bytes
3. name is unicode containing only ASCII-encodable bytes
4. name is unicode containing non-ASCII encodable bytes
5. name is neither str nor unicode

The try/except version of the code will reject cases 2 and 5. The isinstance version of the code will reject none of the cases. Case 5 isn't very important, it's an application bug, not our problem. But case 2 is somewhat important, since it's how an application would be written today that actually wanted to send non-ASCII filenames to clients. Breaking that would be mean.

[tom.prince]

1. comment 8 suggested that returning bytes from IFTPShell.list should probably not be deprecated, since the protocol allows arbitrary byte strings there. This would suggest that IFTPShell.list be changed to say that the name can either be a byte or unicode string. I don't think this is an incompatible change, bytes is the only thing supported by current consumers, and unicode is what is currently documented.
2. I was going to suggest that the tests be made synchronous, but it appears that use loopback TCP, so they need the reactor. Please file a ticket to fix this.

Please resubmit after addressing the above issues.

[exarkun]

I'm not planning to work on this further in the near future. I wonder if adiroiban would like to finish it up.

[adiroiban]

I will continue your work. Great work, btw :)

I am confused and don't know how should I change the current implementation of IFTPShell.list.

Would it be possible to clarify this over IRC? Thanks!

---

Here is the ticket for refactoring the test to not require the reactor: #6331

Please let me know if I got it right.

[adiroiban]

I talked with Tom Prince over IRC and from what I understood he requested to update the docstring for IFTPShell.list to inform users that they can return both unicode and str.

I think that encouraging the mixed usage of both encoded and un-encoded unicode is wrong.

I am not going to implement this :)

I fill that JP's work is good and it does not require changes.

Cheers, Adi

[tom.prince]

I don't think things that are exposed to filesystem path names can be forced to unicode, so I don't think no supporting bytes is an option.

[tom.prince]

Minor change: Prefer assertIn to assertTrue(... in ...).

I've fixed this up and merged forward.

Will merge once  build results are in.

[tom.prince]

That last comment should have been on #1333.

[tom.prince]

I think that encouraging the mixed usage of both encoded and un-encoded unicode is wrong.

I agree with this in principle, but I think in practice that this is unavoidable. There exists filesystems that don't contain unicode and dropping support for them isn't a good thing.

In any case, the existing in-tree implementations of IFTPShell *don't* return unicode and changing that would be backwards-incompatible. I don't see the point of deprecating the de-facto type of return value of IFTPShell.list.

[adiroiban]

No offence, but how you can agree with a principle (an accepted or professed rule of action or conduct) and in the same time think that in practice it is unavoidable (ie can not be put in practice)?

---

It looks like IFTPShell does return Unicode ... or otherwise, what is the point of this whol ticket? See ticket description.

---

I strongly believe that mixed usage of encoding can be avoided.

I volunteer to fix the current IFTPShell.list implementation so that it will always return Unicode.

Would that help?

---

You cite comment 8 where JP requested to keep 'str' support, but later in comment 16 (point 2.7) he requested to deprecated support for 'str' .

Does it mean that comment 8 from JP is still valid?

---

The point of deprecating return of str from IFTPShell.list is to stop encouraging mixed usage of text strings.

---

One option is to drop Unicode support and then IFTPShell will consistently handle only str / byte strings.

---

No hard feelings! I just wanted to make my point clear.

Please correct me if I got something wrong.

I apology for this ping-pong.

Cheers, Adi

[exarkun]

In any case, the existing in-tree implementations of IFTPShell *don't* return unicode and changing that would be backwards-incompatible.

Well... sort of. IFTPShell.list is documented as returning the path name as unicode. Changing an implementation match up with the interface it supposedly implements is a bit fuzzier as far as backwards compatibility goes. The interface and implementation are in conflict, so *something* has to change incompatibly. It comes down to a question of what will do the least harm.

I volunteer to fix the current IFTPShell.list implementation so that it will always return Unicode. Would that help?

You can only fix the implementation in Twisted (and, I suppose, your own). You can't fix anyone else's implementation, and those need to keep working as well.

So both unicode and bytes must be supported. And as Tom said, it's not always the case that you can actually represent a path name as unicode instead of bytes. What do you tell the people who are in that situation, if bytes is not allowed?

[adiroiban]

Thanks JP for the comment.

Does it mean that you "revoke" your comment number 16 point 2.7 together with your code and you no longer want a deprecation warning ?

I am confused.

[exarkun]

Does it mean that you "revoke" your comment number 16 point 2.7 together with your code and you no longer want a deprecation warning ?

It seems like, for now at least, the case shouldn't even be deprecated, yes. If you have ideas for a cleaner interface that doesn't encourage the mixing of bytes and text like this, I think we should explore them. If we figure out something that both agrees with the reality that some filesystems are made out of bytes, not text, but that presents a friendlier interface to developers, then we can consider deprecating bytes return values from IFTPShell.list.

[adiroiban]

Sorry for being a PITA. I still want to do one more try before giving up and ending with a "twisted" IFTPShell.list()

I think that this bug is due to the way os.listdir is implemented in 2.3 and up.  http://docs.python.org/2/library/os.html

Changed in version 2.3: On Windows NT/2k/XP and Unix, if path is a Unicode object, the result will be a list of Unicode objects. Undecodable filenames will still be returned as string objects.

So os.list can return a list of "bytes objects", a list of "unicode objects" or a mixed list of unicode and bytes objects. This is fine for a low level API.

Still, for high level FTP API like sending a list of names to the FTP client, I think that the FTP server should be consistent, handle the error filesystem encoding error and not mix the objects. Otherwise we are just propagating the "filename error" without handling and expect it to be handled by the client.

---

I have attached a patch on top of ftp-list-response-encoding-5411 branch which removed the deprecation working for IFTPShell.list result.

It also removes the deprecation warnings for DTP.send_line since now we should not have unicode reaching this level.

Let me know if you want to keep this warning.

---

Is it so hard to ask custom implementation of IFTPShell.list to update the code in order to return a consistent result?

The current code does fix this ticket but we are just hiding the dirt under the carpet.

[tomprince]

(In [37825]) Apply 5411-4 from adiroiban.

Refs: #5411

[tom.prince]

<... a bunch of comments that don't suggest anything specific ...>

I'm not sure how to respond to this.

Is it so hard to ask custom implementation of IFTPShell.list to update the code in order to return a consistent result?

It isn't hard, but it is incompatible, and/or doesn't agree with the RFC.

The current code does fix this ticket but we are just hiding the dirt under the carpet.

I'd suggest rather that we are documenting that the standard mandates that dirt be allowed on the floor (which it does, since there are existing implementations that have it on the floor).

1. test_NLSTNonASCIIBytes should still exist.
2. The documentation for IFTPShell.list needs to be updated. In particular, I noticed that the path argument it takes is documented as being unicode, but is in fact bytes.
3. warnAboutFunction isn't used, so shouldn't be imported.

[adiroiban]

Replying to tom.prince:

1. test_NLSTNonASCIIBytes should still exist. 2. The documentation for IFTPShell.list needs to be updated. In particular, I noticed that the path argument it takes is documented as being unicode, but is in fact bytes. 3. warnAboutFunction isn't used, so shouldn't be imported.

1. I have re-added both NLST and LIST NonASCIIBytes tests... but without the deprecation warning.

2. I have update the IFTPShell.list to inform that bytes can be passed.

3. I have removed warnAboutFunction. Sorry for that. My text editor is running pyflakes and everthing is messed up with twisted code.

I have attached a patch on top of previous patch.

I hope that this time everthing is ok.

Sorry again for all the trouble and thanks for all your work!

[exarkun]

Is it so hard to ask custom implementation of IFTPShell.list to update the code in order to return a consistent result?

It isn't hard, but it is incompatible, and/or doesn't agree with the RFC.

The current code does fix this ticket but we are just hiding the dirt under the carpet.

I'd suggest rather that we are documenting that the standard mandates that dirt be allowed on the floor (which it does, since there are existing implementations that have it on the floor).

Since it seems there is a desire for something nicer here, perhaps twisted.protocols.ftp could additionally supply a helper API for IFTPShell implementations that are forced to deal with bytes but want to be nice and give unicode to the protocol implementation (and therefore UTF-8, I think?) to the client.

This is an idea for a future ticket though, not meant as an objection to the latest work done on this ticket or even a suggestion that more work in this direction should be done before resolving this ticket. (Also, I'm not even sure it's possible to have such a general helper function, but who knows.)

[adiroiban]

Thanks exarkun for your comment. This thread is already to long and I also hope that the patch at is now will be accepted soon.

We can continue the discussion on the ticket dedicated to UTF-8 support.

[tomprince]

(In [38009]) Apply 5411-5.diff from adiroiban.

Refs: #5411.

[tom.prince]

A couple of minor documetnation points.

1. _checkListResult:
   • The @return annotation might be clearer if it just said that it is the encoded or wire-formated version of name.
   • The documentation might be made clearer if the function was renamed according to what it did, rather than where it gets used. (i.e. it encodes unicode to utf8 and passes other stuff through.
2. test_LISTNonASCIIBytes the docstring needs updating.

Otherwise, this looks good.

[tom.prince]

 buildresults eventually

[adiroiban]

Hi. Thanks for the review. I have attached a new diff on top of previous diff.

The buildresult are failing on Win XP but It does not look like they are cause by changes in FTP protocol.

pyflakes and twistedcheker are reporting many other errors for ftp code.

If you want, I can fix them in this branch, but I prefer to fix them in another ticket/branch.

[tomprince]

(In [38102]) Apply 5411-6.diff from adiroiban.

Refs: #5411.

[tom.prince]

Those errors aren't actually new, or in some cases real. (Unfortunately our infrastructure doesn't handle this ideally yet).

The changes looks good, although I think the docstring and name of _checkWireFormat could be better. I've gone ahead and changed it to explain exactly what it does, and added reference the RFC that motivates the behavior. (The docstring you had made sense in the context of this ticket, but I think would be hard to understand in isolation).

I'll go ahead and merge ones the buildbot results come back.

[tomprince]

(In [38106]) Merge ftp-list-response-encoding-5411: Encode unicode names from ftp_NLST and ftp_LIST.

Author: adiroiban, exarkun Reviewers: therve, exarkun, tom.prince Fixes: #5411

twisted.protocols.ftp.FTP now supports IFTPShell implementations which return non-ASCII filenames as unicode strings.