Opened 4 years ago

Closed 4 years ago

#83 closed defect (fixed)

Crash on ibiblio.org

Reported by: ftputiluser Owned by: schwa
Priority: major Milestone: 3.2
Component: Library Version: 3.1
Keywords: mktime, OverflowError Cc: pombredanne@…

Description

Using pypi ftptil 3.1 on Win7, Python 2.6.6 in a virtualenv:

>>> host =ftputil.FTPHost('mirrors.ibiblio.org', 'anonymous', 'me@anonymous.com')
>>> host.listdir(host.curdir)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "c:\tests\lib\site-packages\ftputil\host.py", line 813, in listdir
    items = self._stat._listdir(path)
  File "c:\tests\lib\site-packages\ftputil\stat.py", line 642, in _listdir
    return self.__call_with_parser_retry(self._real_listdir, path)
  File "c:\tests\lib\site-packages\ftputil\stat.py", line 618, in __call_with_parser_retry
    result = method(*args, **kwargs)
  File "c:\tests\lib\site-packages\ftputil\stat.py", line 502, in _real_listdir
    for stat_result in self._stat_results_from_dir(path):
  File "c:\tests\lib\site-packages\ftputil\stat.py", line 475, in _stat_results_from_dir
    self._host.time_shift())
  File "c:\tests\lib\site-packages\ftputil\stat.py", line 340, in parse_line
    with_precision=True)
  File "c:\tests\lib\site-packages\ftputil\stat.py", line 187, in parse_unix_time
    hour, minute, 0, 0, 0, -1) )
OverflowError: mktime argument out of range
>>>

Change History (19)

comment:1 Changed 4 years ago by schwa

  • Keywords mktime OverflowError added
  • Status changed from new to assigned
  • Version set to 3.1

Hi, thanks for the report!

I can't reproduce the exception in either Python 2 nor Python 3:

