8344217: Remove calls to SecurityManager and doPrivileged in java.net.DatagramSocket and java.net.NetMulticastSocket after JEP 486 integration #22303

dfuch · 2024-11-21T18:43:55Z

Please find here a patch that removes use of SecurityManager and doPrivileged in DatagramSocket/MulticastSocket implementation.

Some allusion to the SecurityManager was missed in DatagramSocket::connect, so this patch contains a small API documentation change that will require a CSR.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue
Change requires CSR request JDK-8344777 to be approved

Issues

JDK-8344217: Remove calls to SecurityManager and doPrivileged in java.net.DatagramSocket and java.net.NetMulticastSocket after JEP 486 integration (Sub-task - P4)
JDK-8344777: Remove calls to SecurityManager and doPrivileged in java.net.DatagramSocket and java.net.NetMulticastSocket after JEP 486 integration (CSR)

Reviewers

Roger Riggs (@RogerRiggs - Reviewer) Review applies to 668e0f99
Jaikiran Pai (@jaikiran - Reviewer) Review applies to 668e0f99
Alan Bateman (@AlanBateman - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/22303/head:pull/22303
$ git checkout pull/22303

Update a local copy of the PR:
$ git checkout pull/22303
$ git pull https://git.openjdk.org/jdk.git pull/22303/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22303

View PR using the GUI difftool:
$ git pr show -t 22303

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/22303.diff

Using Webrev

Link to Webrev Comment

….net.DatagramSocket and java.net.NetMulticastSocket after JEP 486 integration

dfuch · 2024-11-21T18:44:09Z

/csr required

bridgekeeper · 2024-11-21T18:44:59Z

👋 Welcome back dfuchs! 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 · 2024-11-21T18:45:45Z

@dfuch 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:

8344217: Remove calls to SecurityManager and doPrivileged in java.net.DatagramSocket and java.net.NetMulticastSocket after JEP 486 integration

Reviewed-by: alanb, rriggs, jpai

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 19 new commits pushed to the master branch:

1114704: 6672644: JComboBox still scrolling if switch to another window and return back
98b6678: 8343741: SA jstack --mixed should print information about VM locks
1b2d9ca: 8344881: Problemlist java/awt/Robot/InfiniteLoopException.java on Linux
6aec2dc: 8344788: Specify that the access control context parameters of Subject.doAsPrivileged are ignored
079f503: 8344568: Renaming ceil_log2 to log2i_ceil
51763b6: 8344525: Fix leftover ExceptionOccurred in java.base
4b16530: 8344795: Remove uses of AccessControlContext in java.desktop module
5154b71: 8343598: Since Checker can mark some preview elements as new even if bytecode reference is identical
8b98f95: 8298387: Implement JEP 497: Quantum-Resistant Module-Lattice-Based Digital Signature Algorithm
21e0fb8: 8343529: serviceability/sa/ClhsdbWhere.java fails AssertionFailure: Corrupted constant pool
... and 9 more: https://git.openjdk.org/jdk/compare/a07b72bfcfa603453ea042e89adeb9dcec8bf63a...master

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 · 2024-11-21T18:46:23Z

@dfuch usage: /csr [needed|unneeded], requires that the issue the pull request refers to links to an approved CSR request.

openjdk · 2024-11-21T18:46:36Z

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

mlbridge · 2024-11-21T18:50:56Z

Webrevs

01: Full - Incremental (72b67a89)
00: Full (668e0f99)

RogerRiggs

looks fine.

AlanBateman · 2024-11-22T07:48:04Z

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

-     * matching the packet's and the socket's address and port. On a send
-     * operation, if the packet's address is set and the packet's address
+     * <p> When a socket is connected, additional checks are performed
+     * on a send operation: if the packet's address is set and the packet's address


Yes, an oversight that the "Care should be taken ..." wasn't removed as part of JEP 486.

Opinions may differ but I think it may be simpler to remove this sentence completely rather than re-word it. The right place to specify that send may throw IAE is in the send method description and is already does this.

If you do want to keep something it could be reduced down to: "When the socket is connected, the send method checks that the packet's address matches the remote address that the socket is connected to", no need to say anything about IAE.

Thanks Alan. Done. I have also updated the CSR.

AlanBateman · 2024-11-22T11:46:30Z

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

+            // explicitFilter may be set to 'true' at connect() time and will
+            // be set to 'false' in disconnect() - or when there's no more
+            // pending packets to filter. If explicitFilter is true,
+            // it means we're connected.
            if (explicitFilter) {


Maybe some day we will be able to re-visit this and just have connect flush any pending datagrams.

dfuch · 2024-11-25T10:07:31Z

/integrate

openjdk · 2024-11-25T10:08:47Z

Going to push as commit 519bb26.
Since your change was applied there have been 34 commits pushed to the master branch:

d112f35: 8344855: Remove calls to SecurityManager and doPrivileged in HTTP related implementation classes in the sun.net and sun.net.www.http packages after JEP 486 integration
da4b7a8: 8341436: containers/docker/TestJcmdWithSideCar.java takes needlessly long to run
333a997: 8335231: [macos] Test java/awt/print/PrinterJob/Cancel/PrinterJobCancel.java failed on macOS because the case didn't get the expected PrintAbortException
9576546: 8343698: Linux x86_64 lto build gives a lot of warnings and fails lto-wrapper: fatal error: make returned 2 exit status
68ba7ee: 8340205: Native linker allows MemoryLayout consisting of only PaddingLayout
6f622da: 8344923: Problem list on java/awt/Robot/ScreenCaptureRobotTest.java on macOS
8f08020: 8344903: Improve error handling TestJhsdbJstackPrintVMLocks.java
a83cfe2: 8344917: Fix recent NULL usage backsliding
6711e13: 8344914: OOME in several tests since JDK-6672644 - JComboBox still scrolling
a6220fa: 8344303: Remove usage of URLUtil.getConnectPermission from sun.awt.SunToolkit and sun.awt.image.URLImageSource
... and 24 more: https://git.openjdk.org/jdk/compare/a07b72bfcfa603453ea042e89adeb9dcec8bf63a...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-11-25T10:08:56Z

@dfuch Pushed as commit 519bb26.

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

8344217: Remove calls to SecurityManager and and doPrivileged in java…

668e0f9

….net.DatagramSocket and java.net.NetMulticastSocket after JEP 486 integration

openjdk bot added the rfr Pull request is ready for review label Nov 21, 2024

openjdk bot added net net-dev@openjdk.org csr Pull request needs approved CSR before integration labels Nov 21, 2024

RogerRiggs approved these changes Nov 21, 2024

View reviewed changes

jaikiran approved these changes Nov 22, 2024

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Nov 22, 2024

AlanBateman reviewed Nov 22, 2024

View reviewed changes

Merge branch 'master' into DS-SM-8344217

c9a7336

openjdk bot added csr Pull request needs approved CSR before integration and removed ready Pull request is ready to be integrated labels Nov 22, 2024

Simplify wording in DatagramSocket::connect

72b67a8

AlanBateman reviewed Nov 22, 2024

View reviewed changes

AlanBateman approved these changes Nov 22, 2024

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Nov 22, 2024

openjdk bot added the integrated Pull request has been integrated label Nov 25, 2024

openjdk bot closed this Nov 25, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8344217: Remove calls to SecurityManager and doPrivileged in java.net.DatagramSocket and java.net.NetMulticastSocket after JEP 486 integration #22303

8344217: Remove calls to SecurityManager and doPrivileged in java.net.DatagramSocket and java.net.NetMulticastSocket after JEP 486 integration #22303

dfuch commented Nov 21, 2024 •

edited by openjdk bot

Loading

dfuch commented Nov 21, 2024

bridgekeeper bot commented Nov 21, 2024

openjdk bot commented Nov 21, 2024 •

edited

Loading

openjdk bot commented Nov 21, 2024

openjdk bot commented Nov 21, 2024

mlbridge bot commented Nov 21, 2024 •

edited

Loading

RogerRiggs left a comment

AlanBateman Nov 22, 2024

dfuch Nov 22, 2024

AlanBateman Nov 22, 2024

dfuch commented Nov 25, 2024

openjdk bot commented Nov 25, 2024

openjdk bot commented Nov 25, 2024

8344217: Remove calls to SecurityManager and doPrivileged in java.net.DatagramSocket and java.net.NetMulticastSocket after JEP 486 integration #22303

8344217: Remove calls to SecurityManager and doPrivileged in java.net.DatagramSocket and java.net.NetMulticastSocket after JEP 486 integration #22303

Conversation

dfuch commented Nov 21, 2024 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

dfuch commented Nov 21, 2024

bridgekeeper bot commented Nov 21, 2024

openjdk bot commented Nov 21, 2024 • edited Loading

openjdk bot commented Nov 21, 2024

openjdk bot commented Nov 21, 2024

mlbridge bot commented Nov 21, 2024 • edited Loading

Webrevs

RogerRiggs left a comment

Choose a reason for hiding this comment

AlanBateman Nov 22, 2024

Choose a reason for hiding this comment

dfuch Nov 22, 2024

Choose a reason for hiding this comment

AlanBateman Nov 22, 2024

Choose a reason for hiding this comment

dfuch commented Nov 25, 2024

openjdk bot commented Nov 25, 2024

openjdk bot commented Nov 25, 2024

dfuch commented Nov 21, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Nov 21, 2024 •

edited

Loading

mlbridge bot commented Nov 21, 2024 •

edited

Loading