Re: RFR: JDK-8243376: java.net.SocketPermission.implies(Permission p) spec is mismatching with implementation [v9]
On Wed, 10 Mar 2021 17:32:30 GMT, Jayashree S Kumar wrote: >> Issue >> >> https://bugs.openjdk.java.net/browse/JDK-8243376 >> >> Problem >> >> The scenario is: >> - Some specified target hostname resolves to two IP addresses (always the >> same address pair). >> - The DNS resolved order of the two ip addresses changes (a usual >> LoadBalancer type behavior). >> - The CNAME of the two ip addresses differ. >> >> In SocketPermission class(void getIP() method), it internally resolves and >> saves only the first IP address resolved, not all the IP addresses resolved. >> - Depending on when the implier/implied SocketPermission hostname is >> resolved, the resolved addresses order differs, and the internally saved IP >> address mismatches, resulting on SocketPermission#implies() false. >> >> >> Michael McMahon kindly reviewed and suggested changes: >> https://mail.openjdk.java.net/pipermail/net-dev/2020-May/014001.html > > Jayashree S Kumar has updated the pull request incrementally with one > additional commit since the last revision: > > Jcheck Review: Corrected the tab error Hi Vyom, Thanks a lot for reaching out. We have reopened to try and write the additional tests in "jdk/java/net/SocketPermission/Equals.java" for equal & hashCode method. - PR: https://git.openjdk.java.net/jdk/pull/1916
Re: RFR: JDK-8243376: java.net.SocketPermission.implies(Permission p) spec is mismatching with implementation [v9]
> Issue > > https://bugs.openjdk.java.net/browse/JDK-8243376 > > Problem > > The scenario is: > - Some specified target hostname resolves to two IP addresses (always the > same address pair). > - The DNS resolved order of the two ip addresses changes (a usual > LoadBalancer type behavior). > - The CNAME of the two ip addresses differ. > > In SocketPermission class(void getIP() method), it internally resolves and > saves only the first IP address resolved, not all the IP addresses resolved. > - Depending on when the implier/implied SocketPermission hostname is > resolved, the resolved addresses order differs, and the internally saved IP > address mismatches, resulting on SocketPermission#implies() false. > > > Michael McMahon kindly reviewed and suggested changes: > https://mail.openjdk.java.net/pipermail/net-dev/2020-May/014001.html Jayashree S Kumar has updated the pull request incrementally with one additional commit since the last revision: Jcheck Review: Corrected the tab error - Changes: - all: https://git.openjdk.java.net/jdk/pull/1916/files - new: https://git.openjdk.java.net/jdk/pull/1916/files/ecc9a36b..fe714efd Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1916&range=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1916&range=07-08 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/1916.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1916/head:pull/1916 PR: https://git.openjdk.java.net/jdk/pull/1916
Re: RFR: JDK-8243376: java.net.SocketPermission.implies(Permission p) spec is mismatching with implementation [v6]
On Fri, 5 Mar 2021 15:08:03 GMT, Michael McMahon wrote:
>> Jayashree S Kumar has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Code Review: Incorporated Indention correction, added getIP and updated
>> try-catch inside for loop
>
> src/java.base/share/classes/java/net/SocketPermission.java line 706:
>
>> 704: for (String cname : cnames) {
>> 705: if (Objects.nonNull(cname) && match(cname, hname)) {
>> 706: return true;
>
> Sorry, forgot to mention, I think I'd prefer just to have:
> if (cname != null && match(..)))
>
> I think Objects.nonNull exists more for use as a method reference.
Done.
> test/jdk/java/net/SocketPermission/SocketPermissionIm.java line 32:
>
>> 30:} while (testPass <= 2);
>> 31: }
>> 32:
>
> Formatting looks off in the file. The do statement in 22 should be aligned
> with SocketPermission in line 120.
>
> The implementation changes look fine.
Corrected the alignment.
> test/jdk/java/net/SocketPermission/SocketPermissionIm.java line 18:
>
>> 16: String hostsFileName = System.getProperty("test.src", ".") +
>> File.separator + "Host.txt";
>> 17: System.setProperty("jdk.net.hosts.file", hostsFileName);
>> 18:
>
> It's better to specify this system property on the command line because (we
> discovered recently) there are some environments where the name service gets
> initialized before main() is called in which case setting the property here
> is too late. So, you need to add a -Djdk.net.hosts.file=${test.src}
Removed the system property and added this as a command-line option.
> test/jdk/java/net/SocketPermission/SocketPermissionIm.java line 6:
>
>> 4: * @summary SocketPermission implies(Permission p) spec allows, If the
>> object was initialized with a single IP address and one of p's IP addresses
>> is equal to this object's IP addr
>> 5: * @run java -Dsun.net.inetaddr.ttl=0 SocketPermissionIm
>> 6: */
>
> Instead of:
> @run java
> you need:
> @run main/othervm
>
> Also see further comment below.
Done
-
PR: https://git.openjdk.java.net/jdk/pull/1916
Re: RFR: JDK-8243376: java.net.SocketPermission.implies(Permission p) spec is mismatching with implementation [v8]
> Issue > > https://bugs.openjdk.java.net/browse/JDK-8243376 > > Problem > > The scenario is: > - Some specified target hostname resolves to two IP addresses (always the > same address pair). > - The DNS resolved order of the two ip addresses changes (a usual > LoadBalancer type behavior). > - The CNAME of the two ip addresses differ. > > In SocketPermission class(void getIP() method), it internally resolves and > saves only the first IP address resolved, not all the IP addresses resolved. > - Depending on when the implier/implied SocketPermission hostname is > resolved, the resolved addresses order differs, and the internally saved IP > address mismatches, resulting on SocketPermission#implies() false. > > > Michael McMahon kindly reviewed and suggested changes: > https://mail.openjdk.java.net/pipermail/net-dev/2020-May/014001.html Jayashree S Kumar has updated the pull request incrementally with one additional commit since the last revision: Code Review: Addressed Michael's changes - Changes: - all: https://git.openjdk.java.net/jdk/pull/1916/files - new: https://git.openjdk.java.net/jdk/pull/1916/files/e845a4ee..ecc9a36b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1916&range=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1916&range=06-07 Stats: 13 lines in 2 files changed: 1 ins; 2 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/1916.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1916/head:pull/1916 PR: https://git.openjdk.java.net/jdk/pull/1916
Re: RFR: JDK-8243376: java.net.SocketPermission.implies(Permission p) spec is mismatching with implementation [v7]
> Issue > > https://bugs.openjdk.java.net/browse/JDK-8243376 > > Problem > > The scenario is: > - Some specified target hostname resolves to two IP addresses (always the > same address pair). > - The DNS resolved order of the two ip addresses changes (a usual > LoadBalancer type behavior). > - The CNAME of the two ip addresses differ. > > In SocketPermission class(void getIP() method), it internally resolves and > saves only the first IP address resolved, not all the IP addresses resolved. > - Depending on when the implier/implied SocketPermission hostname is > resolved, the resolved addresses order differs, and the internally saved IP > address mismatches, resulting on SocketPermission#implies() false. > > > Michael McMahon kindly reviewed and suggested changes: > https://mail.openjdk.java.net/pipermail/net-dev/2020-May/014001.html Jayashree S Kumar has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 13 commits: - Merge branch 'master' of https://github.com/openjdk/jdk into socketperm_implies - Code Review: Incorporated Indention correction, added getIP and updated try-catch inside for loop - Merge branch 'master' of https://github.com/openjdk/jdk into socketperm_implies - Code Review: cname made array accounting for multiple cname values - Merge branch 'master' of https://github.com/openjdk/jdk into socketperm_implies - Incorporated changes suggested by Vyom in testcase - Fixed all Whitespace error in testcase - Fixed all Whitespace error in testcase - Fixed automerge failed conflicts - Correct WhiteSpace error - ... and 3 more: https://git.openjdk.java.net/jdk/compare/4b5be40a...e845a4ee - Changes: https://git.openjdk.java.net/jdk/pull/1916/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1916&range=06 Stats: 100 lines in 3 files changed: 61 ins; 11 del; 28 mod Patch: https://git.openjdk.java.net/jdk/pull/1916.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1916/head:pull/1916 PR: https://git.openjdk.java.net/jdk/pull/1916
Re: RFR: JDK-8243376: java.net.SocketPermission.implies(Permission p) spec is mismatching with implementation [v6]
On Fri, 5 Mar 2021 15:47:43 GMT, Vyom Tewari wrote: >> Changes requested by michaelm (Reviewer). > > As you change equal & hashCode method, if possible can you please add some > additional tests in "jdk/java/net/SocketPermission/Equals.java" just to make > sure we test every corner case. I have addressed the changes mentioned by Micheal and now working on Vyom's suggestions, will send the new patch for review soon. Thanks! - PR: https://git.openjdk.java.net/jdk/pull/1916
Re: RFR: JDK-8243376: java.net.SocketPermission.implies(Permission p) spec is mismatching with implementation [v6]
> Issue > > https://bugs.openjdk.java.net/browse/JDK-8243376 > > Problem > > The scenario is: > - Some specified target hostname resolves to two IP addresses (always the > same address pair). > - The DNS resolved order of the two ip addresses changes (a usual > LoadBalancer type behavior). > - The CNAME of the two ip addresses differ. > > In SocketPermission class(void getIP() method), it internally resolves and > saves only the first IP address resolved, not all the IP addresses resolved. > - Depending on when the implier/implied SocketPermission hostname is > resolved, the resolved addresses order differs, and the internally saved IP > address mismatches, resulting on SocketPermission#implies() false. > > > Michael McMahon kindly reviewed and suggested changes: > https://mail.openjdk.java.net/pipermail/net-dev/2020-May/014001.html Jayashree S Kumar has updated the pull request incrementally with one additional commit since the last revision: Code Review: Incorporated Indention correction, added getIP and updated try-catch inside for loop - Changes: - all: https://git.openjdk.java.net/jdk/pull/1916/files - new: https://git.openjdk.java.net/jdk/pull/1916/files/4db8ca69..b0390cfa Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1916&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1916&range=04-05 Stats: 48 lines in 2 files changed: 13 ins; 9 del; 26 mod Patch: https://git.openjdk.java.net/jdk/pull/1916.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1916/head:pull/1916 PR: https://git.openjdk.java.net/jdk/pull/1916
Re: RFR: JDK-8243376: java.net.SocketPermission.implies(Permission p) spec is mismatching with implementation [v4]
On Mon, 8 Feb 2021 14:43:10 GMT, Michael McMahon wrote: >> Jayashree S Kumar has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Code Review: cname made array accounting for multiple cname values > > Changes requested by michaelm (Reviewer). @Michael-Mc-Mahon and @dfuch: I have addressed the review comments. Please take a look. Thanks! - PR: https://git.openjdk.java.net/jdk/pull/1916
Re: RFR: JDK-8243376: java.net.SocketPermission.implies(Permission p) spec is mismatching with implementation [v5]
> Issue > > https://bugs.openjdk.java.net/browse/JDK-8243376 > > Problem > > The scenario is: > - Some specified target hostname resolves to two IP addresses (always the > same address pair). > - The DNS resolved order of the two ip addresses changes (a usual > LoadBalancer type behavior). > - The CNAME of the two ip addresses differ. > > In SocketPermission class(void getIP() method), it internally resolves and > saves only the first IP address resolved, not all the IP addresses resolved. > - Depending on when the implier/implied SocketPermission hostname is > resolved, the resolved addresses order differs, and the internally saved IP > address mismatches, resulting on SocketPermission#implies() false. > > > Michael McMahon kindly reviewed and suggested changes: > https://mail.openjdk.java.net/pipermail/net-dev/2020-May/014001.html Jayashree S Kumar has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 11 commits: - Merge branch 'master' of https://github.com/openjdk/jdk into socketperm_implies - Code Review: cname made array accounting for multiple cname values - Merge branch 'master' of https://github.com/openjdk/jdk into socketperm_implies - Incorporated changes suggested by Vyom in testcase - Fixed all Whitespace error in testcase - Fixed all Whitespace error in testcase - Fixed automerge failed conflicts - Correct WhiteSpace error - Merge branch 'master' of https://github.com/openjdk/jdk into socketperm_implies - Added testcase and corrected WhiteSpace tab error - ... and 1 more: https://git.openjdk.java.net/jdk/compare/05c6009e...4db8ca69 - Changes: https://git.openjdk.java.net/jdk/pull/1916/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1916&range=04 Stats: 94 lines in 3 files changed: 56 ins; 10 del; 28 mod Patch: https://git.openjdk.java.net/jdk/pull/1916.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1916/head:pull/1916 PR: https://git.openjdk.java.net/jdk/pull/1916
Re: RFR: JDK-8243376: java.net.SocketPermission.implies(Permission p) spec is mismatching with implementation [v4]
> Issue > > https://bugs.openjdk.java.net/browse/JDK-8243376 > > Problem > > The scenario is: > - Some specified target hostname resolves to two IP addresses (always the > same address pair). > - The DNS resolved order of the two ip addresses changes (a usual > LoadBalancer type behavior). > - The CNAME of the two ip addresses differ. > > In SocketPermission class(void getIP() method), it internally resolves and > saves only the first IP address resolved, not all the IP addresses resolved. > - Depending on when the implier/implied SocketPermission hostname is > resolved, the resolved addresses order differs, and the internally saved IP > address mismatches, resulting on SocketPermission#implies() false. > > > Michael McMahon kindly reviewed and suggested changes: > https://mail.openjdk.java.net/pipermail/net-dev/2020-May/014001.html Jayashree S Kumar has updated the pull request incrementally with one additional commit since the last revision: Code Review: cname made array accounting for multiple cname values - Changes: - all: https://git.openjdk.java.net/jdk/pull/1916/files - new: https://git.openjdk.java.net/jdk/pull/1916/files/39cc67da..c3ae7501 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1916&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1916&range=02-03 Stats: 40 lines in 1 file changed: 11 ins; 2 del; 27 mod Patch: https://git.openjdk.java.net/jdk/pull/1916.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1916/head:pull/1916 PR: https://git.openjdk.java.net/jdk/pull/1916
Re: RFR: JDK-8243376: java.net.SocketPermission.implies(Permission p) spec is mismatching with implementation [v2]
On Mon, 11 Jan 2021 15:04:03 GMT, Michael McMahon wrote: >> Jayashree S Kumar has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Incorporated changes suggested by Vyom in testcase > > Hi Jay, > Looking back to my original comment, I think I suggested that the fix should > account for multiple cname values (one for each IP address in the addresses > array). That is still my view. In other words, cname needs to be an array, > the same length as addresses (except in the case where the permission was > constructed using a wildcard - in that case it can continue as a single > value, ie the array would have length 1). > > Your solution here drops the caching aspect, and every time getCanonName() is > called it will do the DNS reverse lookup which could slow things down a lot. > Assuming that DNS always returns the same values but just in a different > order, then it should be possible to cache all the canonical names and do a > comparison across them all, without having to go back to DNS each time. > > - Michael. @Michael-Mc-Mahon: Please take a look at the above patch. Thanks! - PR: https://git.openjdk.java.net/jdk/pull/1916
Re: RFR: JDK-8243376: java.net.SocketPermission.implies(Permission p) spec is mismatching with implementation [v3]
> Issue > > https://bugs.openjdk.java.net/browse/JDK-8243376 > > Problem > > The scenario is: > - Some specified target hostname resolves to two IP addresses (always the > same address pair). > - The DNS resolved order of the two ip addresses changes (a usual > LoadBalancer type behavior). > - The CNAME of the two ip addresses differ. > > In SocketPermission class(void getIP() method), it internally resolves and > saves only the first IP address resolved, not all the IP addresses resolved. > - Depending on when the implier/implied SocketPermission hostname is > resolved, the resolved addresses order differs, and the internally saved IP > address mismatches, resulting on SocketPermission#implies() false. > > > Michael McMahon kindly reviewed and suggested changes: > https://mail.openjdk.java.net/pipermail/net-dev/2020-May/014001.html Jayashree S Kumar has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains ten commits: - Merge branch 'master' of https://github.com/openjdk/jdk into socketperm_implies - Incorporated changes suggested by Vyom in testcase - Fixed all Whitespace error in testcase - Fixed all Whitespace error in testcase - Fixed automerge failed conflicts - Correct WhiteSpace error - Merge branch 'master' of https://github.com/openjdk/jdk into socketperm_implies - Added testcase and corrected WhiteSpace tab error - JDK-8243376: java.net.SocketPermission.implies(Permission p) spec is mismatching with implementation - Changes: https://git.openjdk.java.net/jdk/pull/1916/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1916&range=02 Stats: 65 lines in 3 files changed: 47 ins; 10 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/1916.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1916/head:pull/1916 PR: https://git.openjdk.java.net/jdk/pull/1916
Re: RFR: JDK-8243376: java.net.SocketPermission.implies(Permission p) spec is mismatching with implementation [v2]
> Issue > > https://bugs.openjdk.java.net/browse/JDK-8243376 > > Problem > > The scenario is: > - Some specified target hostname resolves to two IP addresses (always the > same address pair). > - The DNS resolved order of the two ip addresses changes (a usual > LoadBalancer type behavior). > - The CNAME of the two ip addresses differ. > > In SocketPermission class(void getIP() method), it internally resolves and > saves only the first IP address resolved, not all the IP addresses resolved. > - Depending on when the implier/implied SocketPermission hostname is > resolved, the resolved addresses order differs, and the internally saved IP > address mismatches, resulting on SocketPermission#implies() false. > > > Michael McMahon kindly reviewed and suggested changes: > https://mail.openjdk.java.net/pipermail/net-dev/2020-May/014001.html Jayashree S Kumar has updated the pull request incrementally with one additional commit since the last revision: Incorporated changes suggested by Vyom in testcase - Changes: - all: https://git.openjdk.java.net/jdk/pull/1916/files - new: https://git.openjdk.java.net/jdk/pull/1916/files/e7c5f585..f5a2f88c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1916&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1916&range=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/1916.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1916/head:pull/1916 PR: https://git.openjdk.java.net/jdk/pull/1916
Re: RFR: JDK-8243376: java.net.SocketPermission.implies(Permission p) spec is mismatching with implementation
On Sat, 2 Jan 2021 10:11:30 GMT, Jayashree S Kumar wrote: > Issue > > https://bugs.openjdk.java.net/browse/JDK-8243376 > > Problem > > The scenario is: > - Some specified target hostname resolves to two IP addresses (always the > same address pair). > - The DNS resolved order of the two ip addresses changes (a usual > LoadBalancer type behavior). > - The CNAME of the two ip addresses differ. > > In SocketPermission class(void getIP() method), it internally resolves and > saves only the first IP address resolved, not all the IP addresses resolved. > - Depending on when the implier/implied SocketPermission hostname is > resolved, the resolved addresses order differs, and the internally saved IP > address mismatches, resulting on SocketPermission#implies() false. > > > Michael McMahon kindly reviewed and suggested changes: > https://mail.openjdk.java.net/pipermail/net-dev/2020-May/014001.html Apologies for the various (noise) commits for the Jcheck Whitespace error. Will be more mindful and invest time in setting up local pre-commit check here-on. The actual commits being: Changes to file: src/java.base/share/classes/java/net/SocketPermission.java A new test case: test/jdk/java/net/SocketPermission/SocketPermissionIm.java A new host file: test/jdk/java/net/SocketPermission/Host.txt Thanks! - PR: https://git.openjdk.java.net/jdk/pull/1916
RFR: JDK-8243376: java.net.SocketPermission.implies(Permission p) spec is mismatching with implementation
Issue https://bugs.openjdk.java.net/browse/JDK-8243376 Problem The scenario is: - Some specified target hostname resolves to two IP addresses (always the same address pair). - The DNS resolved order of the two ip addresses changes (a usual LoadBalancer type behavior). - The CNAME of the two ip addresses differ. In SocketPermission class(void getIP() method), it internally resolves and saves only the first IP address resolved, not all the IP addresses resolved. - Depending on when the implier/implied SocketPermission hostname is resolved, the resolved addresses order differs, and the internally saved IP address mismatches, resulting on SocketPermission#implies() false. Michael McMahon kindly reviewed and suggested changes: https://mail.openjdk.java.net/pipermail/net-dev/2020-May/014001.html - Commit messages: - Fixed all Whitespace error in testcase - Fixed all Whitespace error in testcase - Fixed automerge failed conflicts - Correct WhiteSpace error - Merge branch 'master' of https://github.com/openjdk/jdk into socketperm_implies - Added testcase and corrected WhiteSpace tab error - JDK-8243376: java.net.SocketPermission.implies(Permission p) spec is mismatching with implementation Changes: https://git.openjdk.java.net/jdk/pull/1916/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1916&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8243376 Stats: 65 lines in 3 files changed: 47 ins; 10 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/1916.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1916/head:pull/1916 PR: https://git.openjdk.java.net/jdk/pull/1916