Python 2.7.5 (default, Nov 12 2013, 16:18:42) 
[GCC 4.8.2 20131017 (Red Hat 4.8.2-1)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import ftputil
>>> host = ftputil.FTPHost('mirrors.ibiblio.org', 'anonymous', 'me@anonymous.com')
>>> host.listdir(host.curdir)
[u'.bplusvtoc_internal', u'.snapshot', u'.vtoc_internal', u'CPAN', u'CRAN', u'CTAN', u'abiclipart', u'apache', u'beonex', u'blastwave', u'designerunits', u'directory_list', u'eclipse', u'flightgear', u'flock', u'freetds', u'ggzgamingzone', u'gnu', u'grass', u'haiku', u'interactive-fiction', u'jpackage', u'maven', u'maven2', u'mozdev.org', u'mozilla', u'opencsw', u'openssl', u'ostalks', u'ovirt', u'pub', u'robots.txt', u'rsync', u'ruby', u'simgear', u'speakers', u'sugar', u'tdf', u'vinismo', u'wine', u'xemacs']

Python 3.3.2 (default, Nov  8 2013, 13:38:57) 
[GCC 4.8.2 20131017 (Red Hat 4.8.2-1)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import ftputil
>>> host = ftputil.FTPHost('mirrors.ibiblio.org', 'anonymous', 'me@anonymous.com')
>>> host.listdir(host.curdir)
['.bplusvtoc_internal', '.snapshot', '.vtoc_internal', 'CPAN', 'CRAN', 'CTAN', 'abiclipart', 'apache', 'beonex', 'blastwave', 'designerunits', 'directory_list', 'eclipse', 'flightgear', 'flock', 'freetds', 'ggzgamingzone', 'gnu', 'grass', 'haiku', 'interactive-fiction', 'jpackage', 'maven', 'maven2', 'mozdev.org', 'mozilla', 'opencsw', 'openssl', 'ostalks', 'ovirt', 'pub', 'robots.txt', 'rsync', 'ruby', 'simgear', 'speakers', 'sugar', 'tdf', 'vinismo', 'wine', 'xemacs']

This is on Linux though. I don't know if I can get access to a Windows machine. On the other hand, this shouldn't matter; if I remember correctly there's no code in ftputil that does different things depending on the client's operating system.

The directory in an ftp command line client also looks fine:

$ ftp mirrors.ibiblio.org
Connected to mirrors.ibiblio.org (152.19.134.44).
220 Welcome to mirrors.ibiblio.org FTP service.
Name (mirrors.ibiblio.org:schwa): anonymous
331 Please specify the password.
Password:
230 Login successful.
Remote system type is UNIX.
Using binary mode to transfer files.
ftp> dir
227 Entering Passive Mode (152,19,134,44,194,212).
150 Here comes the directory listing.
drwxrwxr-x   12 2557     2557         4096 Jun 16 22:16 CPAN
drwxrwsr-x   10 2557     2557         8192 Jun 16 04:09 CRAN
drwxr-xr-x   17 2557     2557         4096 Jun 15 22:02 CTAN
drwxr-xr-x    5 2098     100          4096 Sep 18  2010 abiclipart
drwxrwxr-x  162 2557     2557        16384 Jun 08 14:08 apache
drwxr-xr-x    5 2557     2557         4096 Nov 29  2004 beonex
drwxr-xr-x    4 2557     2557         4096 Sep 01  2011 blastwave
drwx------    3 2449     100          4096 Jan 19  2011 designerunits
lrwxrwxrwx    1 0        0              28 Apr 29  2011 directory_list -> /var/www/html/directory_list
drwxr-xr-x  189 2557     2557        16384 Jun 16 03:45 eclipse
drwxr-xr-x    4 2433     20120        4096 Dec 02  2003 flightgear
drwxr-xr-x    5 2557     2557         4096 Sep 10  2010 flock
drwxr-xr-x    6 0        0            4096 Mar 11  2012 freetds
drwxr-xr-x    4 2557     2557         4096 Nov 30  2007 ggzgamingzone
drwxr-xr-x    4 1941     20127        4096 Mar 08  2007 gnu
drwxr-xr-x   71 2557     2557       208896 Jun 15 00:22 grass
drwxr-xr-x    3 2557     2557         4096 May 08  2010 haiku
drwxrwxr-x   25 2557     2557         4096 Mar 14 03:05 interactive-fiction
drwxr-xr-x    3 2557     2557         4096 Apr 27  2012 jpackage
drwxrwxr-x 1097 1992     20038       90112 Feb 07  2006 maven
drwxrwxr-x  763 1992     20038       61440 Mar 22 03:00 maven2
drwxr-xr-x 1884 2557     2557       147456 Aug 25  2013 mozdev.org
drwxr-xr-x    5 2557     2557         4096 Mar 06 11:38 mozilla
drwxr-xr-x    8 2557     2557         4096 Apr 04 05:48 opencsw
drwxr-xr-x    4 2557     2557         4096 Dec 20  2012 openssl
drwxr-xr-x    6 2504     100          4096 Nov 28  2010 ostalks
drwxr-xr-x    4 2557     2557         4096 Apr 03 19:54 ovirt
drwxr-xr-x    2 0        0            4096 Jan 04  2011 pub
-rw-r--r--    1 0        0              39 Mar 22  2012 robots.txt
drwxrwsr-x    9 2557     2557         4096 May 26 18:53 rsync
drwxrwxr-x    8 2557     2557         4096 Aug 04  2013 ruby
drwxr-xr-x    3 2433     0            4096 Feb 23  2010 simgear
drwxr-xr-x   27 0        0            4096 Apr 08  2009 speakers
drwxrwsr-x    7 2557     2557         4096 May 30  2013 sugar
drwxr-xr-x    3 2557     0            4096 Aug 08  2013 tdf
drwxr-xr-x    2 2388     0            4096 Apr 27  2008 vinismo
drwxr-xr-x    3 2557     2557         4096 May 14  2012 wine
drwxrwsr-x   16 2557     2557         4096 May 30 16:06 xemacs
226 Directory send OK.
ftp> pwd
257 "/"

Was the directory that led to the exception actually the login directory or did you maybe "simplify" the bug report by leaving out a chdir call, assuming the exception would occur with any directory on that server?

The only explanation that right now comes to my mind is that Ibiblio might have had a problem and the directory listing was broken somehow. Even in this case though, I guess ftputil should give you a ParserError, not an OverflowError.

So, a few questions whose answers might help me find the cause of the problem:

  • Was the directory actually the login directory ("simplified" bug report, see above)?
  • Can you still reproduce the traceback on your end? If yes, in which directory? Please also add a print statement like this just before the failing statement (c:\tests\lib\site-packages\ftputil\stat.py, line 186):
    print "=== %r %r %r %r %r" % (year, month, day, hour, minute)
    
    and send me the output when calling listdir.
  • Would you please try the dir command in an ftp command line client (Win 7 hopefully has one by default) and put the output in this ticket? Do you see differences to the output on my side?
  • Did anyone make changes to this ftputil installation (perhaps to debug some other problem) before the traceback showed up?
  • Is there anything else you or me could try to find out more about the problem?
Last edited 4 years ago by schwa (previous) (diff)

comment:2 Changed 4 years ago by schwa

One more idea: Could you install a new Python 2 or Python 3 version and see if you get a traceback there, too? You may want to wait with this until after all the other ideas failed to help. ;-)

comment:3 follow-up: Changed 4 years ago by ftputiluser

The dir looks good with an ftp client for me (windows, linux or cygwin):

I added some debug print statements:

>>> import ftputil
>>> h2=ftputil.FTPHost("mirrors.ibiblio.org", "anonymous", "me@anonymous.com")
>>> h2.listdir(h2.curdir)
parse_line(u'drwxr-xr-x   39 0        0            4096 Apr 14 10:14 .', 0.0):
else: ==>  (2014, 4, 14, 10, 14, 0, 0, 0, -1)
parse_line(u'drwxr-xr-x   39 0        0            4096 Apr 14 10:14 ..', 0.0):
else: ==>  (2014, 4, 14, 10, 14, 0, 0, 0, -1)
parse_line(u'----------    1 0        0               0 Dec 31  1969 .bplusvtoc_internal', 0.0):
":" not in year_or_time: ==>  (1969, 12, 31, 0, 0, 0, 0, 0, -1)

And printing actual lines received from

        lines = self._host_dir(path)

in ftputil.stat._Stat._stat_results_from_dir you get:

Python 2.6.6 (r266:84297, Aug 24 2010, 18:46:32) [MSC v.1500 32 bit (Intel)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import ftputil
>>> h2=ftputil.FTPHost("mirrors.ibiblio.org", "anonymous", "me@anonymous.com")
>>> h2.listdir(h2.curdir)
[u'drwxr-xr-x   39 0        0            4096 Apr 14 10:14 .',
 u'drwxr-xr-x   39 0        0            4096 Apr 14 10:14 ..',
 u'----------    1 0        0               0 Dec 31  1969 .bplusvtoc_internal',
 u'drwxrwxrwx    2 0        0            4096 Jan 20  2011 .snapshot',
 u'----------    1 0        0               0 Dec 31  1969 .vtoc_internal',
 u'drwxrwxr-x   12 2557     2557         4096 Jun 16 22:16 CPAN',
 u'drwxrwsr-x   10 2557     2557         8192 Jun 17 04:09 CRAN',
 u'drwxr-xr-x   17 2557     2557         4096 Jun 17 02:02 CTAN',
 u'drwxr-xr-x    5 2098     100          4096 Sep 18  2010 abiclipart',
 u'drwxrwxr-x  162 2557     2557        16384 Jun 08 14:08 apache',
 u'drwxr-xr-x    5 2557     2557         4096 Nov 29  2004 beonex',
 u'drwxr-xr-x    4 2557     2557         4096 Sep 01  2011 blastwave',
 u'drwx------    3 2449     100          4096 Jan 19  2011 designerunits',
 u'lrwxrwxrwx    1 0        0              28 Apr 29  2011 directory_list -> /var/www/html/directory_list',
 u'drwxr-xr-x  189 2557     2557        16384 Jun 17 03:45 eclipse',
 u'drwxr-xr-x    4 2433     20120        4096 Dec 02  2003 flightgear',
 u'drwxr-xr-x    5 2557     2557         4096 Sep 10  2010 flock',
 u'drwxr-xr-x    6 0        0            4096 Mar 11  2012 freetds',
 u'drwxr-xr-x    4 2557     2557         4096 Nov 30  2007 ggzgamingzone',
 u'drwxr-xr-x    4 1941     20127        4096 Mar 08  2007 gnu',
 u'drwxr-xr-x   71 2557     2557       208896 Jun 15 00:22 grass',
 u'drwxr-xr-x    3 2557     2557         4096 May 08  2010 haiku',
 u'drwxrwxr-x   25 2557     2557         4096 Mar 14 03:05 interactive-fiction',
 u'drwxr-xr-x    3 2557     2557         4096 Apr 27  2012 jpackage',
 u'drwxrwxr-x 1097 1992     20038       90112 Feb 07  2006 maven',
 u'drwxrwxr-x  763 1992     20038       61440 Mar 22 03:00 maven2',
 u'drwxr-xr-x 1884 2557     2557       147456 Aug 25  2013 mozdev.org',
 u'drwxr-xr-x    5 2557     2557         4096 Mar 06 11:38 mozilla',
 u'drwxr-xr-x    8 2557     2557         4096 Apr 04 05:48 opencsw',
 u'drwxr-xr-x    4 2557     2557         4096 Dec 20  2012 openssl',
 u'drwxr-xr-x    6 2504     100          4096 Nov 28  2010 ostalks',
 u'drwxr-xr-x    4 2557     2557         4096 Apr 03 19:54 ovirt',
 u'drwxr-xr-x    2 0        0            4096 Jan 04  2011 pub',
 u'-rw-r--r--    1 0        0              39 Mar 22  2012 robots.txt',
 u'drwxrwsr-x    9 2557     2557         4096 May 26 18:53 rsync',
 u'drwxrwxr-x    8 2557     2557         4096 Aug 04  2013 ruby',
 u'drwxr-xr-x    3 2433     0            4096 Feb 23  2010 simgear',
 u'drwxr-xr-x   27 0        0            4096 Apr 08  2009 speakers',
 u'drwxrwsr-x    7 2557     2557         4096 May 30  2013 sugar',
 u'drwxr-xr-x    3 2557     0            4096 Aug 08  2013 tdf',
 u'drwxr-xr-x    2 2388     0            4096 Apr 27  2008 vinismo',
 u'drwxr-xr-x    3 2557     2557         4096 May 14  2012 wine',
 u'drwxrwsr-x   16 2557     2557         4096 May 30 16:06 xemacs']
parse_line(u'drwxr-xr-x   39 0        0            4096 Apr 14 10:14 .', 0.0):
else: ==>  (2014, 4, 14, 10, 14, 0, 0, 0, -1)
parse_line(u'drwxr-xr-x   39 0        0            4096 Apr 14 10:14 ..', 0.0):
else: ==>  (2014, 4, 14, 10, 14, 0, 0, 0, -1)
parse_line(u'----------    1 0        0               0 Dec 31  1969 .bplusvtoc_internal', 0.0):
":" not in year_or_time: ==>  (1969, 12, 31, 0, 0, 0, 0, 0, -1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "c:\tests\lib\site-packages\ftputil\host.py", line 813, in listdir
    items = self._stat._listdir(path)
  File "c:\tests\lib\site-packages\ftputil\stat.py", line 645, in _listdir
    return self.__call_with_parser_retry(self._real_listdir, path)
  File "c:\tests\lib\site-packages\ftputil\stat.py", line 621, in __call_with_parser_retry
    result = method(*args, **kwargs)
  File "c:\tests\lib\site-packages\ftputil\stat.py", line 505, in _real_listdir
    for stat_result in self._stat_results_from_dir(path):
  File "c:\tests\lib\site-packages\ftputil\stat.py", line 478, in _stat_results_from_dir
    self._host.time_shift())
  File "c:\tests\lib\site-packages\ftputil\stat.py", line 339, in parse_line
    st_mtime, st_mtime_precision = (self.parse_unix_time(month, day, year_or_time, time_shift, with_precision=True))
  File "c:\tests\lib\site-packages\ftputil\stat.py", line 187, in parse_unix_time
    st_mtime = time.mktime((year, month, day, hour, minute, 0, 0, 0, -1))
OverflowError: mktime argument out of range

Your code chokes on the line with ------- perms and 1969 date. You should IMHO be more defensive when calling mktime and always capture possible exceptions. Also possibly if the time looks like being before the epoch, you could massage it ... or something else, but not error out.

comment:4 Changed 4 years ago by ftputiluser

FYI, this works fine on Linux and Python 2.6. This seems to be only an issue on Windows Py26 and Py27 and that ibiblio.org site, choking on these dot files with a time stamp from before the epoch....

comment:5 in reply to: ↑ 3 ; follow-up: Changed 4 years ago by schwa

Replying to ftputiluser:

Your code chokes on the line with ------- perms

For the record, the mode "----------" never has been a problem, but now I added a unit test containing this special case.

and 1969 date. You should IMHO be more defensive when calling mktime and always capture possible exceptions.

The year (or more specifically the time as a whole when taking time zone differences into account) is indeed the culprit here. I was able to try out different values on a Windows 7 machine today. The potential problem with certain input values is also mentioned in the documentation of the time module:

time.mktime(t)

[...]

If the input value cannot be represented as a valid time, either OverflowError or ValueError will be raised (which depends on whether the invalid value is caught by Python or the underlying C libraries). The earliest date for which it can generate a time is platform-dependent.

Also possibly if the time looks like being before the epoch, you could massage it ... or something else, but not error out.

I feel a need to discuss this a bit. :-)

ftputil is supposed to raise a ParserError if a directory line can't be parsed. ftputil expects a certain format once it has chosen one of the two possible parsers. An obvious example for an invalid line is if a field (for instance the mode for unix-format directories) is missing completely. Then there's the possibility that the "general" format is correct but some detail is wrong, for example if the mode part contains an unexpected letter like in "y---------". So what is invalid is a matter of definition and in this sense a year 1969 could be considered invalid. Similarly, mktime may consider such a date invalid by raising an OverflowError or a ValueError.

With this in mind, refusing the date with 1969 as the year part could be considered a valid handling of this situation. In this case there's still the problem that ftputil should raise a ParserError instead of letting the exception raised by mktime through. So the "minimum" change for me would be to fix the type of the raised exception. In addition I'd add to the documentation that the timestamps that can be parsed depend on the time.mktime implementation for the client platform.

Then there's the possibility to "massage" the timestamp data, as you call it. If I understand your point correctly, this could mean pretending that the year is 1970 instead of anything lower. I hesitate a bit to use a "wrong" year, but I guess for most uses this would be more useful than raising a ParserError.

"Something else" could involve:

  • Using None for the timestamp value if the underlying data can't be processed by time.mktime. This is similar to setting the last access time to None since it can't be parsed from the directory listing. (The timestamp from the listing is assumed to be time of the last modification.) Although this approach would be more "honest" it might confuse clients much more than using an "invented" timestamp value. Also, the methods upload_if_newer and download_if_newer would need to be changed to handle these None values.
  • Complement/replace the mktime call with own code that calculates a "reasonable" value. Indeed, the mktime implementation on my Linux machine gives negative floating point values if the timestamp is lower than the that of the Epoch:
    Python 3.3.2 (default, Nov  8 2013, 13:38:57) 
    [GCC 4.8.2 20131017 (Red Hat 4.8.2-1)] on linux
    Type "help", "copyright", "credits" or "license" for more information.
    >>> import time
    >>> time.mktime((1969, 1, 1, 0, 0, 0, 0, 0, -1))
    -31539600.0
    
    On a unix machine I tested today, year values below (I think) 1900 cause a ValueError whereas on my local machine, even negative year values are accepted.

At this point I might decide to use mktime and in case of an exception set the timestamp float value to 0.0. Unfortunately, what leads to a ValueError or OverflowError is platform-specific, so if I don't want to write my own version of mktime, I can only assume that using 0.0 is a reasonable workaround although the exception might mean something completely different, for example using a value of 32 for the day.

Please give me a few days to think about it, but feel free to give feedback on the discussion above. :-)

