8344365: SecurityManager cleanups in java.sql and java.sql.rowset modules

[eirbjo]

Please review this PR which cleans up SecurityManager-related code in java.sql and java.sql.rowset modules post JEP-486

There are quite a few changes to review, but all relatively straightforward:

DriverManager

• Remove SecurityManager::checkPermission calls in the setLogWriter, setLogStream and deregisterDriver methods
• Remove two now-unused package private SQLPermission constants
• ensureDriversInitialized is updated to remove AccessController::doPrivileged when reading a system property and when initializing drivers

CachedRowSetImpl

• Remove AccessController::doPrivileged when getting a SyncFactory instance
• getObject is update to remove a call to ReflectUtil::checkPackageAccess

CachedRowSetWriter

• A call to ReflectUtil::checkPackageAccess is removed.

SerialJavaObject

• getFields is updated to remove call to ReflectUtil::checkPackageAccess. @CallerSensitive is no longer needed for this method.
• The test CheckCSMs.java is updated to remove references to SerialJavaObject:getFields

SyncFactory

• initMapIfNecessary is updated to remove call to AccessController::doPrivileged when reading system properties and when reading properties from an input stream
• getInstance is updated to remove calls to ReflectUtil::checkPackageAccess
• setLogger method is updated to remove call to SecurityManager::checkPermission
• setJNDIContext methods are updated to remove call to SecurityManager::checkPermission

RowsetProvider

• Static initializer is updated to call System::getProperty directly
• newFactory is updated to call System::getProperty directly
• newFactory is updated to not call ReflectUtil.checkPackageAccess
• getContextClassLoader is updated to not call AccessController::doPrivileged
• getFactoryClass is updated to not call ReflectUtil.checkPackageAccess
• getSystemProperty is removed

SQLInputImpl

• A call to ReflectUtil::checkPackageAccess is removed

TestPolicy.java in test/java/sql/testng/util

• This is now unused and removed

Ran test/jdk/java/sql and test/jdk/javax/sql tests locally. GHA results pending.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)

Issue

• JDK-8344365: SecurityManager cleanups in java.sql and java.sql.rowset modules (Enhancement - P4)

Reviewers

• Roger Riggs (@RogerRiggs - Reviewer)
• Brent Christian (@bchristi-git - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22185

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back eirbjo! 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]

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

8344365: SecurityManager cleanups in java.sql and java.sql.rowset modules

Reviewed-by: rriggs, bchristi

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

• 5cb0d43: 8293040: Argfile documentation for java launcher tool is confusing regarding usage of wildcards
• 8d43e0d: 8344331: SM cleanup in java.scripting
• f636674: 8344247: Move objectWaiter field to VirtualThread instance
• de6e013: 8344310: Remove Security Manager dependencies from javax.crypto and com.sun.crypto packages

Please see this link for an up-to-date comparison between the source branch of this pull request and 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]

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

• core-libs

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]

Webrevs

• 03: Full - Incremental (0278c59e)
• 02: Full (3f1df59a)
• 01: Full - Incremental (d539a31a)
• 00: Full (d45c5ec0)

[AlanBateman]

I think Brent and/or Lance have been working on this already. If you are taking this, can you remove src/java.sql.rowset/share/classes/com/sun/rowset/internal/XmlReaderContentHandler.java from the patch as it introduces a behavioural change that may require further work.

[eirbjo]

I think Brent and/or Lance have been working on this already.

Happy to yield if Brent/Lance has something cooking. Otherwise, perhaps their time is better spent as reviewers.

If you are taking this, can you remove src/java.sql.rowset/share/classes/com/sun/rowset/internal/XmlReaderContentHandler.java from the patch as it introduces a behavioural change that may require further work.

Seems like a clean inline, how does this introduce a behavioural change?

ReflectUtil:

public static Class<?> forName(String name) throws ClassNotFoundException {
    return Class.forName(name);
}

[AlanBateman]

Seems like a clean inline, how does this introduce a behavioural change?

Class.forName's behavior depends on the caller's defining class loader. I don't know where Brent and/or Lance is on the changes for this module but if you are taking it then I would prefer if the changes to XmlReaderContentHandler were dropped.

[eirbjo]

Class.forName's behavior depends on the caller's defining class loader. I don't know where Brent and/or Lance is on the changes for this module but if you are taking it then I would prefer if the changes to XmlReaderContentHandler were dropped.

Thanks, makes sense. I have reverted changes in XmlReaderContentHandler

[LanceAndersen]

Thanks for tackling this Eirik. I made a pass through and I think the changes are reasonable.

I have asked Sean to also make a pass as well

[RogerRiggs]

Looks fine.

[RogerRiggs]

/reviewers 2 reviewer

[openjdk]

@eirbjo this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout sm-cleanup-sql
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

[openjdk]

@RogerRiggs
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

[eirbjo]

Hey @RogerRiggs, I needed to merge with master to resolve a conflict after the integration of your PR #22041.

Would you mind taking another look at the merge commit 3f1df59 and re-review this PR?

While looking at your change in #22041, I noticed you removed ObjectStreamField#getType from CheckCSM.KNOWN_NON_FINAL_CSMS, but that it remains in CheckCSM.UNSUPPORTED_VIRTUAL_METHODS.

CheckCSM runs fine with this method removed from UNSUPPORTED_VIRTUAL_METHODS which leads me to think it may have been a leftover?

If it was, I'm happy to remove it while I'm visiting this code anyhow, or you could clean it up via some other PR.

Thanks :-)

[RogerRiggs]

Yes, please cleanup the leftover mention, in CheckCSMs. of ObjectStreamField.getType().
And I'll re-review.

[eirbjo]

Yes, please cleanup the leftover mention, in CheckCSMs. of ObjectStreamField.getType(). And I'll re-review.

Thanks, see 0278c59. This leaves UNSUPPORTED_VIRTUAL_METHODS as an empty set. Should I remove this field and the code using this, or do you think leaving the set empty is enough for this PR? Is someone adding to this set later probable?

[RogerRiggs]

Looks good, thanks for the update.

[RogerRiggs]

Leave the test with an empty set of methods for CallerSensistiveAdepter will make it clearer to check for matching test conditions.

[liach]

Your mailing list message recommends removal of this test. This test covers more than ensuring that CSMs are final or static; it actually scans all platform classes to ensure CSMs are present wherever they are needed.

[eirbjo]

Sure, I'll leave the test with the empty set as suggested by Roger.

[bchristi-git]

I think the changes look fine.

[bchristi-git]

I submitted an internal automated test run and it passes cleanly.

[eirbjo]

/integrate

[openjdk]

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

• 9e92a9e: 8344059: Remove doPrivileged calls from windows platform sources in the java.desktop module
• 3729884: 8344371: RISC-V: compiler/intrinsics/chacha/TestChaCha20.java fails after JDK-8343555
• dd86369: 8344262: Win32AttachOperationRequest objects are created by using global new
• 5cb0d43: 8293040: Argfile documentation for java launcher tool is confusing regarding usage of wildcards
• 8d43e0d: 8344331: SM cleanup in java.scripting
• f636674: 8344247: Move objectWaiter field to VirtualThread instance
• de6e013: 8344310: Remove Security Manager dependencies from javax.crypto and com.sun.crypto packages

Your commit was automatically rebased without conflicts.

[openjdk]

@eirbjo Pushed as commit d85dd77.

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