JDK-8243376: java.net.SocketPermission.implies(Permission p) spec is mismatching with implementation #1916

jaysk1 · 2021-01-02T10:11:30Z

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

Progress

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

Issue

JDK-8243376: java.net.SocketPermission.implies(Permission p) spec is mismatching with implementation

Download

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

…mismatching with implementation

bridgekeeper · 2021-01-02T10:11:57Z

👋 Welcome back jaysk1! 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 · 2021-01-02T10:12:53Z

@jaysk1 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.

…rm_implies

openjdk · 2021-01-05T09:26:06Z

⚠️ @jaysk1 This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

mlbridge · 2021-01-05T09:48:06Z

Webrevs

jaysk1 · 2021-01-05T09:48:11Z

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!

vyommani · 2021-01-05T11:25:25Z

Please find below minor comments.
1-> Please use File.separator,
String hostsFileName = System.getProperty("test.src", ".") + File.separator + "Host.txt";
we don't need "System.exit(0);" break will work instead.

Michael-Mc-Mahon

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.

…rm_implies

jaysk1 · 2021-02-05T11:47:21Z

@Michael-Mc-Mahon: Please take a look at the above patch. Thanks!

src/java.base/share/classes/java/net/SocketPermission.java

test/jdk/java/net/SocketPermission/SocketPermissionIm.java

…rm_implies

…ed try-catch inside for loop

jaysk1 · 2021-02-09T19:36:03Z

@Michael-Mc-Mahon and @dfuch: I have addressed the review comments. Please take a look. Thanks!

test/jdk/java/net/SocketPermission/SocketPermissionIm.java

src/java.base/share/classes/java/net/SocketPermission.java

vyommani · 2021-03-05T15:47:43Z

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.

jaysk1 · 2021-03-10T16:04:51Z

I have addressed the changes mentioned by @Michael-Mc-Mahon and now working on Vyom's suggestions, will send the new patch for review soon. Thanks!

…rm_implies

bridgekeeper · 2021-04-21T13:27:43Z

@jaysk1 This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper · 2021-05-19T16:17:55Z

@jaysk1 This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

jaysk1 · 2021-12-16T07:48:47Z

/open

openjdk · 2021-12-16T07:49:47Z

@jaysk1 This pull request is now open

vyommani · 2021-12-20T03:34:39Z

Hi Jay, overall changes look ok to me, please do let me know if you need any help.

jaysk1 · 2021-12-20T08:04:44Z

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.

pushkarnk

@jaysk1 Thanks for this PR! I've left some questions inline.

pushkarnk · 2022-01-10T09:37:17Z

src/java.base/share/classes/java/net/SocketPermission.java

                    that.getCanonName();
                }
-
-                return (this.cname.equalsIgnoreCase(that.cname));
+                return this.cnames[0].equalsIgnoreCase(that.cnames[0]);


@jaysk1 On the event of us reaching here, won't this and that have multiple canonical names each, in different orders? Do you think we need to compare the arrays here?

pushkarnk · 2022-01-10T09:37:17Z

src/java.base/share/classes/java/net/SocketPermission.java