Last edited 4 years ago by schwa (previous) (diff)

comment:6 Changed 4 years ago by schwa

Another approach could be to provide more "hooks" for user-supplied parsers. So a user could inherit from UnixParser, override the hook's default implementation and set this parser with FTPHost.set_parser.

A variant of this is already possible by overriding the method ignores_line and effectively "filter out" lines with a year of 1969 and below. However, it's currently not possible to modify the line to have it afterward processed by the default implementation of parse_line.

At the moment, I can imagine extracting the time.mktime call into it's own method that can be overridden by the user. This method would take the tuple passed to time.mktime in the current code. With this approach, an ftputil user could decide which heuristic he/she considers most appropriate if the default of calling time.mktime isn't enough.

Last edited 4 years ago by schwa (previous) (diff)

comment:7 follow-up: Changed 4 years ago by ftputiluser

ok, so the culprit is mktime and its variations.

Have you considered using dateutil?

Using the string returned by the ibiblio server:

>>> import dateutil
>>> a='Dec 31  1969'
>>> from dateutil.parser import *
>>> p=parse(a)
>>> p
datetime.datetime(1969, 12, 31, 0, 0)

Note that I checked pyfilesystem and ftpretty ways: they are rather simplistic and do not go the extra mile to get an ls-al like listing, so the . files you report are never seen by them and they never see the issue either ....

