RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line
Gary Adams
gary.adams at oracle.com
Fri Feb 15 18:39:53 UTC 2019
It isn't pretty, but it's functional : "-f<pid> to force communication. ...
Revised webrev: http://cr.openjdk.java.net/~gadams/8149461/webrev.02/
On 2/15/19, 11:57 AM, Daniel D. Daugherty wrote:
> Yes. That's the direction I was thinking about.
> Don't know about the '-F<pid>' versus '-F <pid>', but
> that a cmd line option parsing detail.
>
> Dan
>
>
> On 2/15/19 11:37 AM, Gary Adams wrote:
>> Here's a quick dirty "-F<pid>" that gets past
>> the "-XX:-UsePerfData" setting for jcmd.
>> Need to follow up on docs and usage
>> for the other commands.
>>
>> Is this the direction you were thinking?
>>
>> diff --git
>> a/src/jdk.jcmd/share/classes/sun/tools/common/ProcessArgumentMatcher.javab/src/jdk.jcmd/share/classes/sun/tools/common/ProcessArgumentMatcher.java
>> ---
>> a/src/jdk.jcmd/share/classes/sun/tools/common/ProcessArgumentMatcher.java
>> +++
>> b/src/jdk.jcmd/share/classes/sun/tools/common/ProcessArgumentMatcher.java
>> @@ -48,16 +48,27 @@
>> public class ProcessArgumentMatcher {
>> private String matchClass;
>> private String singlePid;
>> + private static boolean bypassPid;
>> + private long pid;
>>
>> public ProcessArgumentMatcher(String pidArg) {
>> if (pidArg == null || pidArg.isEmpty()) {
>> throw new IllegalArgumentException("Pid string is invalid");
>> }
>> if (pidArg.charAt(0) == '-') {
>> + if (pidArg.charAt(1) == 'F') {
>> + // Allow -F<pid> to bypass the pid check
>> + pid = Long.parseLong(pidArg.substring(2));
>> + if (pid != 0) {
>> + singlePid = String.valueOf(pid);
>> + }
>> + bypassPid = true;
>> + } else {
>> throw new IllegalArgumentException("Unrecognized " +
>> pidArg);
>> }
>> + }
>> try {
>> - long pid = Long.parseLong(pidArg);
>> + pid = Long.parseLong(pidArg);
>> if (pid != 0) {
>> singlePid = String.valueOf(pid);
>> }
>> @@ -170,4 +181,21 @@
>> public Collection<String> getVirtualMachinePids() {
>> return this.getVirtualMachinePids(null);
>> }
>> +
>> + // Check that pid matches a known running Java process
>> + public static boolean checkJavaPid(String pid) {
>> + // Skip the perf data pid visibility check if "-F<pid>"
>> requested.
>> + if (bypassPid) {
>> + return true;
>> }
>> + List<VirtualMachineDescriptor> l = VirtualMachine.list();
>> + boolean found = false;
>> + for (VirtualMachineDescriptor vmd: l) {
>> + if (vmd.id().equals(pid)) {
>> + found = true;
>> + break;
>> + }
>> + }
>> + return found;
>> + }
>> +}
>> On 2/15/19, 10:24 AM, Daniel D. Daugherty wrote:
>>> On 2/15/19 8:44 AM, Gary Adams 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.
>>>
>>> Or make it a best effort solution. The idea that a jmap on a non-Java
>>> PID could kill that PID violates the "do no harm" principle for an
>>> observer tool.
>>>
>>> Of what I have seen on the thread so far, I like these:
>>>
>>> - make the PID check on the specified PID only (should be pretty fast)
>>> - add a force option that tries to attach to the PID regardless
>>> of what the sanity check says; that will solve the -XX:-UsePerfData
>>> problem.
>>>
>>> Writing/updating tests for this is going to be "interesting".
>>>
>>> Dan
>>>
>>>
>>>
>>>>
>>>> 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.adams at oracle.com>
>>>>> *Gesendet:* Freitag, Februar 15, 2019 2:19 PM
>>>>> *An:* gary.adams at 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-bounces at openjdk.java.net> im Auftrag von
>>>>>>> gary.adams at 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.holmes at oracle.com <mailto:david.holmes at 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/
>>>>>>>> <http://cr.openjdk.java.net/%7Egadams/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
>>>>>>>> <http://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 <http://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
>>>>>>>> >>
>>>>>>>> >>
>>>>>>>> >
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190215/cbebc405/attachment-0001.html>
More information about the serviceability-dev
mailing list