classification
Title: Unhandled exception (TypeError) with ftplib in function retrbinary/retrlines causes inoperable behavior without crashing
Type: behavior Stage: patch review
Components: Library (Lib) Versions: Python 3.6, Python 3.5
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Sam Adams, SilentGhost, giampaolo.rodola, r.david.murray
Priority: normal Keywords: needs review, patch

Created on 2015-12-23 19:38 by Sam Adams, last changed 2016-03-08 19:59 by giampaolo.rodola.

Files
File name Uploaded Description Edit
ftplib.py Sam Adams, 2015-12-24 02:07
issue25933.diff SilentGhost, 2015-12-24 10:43 review
Messages (7)
msg256927 - (view) Author: Sam Adams (Sam Adams) * Date: 2015-12-23 19:38
Hello,

I believe that I found a bug in ftplib.py in version 3.3.6.

Whenever a user opens a file for writing in nonbinary mode and then proceeds to call the retrbinary function, or opens the file in binary mode and proceeds to call the retrlines fuction, then attempts to write to the file using the callback function, an unhandled TypeError exception is raised that does not cause the program to crash, but rather cause the last message from the server to not be "read" by the readline() function, thus causing cascading errors until the function readline() is called on its own or the connection with the server is reset.

Code to replicate:
from ftplib import FTP
ftp = FTP('your.server.here')
ftp.login()
fileTest = open('filename','w') <---- not binary
ftp.retrbinary('retr randomfilehere',fileTest.write)

Unhandled exception raised: 

File "/usr/local/lib/python3.3/ftplib.py", line 434, in retrbinary
    callback(data)
TypeError: must be str, not bytes

Likewise, if 

ftp.retrlines('retr randomfilehere', fileTest.write) 

is called when fileTest is opened for writing with binary option the following error is raised:

File "/usr/local/lib/python3.3/ftplib.py", line 464, in retrlines
    callback(line)
TypeError: 'str' does not support the buffer interface

Calling ftp.getline() clears the error and resumes normal operation


Possible Solution:

add exception handling in lines 434/464 for TypeError
msg256929 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-12-23 19:48
Can you test this against 3.5?  I guessing it hasn't been fixed, but we did do some bytes/string fixes since 3.3, so it might have been.
msg256930 - (view) Author: Sam Adams (Sam Adams) * Date: 2015-12-23 19:57
I don't have access to 3.5 where I am now. I can try later on, but it appears after a quick glance that the code for this function between 3.3 and 3.5 is the same for calling the callback function.
msg256931 - (view) Author: SilentGhost (SilentGhost) * (Python triager) Date: 2015-12-23 20:37
With the default branch I get regular TypeError exception, without any extras. To me it seems that its responsibility of the caller to provide the correct callback for the type of data being retrieved. So, I'm not exactly sure that this is a bug at all.
msg256947 - (view) Author: Sam Adams (Sam Adams) * Date: 2015-12-24 02:07
Silent: The issue that i see is how the error is handled. I can trap the TypeError easily, however, if I keep the socket open, the behavior of ftplib will not be as intended. For example:

fileTest = open('filename1', 'wb')

ftp.retrlines('RETR README.html', fileTest.write)

This will give a TypeError, as intended

However, what happens next is, I believe, not intended --

If i send a command after this trapped error, say, ftp.sendcmd('NOOP')

The result will not be 
200 NOOP command successful

but, instead,

226 Transfer Complete

As the previous status was not read from the file created by the socket, so if i want to try and do anything for the user after, the message received, and subsequently parsed by the getresp function, will not be the message that is expected.

For a noop command this is ok, but for a transfer command, it will set the mode to either A or I and return a 200 status when a different status was expected. This will raise an error. 

The only way to fix this is to either run the internal function getline() or reset the connection, thus clearing all messages from the file.

I have attached a file that has a fix, Im not sure if this is helpful or not.

I modified the retrbinary function on lines:
440,449-450,454-455

I also modified the retrlines function on lines:
470,490-491,496-497
msg256960 - (view) Author: SilentGhost (SilentGhost) * (Python triager) Date: 2015-12-24 10:43
OK, here is the patch with the test that I think is exercising the issue.
msg261375 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2016-03-08 19:59
It looks to me that forcing self.voidresp() in case the callback raises an exception is a bad idea: it may easily end up hanging if the server is not supposed to send a response at that point (see: the transfer is not finished).

As for the issue per se: I think there's nothing wrong with the current behavior except perhaps we could provide a better error message. But in order to do that we should check file mode before hand and relying on the file mode is also not a good idea IMO because the file object can be anything.
History
Date User Action Args
2016-03-08 19:59:37giampaolo.rodolasetnosy: + giampaolo.rodola
messages: + msg261375
2015-12-24 10:43:10SilentGhostsetkeywords: + needs review, patch
files: + issue25933.diff
messages: + msg256960

stage: patch review
2015-12-24 02:07:47Sam Adamssetfiles: + ftplib.py

messages: + msg256947
2015-12-23 20:37:09SilentGhostsetnosy: + SilentGhost

messages: + msg256931
versions: + Python 3.5, Python 3.6, - Python 3.3
2015-12-23 19:57:33Sam Adamssetmessages: + msg256930
2015-12-23 19:48:16r.david.murraysetnosy: + r.david.murray
messages: + msg256929
2015-12-23 19:40:47Sam Adamssettype: behavior
2015-12-23 19:38:46Sam Adamscreate