I will be happy to submit patches too btw, I have your hg clone handy ;)

Danke sehr! for the quick replies.

--

Philippe

Last edited 4 years ago by ftputiluser (previous) (diff)

comment:8 Changed 4 years ago by ftputiluser

one approach could be to keep your current mktime ways and catch exceptions, then attempt to use dateutil, and if it fails return some empty date as a last resort either a float or a well known impossible date like 1900-1-1 00:00 for these rare cases where everything falls part... as long as it is documented this can be caught easily.

Last edited 4 years ago by ftputiluser (previous) (diff)

comment:9 follow-up: Changed 4 years ago by ftputiluser

comment:10 follow-up: Changed 4 years ago by ftputiluser

I really like your code which is super clean and I have a big need for a reliable and feature rich ftp client... so I am motivated!

comment:11 in reply to: ↑ 5 ; follow-up: Changed 4 years ago by ftputiluser

Replying to schwa:

At this point I might decide to use mktime and in case of an exception set the timestamp float value to 0.0. Unfortunately, what leads to a ValueError or OverflowError is platform-specific, so if I don't want to write my own version of mktime, I can only assume that using 0.0 is a reasonable workaround although the exception might mean something completely different, for example using a value of 32 for the day.

Please give me a few days to think about it, but feel free to give feedback on the discussion above. :-)

