|
Is anyone setup to try this with
docker? I know initially jvm's running on the same host OS but in
different docker containers would not show up when requesting a
list of JVMs, but could be attached to when a specific PID was
provided. I believe all these issues are now fixed, but would be
good to double check that this change is not interfering in any
way.
Chris
On 2/15/19 10: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
�
Von: Gary Adams <gary.ad...@oracle.com>
Gesendet: Freitag, Februar 15, 2019 2:19 PM
An: gary.ad...@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
�
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:
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.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().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
>>
>>
>
|
