On Wed, 2 Aug 2023 06:59:00 GMT, Yi Yang <yy...@openjdk.org> wrote:
>> I see now that this is a new test, and not an SA test, so probably should
>> never have been using SATestUtils in the first place (I'm curious as to how
>> that ended up happening). You should also remove the import of SATestUtils.
>
>> Before we get here we already checked if we need sudo (not running as root)
>> and verified that passwordless sudo is supported. Otherwise the test is
>> skipped and would never get here.
>
> The reason seems that `SATestUtils.createProcessBuilder` does not touch any
> verification of whether passwordless sudo is supported, this verification is
> done by `SATestUtils.skipIfCannotAttach`.
>
>
> public class SATestUtils {
> public static ProcessBuilder createProcessBuilder(JDKToolLauncher
> launcher) {
> List<String> cmdStringList = Arrays.asList(launcher.getCommand());
> if (needsPrivileges()) {
> cmdStringList = addPrivileges(cmdStringList);
> }
> return new ProcessBuilder(cmdStringList);
> }
> public static boolean needsPrivileges() {
> return Platform.isOSX() && !Platform.isRoot();
> }
> private static List<String> addPrivileges(List<String> cmdStringList) {
> if (!Platform.isOSX()) {
> throw new RuntimeException("Can only add privileges on OSX.");
> }
>
> System.out.println("Adding 'sudo -E -n' to the command.");
> List<String> outStringList = new ArrayList<String>();
> outStringList.add("sudo");
> outStringList.add("-E"); // Preserve existing environment variables.
> outStringList.add("-n"); // non-interactive. Don't prompt for
> password. Must be cached or not required.
> outStringList.addAll(cmdStringList);
> return outStringList;
> }
> public static void skipIfCannotAttach() {
> ...
> if (!Platform.isRoot() && !canAddPrivileges()/*here*/) {
> throw new SkippedException("SA Attach not expected to
> work. Insufficient privileges (not root and can't use sudo).");
> }
> }
> } catch (IOException e) {
> throw new RuntimeException("skipIfCannotAttach() failed due to
> IOException.", e);
> }
> }
>
>
>> So if sudo is required, the test will fail to attach to the debuggee.
>
> I followed what DuplicateArrayClassesTest.java does. All occurrences of
> skipIfCannotAttach are currently under `test/hotspot/jtreg/serviceability/sa
> or jhsdb`. I'm not sure if this problem can happen, if so maybe all
> jcmd+heapdump related tests need to be changed accordingly? Maybe this can be
> done in other patches. Honestly I'm not sure.
But DuplicateArrayClassesTest.java does not use
SATestUtils.createProcessBuilder(), so I'm still wondering how this use of
SATestUtils.createProcessBuilder() got in this new test in the first place.
Yes, a test must call SATestUtils.skipIfCannotAttach() before calling
SATestUtils.createProcessBuilder(). But these calls should only be made by SA
tests. Non SA tests have no need for them.
And just to clarify, there are two types of "attaches" going on here. SA attach
does a debugger attach, which varies depending on the OS, but requires some
sort of root or sudo priveledges on OSX. jcmds and tools like jstack and jmap
use the java Attach API, which is just a socket based connection with the JVM,
and just requires that the attaching process be the same user as the JDK
process.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13667#discussion_r1282252095