FWIW, this is what I did in my patch: if the date cannot be parsed then return the epoch, which 0.0 and eventually OS dependent. That said an alternative could be to use datetime and not mktime. Datetime years in contrast with mktime can go from 1 to 9999. But in earnest an FTP file with a date that pre-dates the epoch just does not make sense. Your call and your decision. Though it does not make sense to me to have that as a user hook feature. Not worth it and too rare.

comment:12 in reply to: ↑ 7 Changed 4 years ago by schwa

Replying to ftputiluser:

Have you considered using dateutil?

I don't remember whether I had heard of it, I think I hadn't.

Since its first release, ftputil hasn't had any dependencies apart from the Python standard library, and I think that's a good thing. Not everyone can or is allowed to install arbitrary packages on the system. Of course, for local development you can use virtual environments, but for some "real" deployment situations this isn't possible.

dateutil wouldn't only be a dependency outside the Python standard library. It also itself has a dependency on six.

Note that I checked pyfilesystem and ftpretty ways: they are rather simplistic and do not go the extra mile to get an ls-al like listing, so the . files you report are never seen by them and they never see the issue either ....

I must admit I hadn't heard of pyfilesystem. From time to time I check PyPI for alternative FTP client libraries, but pyfilesystem isn't listed there it seems. (I was about to write that ftpretty is rather new on PyPI, but then noticed that old releases don't seem to be listed anymore on PyPI.)

As it happens, including "hidden" files is a quite recent addition to ftputil although the library is over 10 years old. :-)

