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-boun...@openjdk.java.net> im Auftrag von
gary.ad...@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.hol...@oracle.com
<mailto:david.hol...@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
>>
>>
>