-        if (this.cname != null) {
-            return this.cname.equalsIgnoreCase(that.cname);
+        if (this.cnames != null) {
+            return this.cnames[0].equalsIgnoreCase(that.cnames[0]);


@jaysk1 Can this comparison fail for SocketPermission objects that have multiple cname entries in different orders?

pushkarnk · 2022-01-10T09:37:17Z

src/java.base/share/classes/java/net/SocketPermission.java

            return this.getName().hashCode();
        else
-            return this.cname.hashCode();
+            return this.cnames[0].hashCode();


@jaysk1 Can this cause "equal" SocketPermission objects with the identical, multiple cname entries, but in a different order, to have unequal hash codes?

pushkarnk · 2022-01-10T09:37:17Z

test/jdk/java/net/SocketPermission/SocketPermissionIm.java

+                       System.out.println("Expected true, returned false");
+                       break;
+                  }
+                  addIpToHostsFile(hostname, "1.2.3."+testPass, hostsFileName);


nit: Looks like these needs an indentation correction.

pushkarnk · 2022-01-10T09:37:17Z

test/jdk/java/net/SocketPermission/SocketPermissionIm.java

+
+    private  static void addIpToHostsFile(String host, String addr, String hostsFileName)
+                                                                      throws Exception {
+       String mapping = addr + " " + host;


nit: indentation?

bridgekeeper · 2022-02-07T12:01:44Z

@jaysk1 This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper · 2022-03-07T13:15:51Z

@jaysk1 This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

JDK-8243376: java.net.SocketPermission.implies(Permission p) spec is …

a2c119d

…mismatching with implementation

openjdk bot added the net label Jan 2, 2021

jaysk1 and others added 4 commits Jan 2, 2021

Correct WhiteSpace error

c19a929

Added testcase and corrected WhiteSpace tab error

5ec8152

Merge branch 'master' of https://github.com/openjdk/jdk into socketpe…

de7e530

…rm_implies

Fixed automerge failed conflicts

5db3d3e

Jayashree S Kumar added 2 commits Jan 5, 2021

Fixed all Whitespace error in testcase

eeb5f4c

Fixed all Whitespace error in testcase

e7c5f58

openjdk bot added the rfr label Jan 5, 2021

Incorporated changes suggested by Vyom in testcase

f5a2f88

Michael-Mc-Mahon suggested changes Jan 11, 2021

View changes

Jayashree S Kumar added 2 commits Feb 5, 2021

Merge branch 'master' of https://github.com/openjdk/jdk into socketpe…

39cc67d

…rm_implies

Code Review: cname made array accounting for multiple cname values

c3ae750

Michael-Mc-Mahon suggested changes Feb 8, 2021

View changes

src/java.base/share/classes/java/net/SocketPermission.java Outdated Show resolved Hide resolved

src/java.base/share/classes/java/net/SocketPermission.java Show resolved Hide resolved

Michael-Mc-Mahon suggested changes Feb 8, 2021

View changes

src/java.base/share/classes/java/net/SocketPermission.java Show resolved Hide resolved

dfuch suggested changes Feb 8, 2021

View changes

test/jdk/java/net/SocketPermission/SocketPermissionIm.java Outdated Show resolved Hide resolved

Jayashree S Kumar added 2 commits Feb 9, 2021

Merge branch 'master' of https://github.com/openjdk/jdk into socketpe…

4db8ca6

…rm_implies

Code Review: Incorporated Indention correction, added getIP and updat…

b0390cf

…ed try-catch inside for loop

Michael-Mc-Mahon suggested changes Mar 5, 2021

View changes

test/jdk/java/net/SocketPermission/SocketPermissionIm.java Show resolved Hide resolved

test/jdk/java/net/SocketPermission/SocketPermissionIm.java Show resolved Hide resolved

test/jdk/java/net/SocketPermission/SocketPermissionIm.java Show resolved Hide resolved

Michael-Mc-Mahon suggested changes Mar 5, 2021

View changes

src/java.base/share/classes/java/net/SocketPermission.java Show resolved Hide resolved

Jayashree S Kumar added 2 commits Mar 10, 2021

Merge branch 'master' of https://github.com/openjdk/jdk into socketpe…

e845a4e

…rm_implies

Code Review: Addressed Michael's changes

ecc9a36

openjdk bot removed the rfr label Mar 10, 2021

Jcheck Review: Corrected the tab error

fe714ef

openjdk bot added the rfr label Mar 10, 2021

bridgekeeper bot closed this May 19, 2021

openjdk bot reopened this Dec 16, 2021

pushkarnk suggested changes Jan 10, 2022

View changes

bridgekeeper bot closed this Mar 7, 2022

openjdk / jdk Public

JDK-8243376: java.net.SocketPermission.implies(Permission p) spec is mismatching with implementation #1916

JDK-8243376: java.net.SocketPermission.implies(Permission p) spec is mismatching with implementation #1916

jaysk1 commented Jan 2, 2021 •

edited by openjdk[bot] bot

bridgekeeper bot commented Jan 2, 2021

openjdk bot commented Jan 2, 2021

openjdk bot commented Jan 5, 2021

mlbridge bot commented Jan 5, 2021 •

edited

jaysk1 commented Jan 5, 2021

vyommani commented Jan 5, 2021 •

edited by bridgekeeper[bot] bot

Michael-Mc-Mahon left a comment

jaysk1 commented Feb 5, 2021

jaysk1 commented Feb 9, 2021

vyommani commented Mar 5, 2021 •

edited

jaysk1 commented Mar 10, 2021 •

edited

bridgekeeper bot commented Apr 21, 2021

bridgekeeper bot commented May 19, 2021

jaysk1 commented Dec 16, 2021

openjdk bot commented Dec 16, 2021

vyommani commented Dec 20, 2021

jaysk1 commented Dec 20, 2021

pushkarnk left a comment •

edited by bridgekeeper[bot] bot

pushkarnk Jan 10, 2022 •

edited by bridgekeeper[bot] bot

pushkarnk Jan 10, 2022 •

edited by bridgekeeper[bot] bot

pushkarnk Jan 10, 2022 •

edited by bridgekeeper[bot] bot

pushkarnk Jan 10, 2022 •

edited by bridgekeeper[bot] bot

pushkarnk Jan 10, 2022 •

edited by bridgekeeper[bot] bot

bridgekeeper bot commented Feb 7, 2022

bridgekeeper bot commented Mar 7, 2022

openjdk / jdk Public

JDK-8243376: java.net.SocketPermission.implies(Permission p) spec is mismatching with implementation #1916

JDK-8243376: java.net.SocketPermission.implies(Permission p) spec is mismatching with implementation #1916

Conversation

jaysk1 commented Jan 2, 2021 • edited by openjdk[bot] bot

Progress

Issue

Download

bridgekeeper bot commented Jan 2, 2021

openjdk bot commented Jan 2, 2021

openjdk bot commented Jan 5, 2021

mlbridge bot commented Jan 5, 2021 • edited

Webrevs

jaysk1 commented Jan 5, 2021

vyommani commented Jan 5, 2021 • edited by bridgekeeper[bot] bot

Michael-Mc-Mahon left a comment

jaysk1 commented Feb 5, 2021

jaysk1 commented Feb 9, 2021

vyommani commented Mar 5, 2021 • edited

jaysk1 commented Mar 10, 2021 • edited

bridgekeeper bot commented Apr 21, 2021

bridgekeeper bot commented May 19, 2021

jaysk1 commented Dec 16, 2021

openjdk bot commented Dec 16, 2021

vyommani commented Dec 20, 2021

jaysk1 commented Dec 20, 2021

pushkarnk left a comment • edited by bridgekeeper[bot] bot

pushkarnk Jan 10, 2022 • edited by bridgekeeper[bot] bot

Choose a reason for hiding this comment

pushkarnk Jan 10, 2022 • edited by bridgekeeper[bot] bot

Choose a reason for hiding this comment

pushkarnk Jan 10, 2022 • edited by bridgekeeper[bot] bot

Choose a reason for hiding this comment

pushkarnk Jan 10, 2022 • edited by bridgekeeper[bot] bot

Choose a reason for hiding this comment

pushkarnk Jan 10, 2022 • edited by bridgekeeper[bot] bot

Choose a reason for hiding this comment

bridgekeeper bot commented Feb 7, 2022

bridgekeeper bot commented Mar 7, 2022

jaysk1 commented Jan 2, 2021 •

edited by openjdk[bot] bot

mlbridge bot commented Jan 5, 2021 •

edited

vyommani commented Jan 5, 2021 •

edited by bridgekeeper[bot] bot

vyommani commented Mar 5, 2021 •

edited

jaysk1 commented Mar 10, 2021 •

edited

pushkarnk left a comment •

edited by bridgekeeper[bot] bot

pushkarnk Jan 10, 2022 •

edited by bridgekeeper[bot] bot

pushkarnk Jan 10, 2022 •

edited by bridgekeeper[bot] bot

pushkarnk Jan 10, 2022 •

edited by bridgekeeper[bot] bot

pushkarnk Jan 10, 2022 •

edited by bridgekeeper[bot] bot

pushkarnk Jan 10, 2022 •

edited by bridgekeeper[bot] bot