I will be happy to submit patches too btw, I have your hg clone handy ;)

Danke sehr! for the quick replies.

You're welcome! :-)

comment:13 in reply to: ↑ 9 ; follow-up: Changed 4 years ago by schwa

Replying to ftputiluser:

See https://bitbucket.org/sschwarzer/ftputil/pull-request/2/a-fix-for-ticket-83/diff for a simple suggested fix returning 0.0.

Thanks for the patch, especially since you thought of adding unit tests! :-)

I'm not yet sure how to solve the issue, but I'm quite sure I won't take the changes without modifications. I need to handle ValueErrors because of other reasons than "year underflow", and I would move the method to the base class for reuse to avoid code duplication.

comment:14 in reply to: ↑ 10 Changed 4 years ago by schwa

Replying to ftputiluser:

I really like your code which is super clean and I have a big need for a reliable and feature rich ftp client... so I am motivated!

I'm very glad about the compliment. Thank you!

comment:15 in reply to: ↑ 13 Changed 4 years ago by ftputiluser

Replying to schwa:

Thanks for the patch, especially since you thought of adding unit tests! :-)

A patch without a test would be like a chicken without a head ... eventually tasty but not alive.

I'm not yet sure how to solve the issue, but I'm quite sure I won't take the changes without modifications. I need to handle ValueErrors because of other reasons than "year underflow", and I would move the method to the base class for reuse to avoid code duplication.

