Hi Gary, thanks for taking this on. I have occasionally killed a foreign process with jcmd and it is a bit embarrassing :)
I think your patch is okay, but I wonder whether a check directly in libattach would be simpler and cover all uses of the attach framework. Such a test could be e.g. simply checking /proc/<pid>/cmdline whether it is a java launcher. Or even better - since this would not work with a custom launcher - check /proc/<pid>/maps for a mapping of a libjvm.so. This could even be done in native coding, right before sending SIGQUIT. Cheers, Thomas On Thu, Feb 14, 2019 at 8:11 PM Gary Adams <gary.ad...@oracle.com> wrote: > The following commands present a similar kill process behavior: > jcmd > jinfo > jmap > jstack > > The following commands do not attach. > jstat sun.jvmstat.monitor.MonitorException "not found" > jps no pid arguments > > This update moves the checkJavaPid method into the > common/ProcessArgumentsMatcher.java > and applies the check before the pid is used for an attach operation. > > Revised webrev: http://cr.openjdk.java.net/~gadams/8149461/webrev.01/ > > On 2/14/19, 12:07 PM, Chris Plummer wrote: > > Hi Gary, > > > > What about the other tools that attach to a user specified process. Do > > they also have this issue? > > > > thanks, > > > > Chris > > > > On 2/14/19 8:35 AM, Gary Adams wrote: > >> There is an old reported problem that using jmap on a process that is > >> not running > >> Java could cause the process to terminate. This is due to the SIGQUIT > >> used > >> when attaching to the process. > >> > >> It is a fairly simple operation to validate that the pid matches one > >> of the known > >> running Java processes using VirtualMachine.list(). > >> > >> Issue: https://bugs.openjdk.java.net/browse/JDK-8149461 > >> > >> Proposed fix: > >> > >> diff --git a/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java > >> b/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java > >> --- a/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java > >> +++ b/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java > >> @@ -1,5 +1,5 @@ > >> /* > >> - * Copyright (c) 2005, 2018, Oracle and/or its affiliates. All > >> rights reserved. > >> + * Copyright (c) 2005, 2019, Oracle and/or its affiliates. All > >> rights reserved. > >> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. > >> * > >> * This code is free software; you can redistribute it and/or modify it > >> @@ -30,6 +30,7 @@ > >> import java.io.InputStream; > >> import java.io.UnsupportedEncodingException; > >> import java.util.Collection; > >> +import java.util.List; > >> > >> import com.sun.tools.attach.VirtualMachine; > >> import com.sun.tools.attach.VirtualMachineDescriptor; > >> @@ -125,6 +126,10 @@ > >> private static void executeCommandForPid(String pid, String > >> command, Object ... args) > >> throws AttachNotSupportedException, IOException, > >> UnsupportedEncodingException { > >> + // Before attaching, confirm that pid is a known Java process > >> + if (!checkJavaPid(pid)) { > >> + throw new AttachNotSupportedException(); > >> + } > >> VirtualMachine vm = VirtualMachine.attach(pid); > >> > >> // Cast to HotSpotVirtualMachine as this is an > >> @@ -196,6 +201,19 @@ > >> executeCommandForPid(pid, "dumpheap", filename, liveopt); > >> } > >> > >> + // Check that pid matches a known running Java process > >> + static boolean checkJavaPid(String pid) { > >> + List<VirtualMachineDescriptor> l = VirtualMachine.list(); > >> + boolean found = false; > >> + for (VirtualMachineDescriptor vmd: l) { > >> + if (vmd.id().equals(pid)) { > >> + found = true; > >> + break; > >> + } > >> + } > >> + return found; > >> + } > >> + > >> private static void checkForUnsupportedOptions(String[] args) { > >> // Check arguments for -F, -m, and non-numeric value > >> // and warn the user that SA is not supported anymore > > > > > >
