RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line
Daniel D. Daugherty
daniel.daugherty at oracle.com
Fri Feb 15 15:24:29 UTC 2019
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
>>>>> <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/9cc733da/attachment-0001.html>
More information about the serviceability-dev
mailing list