On Tue, 1 Aug 2023 17:48:30 GMT, Chris Plummer <cjplum...@openjdk.org> wrote:
>> test/hotspot/jtreg/serviceability/HeapDump/IntegrityHeapDumpTest.java line >> 84: >> >>> 82: .addToolArg(heapDumpFile.getAbsolutePath()); >>> 83: >>> 84: ProcessBuilder processBuilder = new >>> ProcessBuilder(launcher.getCommand()); >> >> Can you explain this change? This will result in the test not being run with >> sudo in cases where sudo is necessary. 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. So if >> sudo is required, the test will fail to attach to the debuggee. Note you >> might not see this issue if you have DevSecurityMode enabled. See >> [JDK-8313357](https://bugs.openjdk.org/browse/JDK-8313357) > > 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. > Can you explain this change? This will result in the test not being run with > sudo in cases where sudo is necessary. 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. So if sudo > is required, the test will fail to attach to the debuggee. Note you might not > see this issue if you have DevSecurityMode enabled. See > [JDK-8313357](https://bugs.openjdk.org/browse/JDK-8313357) I followed what DuplicateArrayClassesTest.java. The reason seems that `SATestUtils.createProcessBuilder` does not touch any verification of whether passwordless sudo is supported, we need to use SATestUtils.skipIfCannotAttach explicitly. 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; } All occurrences of skipIfCannotAttach are currently under `test/hotspot/jtreg/serviceability/sa or jhsdb` ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/13667#discussion_r1281477905
