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 16:37:02 UTC 2019


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.java b/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/cde7a1af/attachment-0001.html>


More information about the serviceability-dev mailing list