UnixFTPEntryParser returns null when timestamp is between 12:00am and 12:59am

• Key: NET-72
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: Unassigned
• Reporter: Kevin Flynn

Environment

Operating System: All
Platform: All

Description

This concerns commons.net version 1.2.2.

When I request a ftp listing from a directory with a file having a timestamp
between 12:00am and 12:59am on a Microsoft FTP Service (Version 5.0) box, the
FTPClient listFiles() method returns null for that file. Here is an example
directory listing:

-rwxrwxrwx 1 owner group 115407014 Aug 12 19:33 images_20040812.zip
-rwxrwxrwx 1 owner group 144408943 Aug 13 2:03 images_20040813.zip
-rwxrwxrwx 1 owner group 146215270 Aug 14 10:29 images_20040814.zip
-rwxrwxrwx 1 owner group 144822990 Aug 17 6:00 images_20040817.zip
-rwxrwxrwx 1 owner group 22 Aug 18 0:36 test.zip

The entry from listFiles() that corresponding text.zip will be null.

I tracked down the problem to the REGEX pattern in UnixFTPEntryParser. If I
change the expression to the following, I can get the FTPFile from the
listFiles() method.

private static final String REGEX =
"([bcdlfmpSs-])"
+ "(((r|)(w|)([xsStTL-]))((r|)(w|)([xsStTL-]))((r|)(w|)([xsStTL-])))
s+"
+ "(\\d+)
s+"
+ "(\\S+)
s+"
+ "(?\\S+)
s+)?"
+ "(\\d+)
s+"
+ MONTHS + "
s+"
+ "((?:[0-9])|(?:[0-2][0-9])|(?:3[0-1]))
s+"
// Following line changed
+ "((\\d\\d\\d\\d)|((?:[01]\\d)|(?:2[0123])|(?:[0-9])):([012345]\\d))
s+"
// Original line
//+ "((\\d\\d\\d\\d)|((?:[01]\\d)|(?:2[0123])|(?:[1-9])):([012345]\\d))
s+"
+ "(\\S+)(
s*.*)";

Comments

[Jürgen Weber]

I ran into this bug too with an ftp server on W2K German with

-rwxrwxrwx 1 owner group 257112 Jun 27 0:45 testalbum.zip
drwxrwxrwx 1 owner group 0 Aug 21 2002 testdir
drwxrwxrwx 1 owner group 0 May 28 0:39 tmp

testalbum.zip and tmp will result in null.

I see two problems here:

o the regex matcher will fail if there is only one field wrong. Maybe there
should be a regex call for each field, so could at least fill the correctly
parsed fields.
o in case of a failed match there should not be returned null but rather an
empty FTPFile. There was a real ftp server output line, so no need to have it
nulled. If there is a parse error, it's not the fault of the server but a bug in
the regex.

[Steve Cohen]

Adopted your regex fix, in a slightly different form.

On the other hand, I disagree that failure to parse should not return null. This helps us to find
bugs in the regex! The goal is a bug-free regex that works with all variations of the format.
Since the format is not tightly defined anywhere, implementers are somewhat free (I would say
too free) to come up with slightly different variations. In most other servers 0:39 was rendered
as 00:39 which is why we never saw this problem before.

[Jürgen Weber]

Returning null for a non-parsed entry is neither graceful degradation nor a
clear indication of the problem. The API of FTPClient.listFiles() does not
mention that it can return null entries, so it is not obvious why there are
holes in the returned array and why you run into NPEs.

I think you should either return a FTPFile where at least getRawListing() works
(with maybe an additional method getParseStatus() ) or you should state loud and
clear that there was a problem by throwing a ParserException.

[Steve Cohen]

You raise an interesting point. I still don't agree with it. While I see merit in your claim that
having arrays with null members is not a good thing, there are similar problems also inherent in
"incomplete" objects. The uninformed user may just as well stumble on NPEs when
dereferencing a non-existent date in a partially complete FTPFile, for example. The null
member, while obviously not a perfect solution, does announce itself reasonably loudly, is
easily detectable by the user, and that has value. Throwing an exception over the whole list
might be too draconian a penalty for one unparsed listing out of a hundred. Thus, the current
implementation is a reasonable compromise, I think.

Our goal remains to have parsers that handle every conceivable case, even if that is almost
impossible to achieve given the wide array of implementations out there. We are getting there,
asymptotically.

Your point about the return value of listFiles() not being documented to warn the user of this
possibility is a valid one and I have now corrected it, both in FTPClient.listFiles() and in the
mehtods of FTPListParseEngine on which it depends.

[Steve Cohen]

An additional point here is backward compatibility. This API is by now several years old.

Much code has been written based on the twin assumptions that:
1. The returned array may contain null members and these must be checked for before
referencng and
2. Any non-null member is a complete FTPFile object.

Changing these assumptions to
1. Every member of the returned array will be non-null but
2. Any member of the array may be incomplete, with member fields that may be null

will break too much existing code to be seriously considered as an alternative now. If this was
the design phase of a new API, your points would obviously need to be taken into
consideration. Undoubtedly, better designs are possible.

[Lars Kral]

• • • *** Bug 31541 has been marked as a duplicate of this bug. *** has been marked as a duplicate of this bug. ***

[Steve Cohen]

• • • COM-1002 has been marked as a duplicate of this bug. ***