Of course, this was quick hack on my side, just to ensure that things could work at least minimally

Note that as I suggested using a datetime object would work but then the issue is that you lose mktime compatibility and the whole stat-like compatibility , all in all being a bad solution.

So IMHO after giving it a good thought, when a date is before the epoch then use 0.0 which will be the epoch of the OS. OR you could force that to be the unix epoch on all OSes such that the same results would be returned consistently on all OSes.

comment:16 in reply to: ↑ 11 ; follow-up: Changed 4 years ago by schwa

(You're too fast. I had to temporarily save my reply in an editor and copy it back because you replied while I was writing. ;-) )

Replying to ftputiluser:

FWIW, this is what I did in my patch: if the date cannot be parsed then return the epoch, which 0.0 and eventually OS dependent. That said an alternative could be to use datetime and not mktime. Datetime years in contrast with mktime can go from 1 to 9999.

In the end I need a "seconds since the Epoch" value to put in the StatResult tuple to be compatible with the os.stat result. When I just looked I didn't see a way to convert a datetime object to the float value.

But in earnest an FTP file with a date that pre-dates the epoch just does not make sense.

Possibly the timestamp on the server's filesystem actually was 0.0, maybe a kind of "arbitrary" or "base" value. But the server converted this to its local time which resulted in the year 1969.

Though it does not make sense to me to have that as a user hook feature. Not worth it and too rare.

Probably I'll need to factor out the mktime functionality anyway. I think I'll do an implicit check of the values going into mktime with the datetime constructor and if I "still" get an OverflowError or ValueError, I use 0.0 as a sentinel. I agree that this is probably good enough for almost all use cases, so I guess I would only officially open the mktime API if the sentinel approach isn't enough.

comment:17 in reply to: ↑ 16 ; follow-up: Changed 4 years ago by schwa

Replying to schwa:

Probably I'll need to factor out the mktime functionality anyway. I think I'll do an implicit check of the values going into mktime with the datetime constructor and if I "still" get an OverflowError or ValueError, I use 0.0 as a sentinel. I agree that this is probably good enough for almost all use cases, so I guess I would only officially open the mktime API if the sentinel approach isn't enough.

So, as a sketch:

def _mktime(self, mktime_tuple):
    try:
        # Only for sanity checks, not interested in the return
        # value.
        datetime.datetime(*mktime_tuple[:6])
    except ValueError:
        # For example, day == 35. Not all implementations of
        # `mktime` catch this kind of error.
        raise ParserError(...)
    try:
        return time.mktime(mktime_tuple)
    except (OverflowError, ValueError):
        # Sentinel for times before the Epoch, see ticket #83.
        return 0.0
Last edited 4 years ago by schwa (previous) (diff)

comment:18 in reply to: ↑ 17 Changed 4 years ago by ftputiluser

Replying to schwa:

So, as a sketch: [...]

Looks all good to me

comment:19 Changed 4 years ago by schwa

  • Milestone set to 3.2
  • Resolution set to fixed
  • Status changed from assigned to closed

This should be fixed now (stretched out over several commits).

If the directory listing indicates a datetime before the epoch, ftputil will set the datetime for the last modification to 0.0.

Invalid values (like a day value of 32) now raise a ParserError.

Note: See TracTickets for help on using tickets.