Re: RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[Gary Adams]

I'd like to bring closure to this issue.

Even if we could reliable detect libjvm has been loaded in
a process, it does not guarantee the VM has entered the
live phase, has set up the signal handlers and is prepared
proceed with the attach mechanism.

Without any further input, I'll close this bug on Wed Mar 13, 2019.
I'll add as much documentation as possible before closing the issue,
but it is unlikely to be repopened as the use case falls outside
the reasonable use of the feature.

On 2/19/19, 10:50 PM, David Holmes wrote:

On 19/02/2019 10:07 pm, gary.ad...@oracle.com wrote:

I'm fine with just closing the bug as will not fix.


I personally think it is a non-issue but others (Hi Thomas!) consider 
it a real problem.



The issue deals with a situation where the user provided the wrong pid
and the consequences were unexpected.

The changes I was prototyping demonstrate that it is possible to 
restrict

the cases where the attach would be limited to those Java processes
that are visible to jps.

Extending to those processes that are run with -XX:-UsePerfData will 
require

some additional mechanism. A command line approach would introduce
an incompatibility. Is there some other mechanism that could be used?


Only what Thomas suggested in using a platform specific mechanism to 
verify the pid is for a Java process. Though a simple binary name 
check doesn't suffice as the VM may be embedded in another process, so 
actually checking for libjvm as Thomas suggested may be the only 
practical and reliable way to do this.


Thanks,
David
-


On 2/15/19 9:20 PM, David Holmes wrote:
But this will still break existing behaviour as without UsePerfData 
you will now have to use -f 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 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' versus '-F ', 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" 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 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 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" requested.

+  if (bypassPid) {
+  return true;
 }
+  List 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 

Re: RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[David Holmes]

On 19/02/2019 10:07 pm, gary.ad...@oracle.com wrote:

I'm fine with just closing the bug as will not fix.


I personally think it is a non-issue but others (Hi Thomas!) consider it 
a real problem.



The issue deals with a situation where the user provided the wrong pid
and the consequences were unexpected.

The changes I was prototyping demonstrate that it is possible to restrict
the cases where the attach would be limited to those Java processes
that are visible to jps.

Extending to those processes that are run with -XX:-UsePerfData will 
require

some additional mechanism. A command line approach would introduce
an incompatibility. Is there some other mechanism that could be used?


Only what Thomas suggested in using a platform specific mechanism to 
verify the pid is for a Java process. Though a simple binary name check 
doesn't suffice as the VM may be embedded in another process, so 
actually checking for libjvm as Thomas suggested may be the only 
practical and reliable way to do this.


Thanks,
David
-


On 2/15/19 9:20 PM, David Holmes wrote:
But this will still break existing behaviour as without UsePerfData 
you will now have to use -f 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 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' versus '-F ', 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" 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 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 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" 
requested.

+  if (bypassPid) {
+  return true;
 }
+  List 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?

Re: RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[Chris Plummer]

+1. Seemed like a good idea at first, but the potential negative impact 
is just too much.


Chris

On 2/19/19 4:07 AM, gary.ad...@oracle.com wrote:

I'm fine with just closing the bug as will not fix.

The issue deals with a situation where the user provided the wrong pid
and the consequences were unexpected.

The changes I was prototyping demonstrate that it is possible to restrict
the cases where the attach would be limited to those Java processes
that are visible to jps.

Extending to those processes that are run with -XX:-UsePerfData will 
require

some additional mechanism. A command line approach would introduce
an incompatibility. Is there some other mechanism that could be used?

On 2/15/19 9:20 PM, David Holmes wrote:
But this will still break existing behaviour as without UsePerfData 
you will now have to use -f 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 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' versus '-F ', 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" 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 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 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" 
requested.

+� if (bypassPid) {
+� return true;
�}
+� List 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 
*Gesendet:* Freitag, Februar 15, 2019 2:19 PM
*An:* gary.ad...@oracle.com
*Cc:* Bernd Eckenfels; 

Re: RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[gary.ad...@oracle.com]

I'm fine with just closing the bug as will not fix.

The issue deals with a situation where the user provided the wrong pid
and the consequences were unexpected.

The changes I was prototyping demonstrate that it is possible to restrict
the cases where the attach would be limited to those Java processes
that are visible to jps.

Extending to those processes that are run with -XX:-UsePerfData will require
some additional mechanism. A command line approach would introduce
an incompatibility. Is there some other mechanism that could be used?

On 2/15/19 9:20 PM, David Holmes wrote:
But this will still break existing behaviour as without UsePerfData 
you will now have to use -f 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 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' versus '-F ', 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" 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 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 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" 
requested.

+  if (bypassPid) {
+  return true;
 }
+  List 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 
*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 

Re: RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[Chris Plummer]

  
  
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 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' versus '-F ',
  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" 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 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 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" requested.
+� if (bypassPid) {
+� return true;
�}
+� List 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 

Re: RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[Gary Adams]

