RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

Chris Plummer chris.plummer at oracle.com
Tue Feb 19 17:33:31 UTC 2019 

+1. Seemed like a good idea at first, but the potential negative impact 
is just too much.

Chris

On 2/19/19 4:07 AM, gary.adams at oracle.com wrote:
> I'm fine with just closing the bug as will not fix.
>
> The issue deals with a situation where the user provided the wrong pid
> and the consequences were unexpected.
>
> The changes I was prototyping demonstrate that it is possible to restrict
> the cases where the attach would be limited to those Java processes
> that are visible to jps.
>
> Extending to those processes that are run with -XX:-UsePerfData will 
> require
> some additional mechanism. A command line approach would introduce
> an incompatibility. Is there some other mechanism that could be used?
>
> On 2/15/19 9:20 PM, David Holmes wrote:
>> But this will still break existing behaviour as without UsePerfData 
>> you will now have to use -f<pid> whereas it currently "just works".
>>
>> This will need a CSR request for any new flag or change in behaviour.
>>
>> But to be blunt I don't like where this is going and the cure seems 
>> far worse than the disease.
>>
>> David
>>
>> On 16/02/2019 4:39 am, Gary Adams wrote:
>>> 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
>>>>>>>>>>> <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
>>>>>>>>>>> ��� >>
>>>>>>>>>>> ��� >>
>>>>>>>>>>> ��� >
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>

More information about the serviceability-dev mailing list