RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line
David Holmes
david.holmes at oracle.com
Fri Feb 15 13:38:26 UTC 2019
On 15/02/2019 11:28 pm, Gary Adams wrote:
> On 2/15/19, 8:18 AM, David Holmes wrote:
>> On 15/02/2019 8:04 pm, gary.adams at oracle.com wrote:
>>> 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.
>>
>> I don't follow. The target VM is not started with an agent, so how is
>> it recorded in the file system?
> The VirtualMachine list is based on the /tmp/hsperfdata_<user>/<pids>.
> Perf data initialization is handled when the VM enters live mode.
I can run with -XX:-UsePerfdata and the VM will not appear in
/tmp/hsperfdata_* but I can still run jmap against the pid.
David
> A bug addressed last year fixed an issue with agent initialization and VM
> visibility.
>
>>
>> David
>> -----
>>
>>> 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/
>>>> >
>>>> > 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
>>>> >>
>>>> >>
>>>> >
>>>>
>>>
>
More information about the serviceability-dev
mailing list