It isn't pretty, but it's functional : "-f 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' versus '-F ', 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" 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 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 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" 
requested.

+  if (bypassPid) {
+  return true;
 }
+  List 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 
*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 

Re: RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[Daniel D. Daugherty]

Yes. That's the direction I was thinking about.
Don't know about the '-F' versus '-F ', 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" 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 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 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" 
requested.

+  if (bypassPid) {
+  return true;
 }
+  List 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 
*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 

Re: RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[Gary Adams]

Here's a quick dirty "-F" 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.java b/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 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 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" 
requested.

+  if (bypassPid) {
+  return true;
 }
+  List 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 
*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 

Re: RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[Daniel D. Daugherty]

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 
*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

Gruss
Bernd
--
http://bernd.eckenfels.net

*Von:* serviceability-dev 
 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 
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
   

Re: RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[Thomas Stüfe]

Could we not just use another signal? One less... quitty? I think SIGUSR2
may still be unused, but I may be mistaken. Or, one of the real time
signals (SIGRTMIN + x).

Other than that, I still think that knowing the pid there could be platform
dependent ways of verifying the process, e.g. checking that the process has
a libjvm.so module loaded (/proc//maps). Doing this for just one pid
should be cheap.

Cheers, Thomas




On Fri, Feb 15, 2019 at 2:46 PM 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.
>
> 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  
> *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
>
> Gruss
> Bernd
> --
> http://bernd.eckenfels.net
>
> --
> *Von:* serviceability-dev 
>  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 
> 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 

Re: RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[Thomas Stüfe]

On Fri, Feb 15, 2019 at 3:19 PM Thomas Stüfe 
wrote:

> Could we not just use another signal? One less... quitty? I think SIGUSR2
> may still be unused, but I may be mistaken. Or, one of the real time
> signals (SIGRTMIN + x).
>
>
wait, that one would break downward compatibility. Still want to be able to
use new jcmds with older hotspots.


> Other than that, I still think that knowing the pid there could be
> platform dependent ways of verifying the process, e.g. checking that the
> process has a libjvm.so module loaded (/proc//maps). Doing this for
> just one pid should be cheap.
>
> Cheers, Thomas
>
>

>
>
> On Fri, Feb 15, 2019 at 2:46 PM 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.
>>
>> 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  
>> *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
>>
>> Gruss
>> Bernd
>> --
>> http://bernd.eckenfels.net
>>
>> --
>> *Von:* serviceability-dev 
>>  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?
>>
>> 

Re: RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[Gary Adams]

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.

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 
*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

Gruss
Bernd
--
http://bernd.eckenfels.net

*Von:* serviceability-dev 
 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 
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 

Re: RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[David Holmes]

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.ad...@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_/.
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 
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/
>
> 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 

Re: RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[Bernd Eckenfels]

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 
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

Gruss
Bernd
--
http://bernd.eckenfels.net


Von: serviceability-dev 

 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 
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 

Re: RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[Gary Adams]

On 2/15/19, 8:18 AM, David Holmes wrote:

On 15/02/2019 8:04 pm, gary.ad...@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_/.
Perf data initialization is handled when the VM enters live mode.

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 
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/
>
> 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 

Re: RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[Gary Adams]

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 
 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 
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 

Re: RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[David Holmes]

On 15/02/2019 8:04 pm, gary.ad...@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?


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 > 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 

Re: RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[Gary Adams]

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 
 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 > 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 

Re: RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[Bernd Eckenfels]

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  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 
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/
>
> 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
>>> --- 

Re: RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[gary.ad...@oracle.com]

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 > 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 

Re: RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[Thomas Stüfe]

On Fri, Feb 15, 2019 at 3:26 AM David Holmes 
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 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
> >>
> >>
> >
>

Re: RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[David Holmes]

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 there is an onus of responsibility on the user to provide a 
correct pid here, rather than trying to make this foolproof.


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 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

Re: RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[Chris Plummer]

Looks good to me. You could also take on Thomas' suggestion, but I'm not 
sure if there are any common use cases that it would be protecting us 
from accidentally killing a process (other than the ones you are already 
fixing).


Chris

On 2/14/19 11: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 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

Re: RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[Thomas Stüfe]

Hi Gary,

thanks for taking this on. I have occasionally killed a foreign process
with jcmd and it is a bit embarrassing :)

I think your patch is okay, but I wonder whether a check directly in
libattach would be simpler and cover all uses of the attach framework.

Such a test could be e.g. simply checking /proc//cmdline whether it is
a java launcher. Or even better - since this would not work with a custom
launcher - check /proc//maps for a mapping of a libjvm.so.

This could even be done in native coding, right before sending SIGQUIT.

Cheers, Thomas








On Thu, Feb 14, 2019 at 8:11 PM 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 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
> >
> >
>
>

Re: RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[Gary Adams]

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 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

Re: RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[Chris Plummer]

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 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

RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line

[Gary Adams]

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 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