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

2020-10-20 Thread Igor Ignatyev 
Hi all,

could you please review this small patch?

according to [RFC3659](https://tools.ietf.org/html/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--, 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
> 
>   MMDDHHMMSS.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("MMddhhmmss")` (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`

-

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

Changes: https://git.openjdk.java.net/jdk/pull/776/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=776&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8255078
  Stats: 50 lines in 1 file changed: 19 ins; 23 del; 8 mod
  Patch: https://git.openjdk.java.net/jdk/pull/776.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/776/head:pull/776

PR: https://git.openjdk.java.net/jdk/pull/776

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

2020-10-22 Thread Igor Ignatyev 
On Thu, 22 Oct 2020 10:33:06 GMT, Daniel Fuchs  wrote:

> 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

-

PR: https://git.openjdk.java.net/jdk/pull/776

Re: RFR: 8255078: sun/net/ftp/imp/FtpClient$MLSxParser uses wrong datetime format [v2]

2020-10-23 Thread Igor Ignatyev 
> Hi all,
> 
> could you please review this small patch?
> 
> according to [RFC3659](https://tools.ietf.org/html/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--, 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 
>> 
>>   MMDDHHMMSS.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("MMddhhmmss")` (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:
> * [x] tier1
> * [x] `test/jdk/sun/net` on `{linux,windows,macosx}-x64`

Igor Ignatyev has updated the pull request incrementally with one additional 
commit since the last revision:

  added test

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/776/files
  - new: https://git.openjdk.java.net/jdk/pull/776/files/a2867539..7a1c0505

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=776&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=776&range=00-01

  Stats: 192 lines in 1 file changed: 192 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/776.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/776/head:pull/776

PR: https://git.openjdk.java.net/jdk/pull/776

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

2020-10-23 Thread Igor Ignatyev 
On Thu, 22 Oct 2020 16:04:56 GMT, Igor Ignatyev  wrote:

>> Hi Igor, is this testable - and if so shouldn't there be a test?
>> best regards,
>> -- daniel
>
>> 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

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 FtpServer` from `test/jdk/sun/net/www/ftptest` test-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

-

PR: https://git.openjdk.java.net/jdk/pull/776

Re: RFR: 8255078: sun/net/ftp/imp/FtpClient$MLSxParser uses wrong datetime format [v3]

2020-10-23 Thread Igor Ignatyev 
> Hi all,
> 
> could you please review this small patch?
> 
> according to [RFC3659](https://tools.ietf.org/html/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--, 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 
>> 
>>   MMDDHHMMSS.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("MMddhhmmss")` (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:
> * [x] tier1
> * [x] `test/jdk/sun/net` on `{linux,windows,macosx}-x64`

Igor Ignatyev has updated the pull request incrementally with one additional 
commit since the last revision:

  use only filename in assert message

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/776/files
  - new: https://git.openjdk.java.net/jdk/pull/776/files/7a1c0505..9b69cf91

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=776&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=776&range=01-02

  Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/776.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/776/head:pull/776

PR: https://git.openjdk.java.net/jdk/pull/776

Re: RFR: 8255078: sun/net/ftp/imp/FtpClient$MLSxParser uses wrong datetime format [v3]

2020-10-23 Thread Igor Ignatyev 
On Fri, 23 Oct 2020 18:05:07 GMT, Daniel Fuchs  wrote:

>> Igor Ignatyev has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   use only filename in assert message
>
> Thanks for adding a test Igor!
> If that passes on all platform then this looks good to me.

Thanks, Daniel.

-

PR: https://git.openjdk.java.net/jdk/pull/776

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

2020-10-23 Thread Igor Ignatyev 
On Tue, 20 Oct 2020 23:14:40 GMT, Igor Ignatyev  wrote:

> Hi all,
> 
> could you please review this small patch?
> 
> according to [RFC3659](https://tools.ietf.org/html/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--, 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 
>> 
>>   MMDDHHMMSS.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("MMddhhmmss")` (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:
> * [x] tier1
> * [x] `test/jdk/sun/net` on `{linux,windows,macosx}-x64`

This pull request has now been integrated.

Changeset: 6545e19f
Author:Igor Ignatyev 
URL:   https://git.openjdk.java.net/jdk/commit/6545e19f
Stats: 242 lines in 2 files changed: 211 ins; 23 del; 8 mod

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

Reviewed-by: dfuchs

-

PR: https://git.openjdk.java.net/jdk/pull/776

Re: RFR: 8255078: sun/net/ftp/imp/FtpClient$MLSxParser uses wrong datetime format [v3]

2020-10-26 Thread Igor Ignatyev 
On Sun, 25 Oct 2020 00:35:20 GMT, Marcono1234 
 wrote:

>> Igor Ignatyev has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   use only filename in assert message
>
> src/java.base/share/classes/sun/net/ftp/impl/FtpClient.java line 1777:
> 
>> 1775: }
>> 1776: 
>> 1777: private static Date parseRfc3659TimeValue(String s) {
> 
> 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)

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

-

PR: https://git.openjdk.java.net/jdk/pull/776

RFR: 8255405: sun/net/ftp/imp/FtpClient uses SimpleDateFormat in not thread-safe manner

2020-10-26 Thread Igor Ignatyev 
Hi all,

could you please review this small and trivial fix?

`sun/net/ftp/imp/FtpClient::dateFormats` is an array of `SimpleDateFormat` 
which are shared among all instances of `FtpClient`. the fact that 
`SimpleDateFormat` isn't thread-safe renders`FtpClient` to be non-thread-safe 
as well. the patch makes the only usage of `dateFormats` array, 
`parseRfc3659TimeValue` method, `synchronized`.

the problem was reported in #776

Thanks,
-- Igor

-

Commit messages:
 - 8255405: sun/net/ftp/imp/FtpClient uses SimpleDateFormat in not thread-safe 
manner

Changes: https://git.openjdk.java.net/jdk/pull/867/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=867&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8255405
  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/867.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/867/head:pull/867

PR: https://git.openjdk.java.net/jdk/pull/867

Re: RFR: 8255405: sun/net/ftp/imp/FtpClient uses SimpleDateFormat in not thread-safe manner [v2]

2020-10-27 Thread Igor Ignatyev 
> Hi all,
> 
> could you please review this small and trivial fix?
> 
> `sun/net/ftp/imp/FtpClient::dateFormats` is an array of `SimpleDateFormat` 
> which are shared among all instances of `FtpClient`. the fact that 
> `SimpleDateFormat` isn't thread-safe renders`FtpClient` to be non-thread-safe 
> as well. the patch makes the only usage of `dateFormats` array, 
> `parseRfc3659TimeValue` method, `synchronized`.
> 
> the problem was reported in #776
> 
> Thanks,
> -- Igor

Igor Ignatyev has updated the pull request incrementally with three additional 
commits since the last revision:

 - remove \n from MDTM response before parsing
 - added leading 0 to the test
 - use DateTimeFormatter instead of SimpleDateFormat

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/867/files
  - new: https://git.openjdk.java.net/jdk/pull/867/files/f684a9ce..a8e6d809

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=867&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=867&range=00-01

  Stats: 35 lines in 2 files changed: 2 ins; 17 del; 16 mod
  Patch: https://git.openjdk.java.net/jdk/pull/867.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/867/head:pull/867

PR: https://git.openjdk.java.net/jdk/pull/867

Re: RFR: 8255405: sun/net/ftp/imp/FtpClient uses SimpleDateFormat in not thread-safe manner

2020-10-27 Thread Igor Ignatyev 
On Tue, 27 Oct 2020 12:48:58 GMT, Chris Hegarty  wrote:

>> Hi all,
>> 
>> could you please review this small and trivial fix?
>> 
>> `sun/net/ftp/imp/FtpClient::dateFormats` is an array of `SimpleDateFormat` 
>> which are shared among all instances of `FtpClient`. the fact that 
>> `SimpleDateFormat` isn't thread-safe renders`FtpClient` to be 
>> non-thread-safe as well. the patch makes the only usage of `dateFormats` 
>> array, `parseRfc3659TimeValue` method, `synchronized`.
>> 
>> the problem was reported in #776
>> 
>> Thanks,
>> -- Igor
>
> @iignatev Please evaluate using DateTimeFormatter, as suggested by @dfuch in 
> https://bugs.openjdk.java.net/browse/JDK-8255405?focusedCommentId=14376774&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14376774

Hi Chris,

I've actually thought about `DateTimeFormatter` when I was fixing 
[8255078](https://bugs.openjdk.java.net/browse/JDK-8255078) / #776, but when I 
noticed `MDTMformats` and decided to reuse it. anyhow, I have updated 
`FtpClient` to use `DateTimeFormatter` instead of `SimpleDateFormat`. testing 
this, I realized that the test added by 8255078 didn't really produce the 
strings in accordance with RFC 3659; so this PR fixes that as well.

Thanks,
-- Igor

-

PR: https://git.openjdk.java.net/jdk/pull/867

Re: RFR: 8255405: sun/net/ftp/imp/FtpClient uses SimpleDateFormat in not thread-safe manner [v3]

2020-10-28 Thread Igor Ignatyev 
> Hi all,
> 
> could you please review this small and trivial fix?
> 
> `sun/net/ftp/imp/FtpClient::dateFormats` is an array of `SimpleDateFormat` 
> which are shared among all instances of `FtpClient`. the fact that 
> `SimpleDateFormat` isn't thread-safe renders`FtpClient` to be non-thread-safe 
> as well. the patch makes the only usage of `dateFormats` array, 
> `parseRfc3659TimeValue` method, `synchronized`.
> 
> the problem was reported in #776
> 
> Thanks,
> -- Igor

Igor Ignatyev has updated the pull request incrementally with two additional 
commits since the last revision:

 - use UTC TZ for RFC3659_DATETIME_FORMAT
 - run TestFtpTimeValue with different user.timezone

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/867/files
  - new: https://git.openjdk.java.net/jdk/pull/867/files/a8e6d809..322e04f3

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=867&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=867&range=01-02

  Stats: 9 lines in 2 files changed: 4 ins; 1 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/867.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/867/head:pull/867

PR: https://git.openjdk.java.net/jdk/pull/867

Integrated: 8255405: sun/net/ftp/imp/FtpClient uses SimpleDateFormat in not thread-safe manner

2020-10-28 Thread Igor Ignatyev 
On Mon, 26 Oct 2020 16:51:25 GMT, Igor Ignatyev  wrote:

> Hi all,
> 
> could you please review this small and trivial fix?
> 
> `sun/net/ftp/imp/FtpClient::dateFormats` is an array of `SimpleDateFormat` 
> which are shared among all instances of `FtpClient`. the fact that 
> `SimpleDateFormat` isn't thread-safe renders`FtpClient` to be non-thread-safe 
> as well. the patch makes the only usage of `dateFormats` array, 
> `parseRfc3659TimeValue` method, `synchronized`.
> 
> the problem was reported in #776
> 
> Thanks,
> -- Igor

This pull request has now been integrated.

Changeset: 7e305ad1
Author:Igor Ignatyev 
URL:   https://git.openjdk.java.net/jdk/commit/7e305ad1
Stats: 37 lines in 2 files changed: 4 ins; 16 del; 17 mod

8255405: sun/net/ftp/imp/FtpClient uses SimpleDateFormat in not thread-safe 
manner

Reviewed-by: chegar, ryadav, dfuchs

-

PR: https://git.openjdk.java.net/jdk/pull/867

Re: RFR: 8255405: sun/net/ftp/imp/FtpClient uses SimpleDateFormat in not thread-safe manner [v2]

2020-10-28 Thread Igor Ignatyev 
On Wed, 28 Oct 2020 12:52:41 GMT, Rahul Yadav  wrote:

>> src/java.base/share/classes/sun/net/ftp/impl/FtpClient.java line 1770:
>> 
>>> 1768: try {
>>> 1769: var d = LocalDateTime.parse(s, RFC3659_DATETIME_FORMAT);
>>> 1770: result = 
>>> Date.from(d.atZone(ZoneOffset.systemDefault()).toInstant());
>> 
>> Should this be ZoneOffset.UTC rather than System default? I thought the date 
>> returned by the server were supposed to be in GMT.
>
> ZoneOffset.UTC should return GMT as was the case with SimpleDateFormat before 
> the changes.

right,  thanks for spotting that! I don't know what I was thinking when used 
`systemDefault` here and when was also got tricked by my own test 🤦

-

PR: https://git.openjdk.java.net/jdk/pull/867

Re: RFR: 8255405: sun/net/ftp/imp/FtpClient uses SimpleDateFormat in not thread-safe manner [v3]

2020-10-28 Thread Igor Ignatyev 
On Wed, 28 Oct 2020 18:07:28 GMT, Daniel Fuchs  wrote:

>> Igor Ignatyev has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - use UTC TZ for RFC3659_DATETIME_FORMAT
>>  - run TestFtpTimeValue with different user.timezone
>
> Marked as reviewed by dfuchs (Reviewer).

Daniel, Rahul, Chris, thank you for your reviews!

-

PR: https://git.openjdk.java.net/jdk/pull/867