8313357: Revisit requiring SA tests on OSX to either run as root or use sudo #15238

plummercj · 2023-08-11T01:49:57Z

On OSX, don't require that sudo be used to launch SA tools if developer mode is enabled. More details are in the CR.

Due to this change, the following tests are no longer being skipped if the host has developer mode is enabled. They previously required running as root because if sudo was used some files were created with root ownership and could not be deleted:

serviceability/sa/ClhsdbDumpclass.java
serviceability/sa/TestClassDump.java

I tested on my home system (M1) with developer mode enabled and also disabled. Also tested with CI, which appears to have developer mode disabled on most hosts, but there are a few where it is enabled.

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

Issue

JDK-8313357: Revisit requiring SA tests on OSX to either run as root or use sudo (Enhancement - P4)

Reviewers

David Holmes (@dholmes-ora - Reviewer)
Alex Menkov (@alexmenkov - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15238

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-08-11T01:50:52Z

👋 Welcome back cjplummer! 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 · 2023-08-11T01:52:34Z

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

serviceability

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.

Merge

mlbridge · 2023-08-11T02:15:28Z

Webrevs

00: Full (776d4717)

dholmes-ora

This seems reasonable in principle but I would be concerned about the overhead of exec'ing another process here. How many test actually use this? Is this query only ever executed per VM lifetime? (otherwise we should cache the result).

Thanks.

dholmes-ora · 2023-08-11T05:49:23Z

test/lib/jdk/test/lib/SA/SATestUtils.java

+            System.out.print("DevToolsSecurity stdout: " + out);
+            if (out.equals("")) System.out.println();


This looks odd - why not just an unconditional println?

If it is not "", then out will include a newline at the end of the last line, which means you'd end up with an empty line in the output. I considered the following:

if (!out.equals("")) { System.out.print("DevToolsSecurity stdout: " + out); }

But it prints nothing in the output if it is "", and I that's not what I was looking for.

dholmes-ora · 2023-08-11T05:49:45Z

test/lib/jdk/test/lib/SA/SATestUtils.java

+     */
+    private static boolean developerModeEnabled() {
+        List<String> cmd = new ArrayList<String>();
+        cmd.add("DevToolsSecurity");


Is this tool likely to be in the path?

It is for my work machine, my home machine, and the CI machines. If not installed then an exception is thrown. It probably requires that xcode be installed, but this is already a requirement to run the tests.

plummercj · 2023-08-11T18:00:01Z

This seems reasonable in principle but I would be concerned about the overhead of exec'ing another process here. How many test actually use this? Is this query only ever executed per VM lifetime? (otherwise we should cache the result).

Every SA test uses it except I think one test that doesn't need any special privileges to run. That's about 65 tests.

This PR actually reduces the number of processes needed IF developer mode is enabled. Previously launching an SA tool required 3 processes: one to first check if passwordless sudo works, one to launch the SA tool with sudo, and one for the SA tool itself. So now there are 4 IF developer mode is not enabled (added a developer mode check), but there are only two if developer mode is enabled (developer mode check and SA tool launch).

BTW, one thing I didn't really call out in the description is that this allows the SA tests to be run when passwordless sudo is not enabled but developer mode is. Previously they would have just all thrown SkippedException.

I'm not sure about your caching question. I would assume you are asking how many times it executed per test, which is only once. I can't see caching between test runs because the setting can be changed by the user.

dholmes-ora · 2023-08-14T01:12:54Z

I'm not sure about your caching question. I would assume you are asking how many times it executed per test, which is only once.

Okay. That is still unfortunate as I would expect this to be a dynamic property that changes very rarely.

I can't see caching between test runs because the setting can be changed by the user.

I would not be worried about trying to deal with a user that changes the setting in that way. Test environments are expected to kept in a stable condition.

dholmes-ora · 2023-08-14T01:14:40Z

but there are only two if developer mode is enabled (developer mode check and SA tool launch).

So do we expect developer mode to be enabled in our CI testing?

plummercj · 2023-08-14T06:41:41Z

So do we expect developer mode to be enabled in our CI testing?

We don't expect it to be, but it is on a few machines.

plummercj · 2023-08-14T06:49:55Z

I would not be worried about trying to deal with a user that changes the setting in that way. Test environments are expected to kept in a stable condition.

I think it is best to test for developer mode on each test run. The user might change it after a test run after seeing SkippedException, especially if they are aware that developer mode will address it. They may normally run with it disabled, but want it enabled when running SA tests.

dholmes-ora

Thumbs up from me.

Thanks

openjdk · 2023-08-15T01:02:54Z

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

8313357: Revisit requiring SA tests on OSX to either run as root or use sudo

Reviewed-by: dholmes, amenkov

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

0c3bc71: 8281169: Expand discussion of elements and types
f143380: 8314240: test/jdk/sun/security/pkcs/pkcs7/SignerOrder.java fails to compile
6b396da: 8062795: (fs) Files.setPermissions requires read access when NOFOLLOW_LINKS specified
7b28d36: 8314330: java/foreign tests should respect vm flags when start new processes
b32d641: 8311943: Cleanup usages of toLowerCase() and toUpperCase() in java.base
13f6450: 8313765: Invalid CEN header (invalid zip64 extra data field size)
24e896d: 8310275: Bug in assignment operator of ReservedMemoryRegion
1925508: 8314144: gc/g1/ihop/TestIHOPStatic.java fails due to extra concurrent mark with -Xcomp
b80001d: 8314209: Wrong @SInCE tag for RandomGenerator::equiDoubles
ef6db5c: 8314211: Add NativeLibraryUnload event
... and 53 more: https://git.openjdk.org/jdk/compare/9abb2a559e4f809f07db1b747660f68b9d943e3b...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.

plummercj · 2023-08-16T17:47:58Z

Ping! I could use one more review please. Thanks

plummercj · 2023-08-17T15:25:48Z

Thank you Alex and David!

/integrate

openjdk · 2023-08-17T15:26:47Z

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

388dcff: 8282712: VMConnection.open() does not detect if VM failed to be created, resulting in NPE
e8f6b3e: 8314268: Missing include in assembler_riscv.hpp
c634bdf: 8314444: Update jib-profiles.js to use JMH 1.37 devkit
2b81885: 8314071: Test java/foreign/TestByteBuffer.java timed out
32efd23: 8311939: Excessive allocation of Matcher.groups array
ed585d1: 8314280: StructuredTaskScope.shutdown should document that the state of completing subtasks is not defined
6f1071f: 8314213: DocLint should warn about unknown standard tags
4331193: 8314423: Multiple patterns without unnamed variables
249dc37: 8314321: Remove unused field jdk.internal.util.xml.impl.Attrs.mAttrIdx
b78f5a1: 8314076: ICC_ColorSpace#minVal/maxVal have the opposite description
... and 64 more: https://git.openjdk.org/jdk/compare/9abb2a559e4f809f07db1b747660f68b9d943e3b...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-08-17T15:26:53Z

@plummercj Pushed as commit 62ca001.

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

don't require sudo when in developer mode

181e169

openjdk bot changed the title ~~8313357~~ 8313357: Revisit requiring SA tests on OSX to either run as root or use sudo Aug 11, 2023

openjdk bot added the rfr Pull request is ready for review label Aug 11, 2023

openjdk bot added the serviceability serviceability-dev@openjdk.org label Aug 11, 2023

Merge branch 'master' into 8313357_dev_mode

776d471

Merge

dholmes-ora reviewed Aug 11, 2023

View reviewed changes

dholmes-ora approved these changes Aug 15, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 15, 2023

alexmenkov approved these changes Aug 16, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Aug 17, 2023

openjdk bot closed this Aug 17, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Aug 17, 2023

		System.out.print("DevToolsSecurity stdout: " + out);
		if (out.equals("")) System.out.println();

8313357: Revisit requiring SA tests on OSX to either run as root or use sudo #15238

8313357: Revisit requiring SA tests on OSX to either run as root or use sudo #15238

Uh oh!

Conversation

plummercj commented Aug 11, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Aug 11, 2023

Uh oh!

openjdk bot commented Aug 11, 2023

Uh oh!

mlbridge bot commented Aug 11, 2023

Webrevs

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

dholmes-ora Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

plummercj Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

dholmes-ora Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

plummercj Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

plummercj commented Aug 11, 2023

Uh oh!

dholmes-ora commented Aug 14, 2023

Uh oh!

dholmes-ora commented Aug 14, 2023

Uh oh!

plummercj commented Aug 14, 2023

Uh oh!

plummercj commented Aug 14, 2023

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Aug 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plummercj commented Aug 16, 2023

Uh oh!

plummercj commented Aug 17, 2023

Uh oh!

openjdk bot commented Aug 17, 2023

Uh oh!

openjdk bot commented Aug 17, 2023

Uh oh!

Uh oh!

plummercj commented Aug 11, 2023 •

edited by openjdk bot

Loading

openjdk bot commented Aug 15, 2023 •

edited

Loading