Could we not just use another signal? One less... quitty? I think SIGUSR2 may still be unused, but I may be mistaken. Or, one of the real time signals (SIGRTMIN + x).
Other than that, I still think that knowing the pid there could be platform dependent ways of verifying the process, e.g. checking that the process has a libjvm.so module loaded (/proc/<pid>/maps). Doing this for just one pid should be cheap. Cheers, Thomas On Fri, Feb 15, 2019 at 2:46 PM Gary Adams <gary.ad...@oracle.com> wrote: > Confirmed > -XX:-UsePerfData > > prevents visibility to jps, but allows direct access via pid. > > The new check would block access before the attach is attempted. > > May be best to close this bug as "will not fix". > Requires a valid Java process pid. > > On 2/15/19, 8:29 AM, Bernd Eckenfels wrote: > > I wonder, instead of listing all VMs, would it be better to check only the > target PID? > > BTW speaking of hs_perf files: is it supposed to work without > -XX:-UserPerfData also? > > Gruss > Bernd > > Gruss > Bernd > -- > http://bernd.eckenfels.net > > ------------------------------ > *Von:* Gary Adams <gary.ad...@oracle.com> <gary.ad...@oracle.com> > *Gesendet:* Freitag, Februar 15, 2019 2:19 PM > *An:* gary.ad...@oracle.com > *Cc:* Bernd Eckenfels; OpenJDK Serviceability > *Betreff:* Re: RFR: JDK-8149461: jmap kills process if non-java pid is > specified in the command line > > On a linux system with 1 Java process and 500 non-Java processes, > /tmp is not tmpfs mounted, 20 runs average 255.5 ms stddev 9.32. > > JDK-8176828 is the issue that enabled perfmemory visibility once the vm is > in live mode. > > For containers that are configured without a shared view of /tmp, it may > be necessary > to include a override of the pid check. > > On 2/15/19, 7:29 AM, Gary Adams wrote: > > I'll get some measurements on some local systems so we have a > specific idea about the overhead of the pid check. > I imagine in most production environments the /tmp tmpfs > is memory only set of checks. > > One of the "not all vms are recognized" was fixed recently. > When starting a libjdwp session with server=y and suspend=y, > the vm was not recognized until a debugger was attached. > > I'm not opposed to adding a force option to skip the valid pid > check. It might be better effort fixing the hung vm case if we > have a concrete failure to investigate. > > On 2/15/19, 6:47 AM, Bernd Eckenfels wrote: > > Hello, > > I see possible issues here, not sure if they still exist but I wanted to > mention them: > > the list-vm function might be slow on a loaded system (as it is a complex > function). It’s not the best Situation if your diagnostic attempts are slow > down in such a situation. > > Also in the past not all VMs might be recognized by the list function, a > more targeted attach could still succeed. Is that addressed since the > container-PID changes? In both cases a force option would help. > > Gruss > Bernd > > Gruss > Bernd > -- > http://bernd.eckenfels.net > > ------------------------------ > *Von:* serviceability-dev <serviceability-dev-boun...@openjdk.java.net> > <serviceability-dev-boun...@openjdk.java.net> im Auftrag von > gary.ad...@oracle.com > *Gesendet:* Freitag, Februar 15, 2019 12:07 PM > *An:* Thomas Stüfe; David Holmes; Chris Plummer > *Cc:* OpenJDK Serviceability > *Betreff:* Re: RFR: JDK-8149461: jmap kills process if non-java pid is > specified in the command line > > Let me clarify a few things about the proposed fix. > > The VirtualMachine.list() mechanism is based on > Java processes that are up and running. > During VM startup when agent libraries are loaded, > the fact is recorded in the filesystem that a Java process > is eligible for an attach request. > > This is a much smaller list than scanning for all the > running processes and works across all supported > platforms. It also doesn't rely on fragile command line > parsing to determine a Java launcher is used. > > I believe most of the reported hang situations > are not for the first level information for pid and > command line requests. I believe the hangs are due > to the second level requests that actually attach > to the process and issue a command to the running > Java process. > > The overhead for the pid check should be relatively small. > In a standalone command for a one time check at startup > with 10's of Java processes. I know the documentation > states the user is responsible for verifying with jps > that a Java process pid or vmid is provided. But the cost > of human error can be a terminated process. > > Selection by name also has some drawbacks when multiple > copies of a command are running at the same time. > > Running "jcmd 0 ..." is the documented way to run a command on > all running Java processes. > > May I count you as a reviewer on the current changeset? > > On 2/15/19 3:54 AM, Thomas Stüfe wrote: > > > > On Fri, Feb 15, 2019 at 3:26 AM David Holmes <david.hol...@oracle.com> > wrote: > >> Gary, >> >> What is the overhead of doing the validation? How do we list VMs? Do we >> need to examine every running process to get the list of VMs? Wouldn't >> it be better to check the given process is a VM rather than checking all >> potential VM processes? >> >> > I think this is a valid point. Listing VMs is normally quick (just use > jcmd without any parms) but I have seen this fail or hang rarely in > situations where I then still was able to talk to my VM via pid. I never > investigated this but I would not like to be unable to connect to my VM > just because some rogue VM mailfunctions. > > This would be an argument for the alternative I offered in my first answer > - just use the proc file system or a similar mechanism to check only the > pid you plan on sending sigquit to... > > >> I think there is an onus of responsibility on the user to provide a >> correct pid here, rather than trying to make this foolproof. >> >> > Oh but it can happen quite easily. For example, by pulling up an older > jcmd from your shell history and forgetting to modify the pid. Thank god > for the command name argument option on jcmd, which I now use mostly. > > We also had a customer which tried to find all VMs on his machine by > attempting to attach with jcmd to every process, killing them left and > right :) > > ... Thomas > > >> Thanks, >> David >> >> On 15/02/2019 5:12 am, Gary Adams 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 >> >> >> >> >> > >> > > > > >
