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 16:57:29 UTC 2019
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
>>>>>>> >>
>>>>>>> >>
>>>>>>> >
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190215/1137512d/attachment-0001.html>
More information about the serviceability-dev
mailing list