RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line
David Holmes
david.holmes at oracle.com
Sat Feb 16 02:20:23 UTC 2019
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