Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8255078: sun/net/ftp/imp/FtpClient$MLSxParser uses wrong datetime format #776

Closed
wants to merge 3 commits into from

Conversation

iignatev
Copy link
Member

@iignatev iignatev commented Oct 20, 2020

Hi all,

could you please review this small patch?

according to RFC3659, time values in MLSx response have the following format:

time-val = 14DIGIT [ "." 1*DIGIT ]

The leading, mandatory, fourteen digits are to be interpreted as, in
order from the leftmost, four digits giving the year, with a range of
1000--9999, two digits giving the month of the year, with a range of
01--12, two digits giving the day of the month, with a range of
01--31, two digits giving the hour of the day, with a range of
00--23, two digits giving minutes past the hour, with a range of
00--59, and finally, two digits giving seconds past the minute, with
a range of 00--60 (with 60 being used only at a leap second). Years
in the tenth century, and earlier, cannot be expressed. This is not
considered a serious defect of the protocol.

The optional digits, which are preceded by a period, give decimal
fractions of a second. These may be given to whatever precision is
appropriate to the circumstance, however implementations MUST NOT add
precision to time-vals where that precision does not exist in the
underlying value being transmitted.

Symbolically, a time-val may be viewed as

YYYYMMDDHHMMSS.sss

The "." and subsequent digits ("sss") are optional. However the "."
MUST NOT appear unless at least one following digit also appears.

MLSxParser, however, uses SimpleDateFormat("yyyyMMddhhmmss") (which is wrong b/c it uses hh instead of HH and doesn't account for optional .sss) to parse modify/create facts and ignore any parse exceptions.

FtpClient actually already has and uses SimpleDateFormat w/ right formats in getLastModified where it parses MDTM response, the patch refactors the code to reuse the same SimpleDateFormat in MLSxParser.

testing:

  • tier1
  • test/jdk/sun/net on {linux,windows,macosx}-x64

Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Testing

Linux x32 Linux x64 Windows x64 macOS x64
Build ✔️ (1/1 passed) ✔️ (5/5 passed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) ✔️ (9/9 passed) (1/9 failed) ✔️ (9/9 passed)

Failed test task

Issue

  • JDK-8255078: sun/net/ftp/imp/FtpClient$MLSxParser uses wrong datetime format

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/776/head:pull/776
$ git checkout pull/776

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 20, 2020

👋 Welcome back iignatyev! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Oct 20, 2020

@iignatev The following label will be automatically applied to this pull request:

  • net

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the net net-dev@openjdk.org label Oct 20, 2020
@iignatev iignatev marked this pull request as ready for review Oct 20, 2020
@openjdk openjdk bot added the rfr Pull request is ready for review label Oct 20, 2020
@mlbridge
Copy link

mlbridge bot commented Oct 20, 2020

Webrevs

@dfuch
Copy link
Member

dfuch commented Oct 22, 2020

Hi Igor, is this testable - and if so shouldn't there be a test?
best regards,
-- daniel

@iignatev
Copy link
Member Author

iignatev commented Oct 22, 2020

Hi Igor, is this testable - and if so shouldn't there be a test?
best regards,
-- daniel

Hi Daniel,

it's testable and originally I planned to add a new test, however upon checking the existing mock ftp-servers, I realized that none of them support MLSx commands, and adding that support doesn't seem to be justified for such a trivial fix. With that being said, I can create a test for this if you believe it's necessary.

-- Igor

@iignatev
Copy link
Member Author

iignatev commented Oct 23, 2020

I took another stab on the test and it turned out to be easier than I originally anticipated. I, however, decided to take a safer approach and don't use FtpServerfromtest/jdk/sun/net/www/ftptesttest-library, and instead introduced a test-specific mock server in the test code. I'm running the test multiple times on{windows,linux,macosx}-x64` to check how stable it's in our environment, so far there are no failures.

-- Igor

dfuch
dfuch approved these changes Oct 23, 2020
Copy link
Member

@dfuch dfuch left a comment

Thanks for adding a test Igor!
If that passes on all platform then this looks good to me.

@openjdk
Copy link

openjdk bot commented Oct 23, 2020

@iignatev This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8255078: sun/net/ftp/imp/FtpClient$MLSxParser uses wrong datetime format

Reviewed-by: dfuchs

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 88 new commits pushed to the master branch:

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Oct 23, 2020
@iignatev
Copy link
Member Author

iignatev commented Oct 23, 2020

Thanks, Daniel.

/integrate

@openjdk openjdk bot closed this Oct 23, 2020
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 23, 2020
@openjdk
Copy link

openjdk bot commented Oct 23, 2020

@iignatev Since your change was applied there have been 88 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

Pushed as commit 6545e19.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

}
return null;
}

private static Date parseRfc3659TimeValue(String s) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this method be synchronized because SimpleDateFormat is not thread-safe, but instances are stored in the static field dateFormats?
(Note though that this issue existed before as well)

Copy link
Member Author

@iignatev iignatev Oct 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @Marcono1234, it indeed needs to be synchronized, I'll take care of fixing that.

Copy link
Member

@dfuch dfuch Oct 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iignatev if you decide to log a followup a better alternative might be to use DateTimeFormatter

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated net net-dev@openjdk.org
3 participants