On Fri, Feb 15, 2019 at 3:19 PM Thomas Stüfe <thomas.stu...@gmail.com> wrote:
> 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). > > wait, that one would break downward compatibility. Still want to be able to use new jcmds with older hotspots. > 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 >>> >> >>> >> >>> > >>> >> >> >> >> >>
