[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR: JDK-8149461: jmap kills process if non-java pid is specified in the command line
From: David Holmes <david.holmes () oracle ! com>
Date: 2019-02-20 3:50:43
Message-ID: 6d6b490d-7c48-e33f-12ff-33bee1723c5c () oracle ! com
[Download RAW message or body]
On 19/02/2019 10:07 pm, gary.adams@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<pid> whereas it currently "just works".
> >
> > This will need a CSR request for any new flag or change in behaviour.
> >
> > But to be blunt I don't like where this is going and the cure seems
> > far worse than the disease.
> >
> > David
> >
> > On 16/02/2019 4:39 am, Gary Adams wrote:
> > > It isn't pretty, but it's functional : "-f<pid> to force
> > > communication. ...
> > >
> > > Revised webrev: http://cr.openjdk.java.net/~gadams/8149461/webrev.02/
> > >
> > > On 2/15/19, 11:57 AM, Daniel D. Daugherty wrote:
> > > > Yes. That's the direction I was thinking about.
> > > > Don't know about the '-F<pid>' versus '-F <pid>', but
> > > > that a cmd line option parsing detail.
> > > >
> > > > Dan
> > > >
> > > >
> > > > On 2/15/19 11:37 AM, Gary Adams wrote:
> > > > > Here's a quick dirty "-F<pid>" that gets past
> > > > > the "-XX:-UsePerfData" setting for jcmd.
> > > > > Need to follow up on docs and usage
> > > > > for the other commands.
> > > > >
> > > > > Is this the direction you were thinking?
> > > > >
> > > > > diff --git
> > > > > a/src/jdk.jcmd/share/classes/sun/tools/common/ProcessArgumentMatcher.javab/src/jdk.jcmd/share/classes/sun/tools/common/ProcessArgumentMatcher.java \
> > > > >
> > > > > ---
> > > > > a/src/jdk.jcmd/share/classes/sun/tools/common/ProcessArgumentMatcher.java
> > > > >
> > > > > +++
> > > > > b/src/jdk.jcmd/share/classes/sun/tools/common/ProcessArgumentMatcher.java
> > > > >
> > > > > @@ -48,16 +48,27 @@
> > > > > public class ProcessArgumentMatcher {
> > > > > private String matchClass;
> > > > > private String singlePid;
> > > > > + private static boolean bypassPid;
> > > > > + private long pid;
> > > > >
> > > > > public ProcessArgumentMatcher(String pidArg) {
> > > > > if (pidArg == null || pidArg.isEmpty()) {
> > > > > throw new IllegalArgumentException("Pid string is
> > > > > invalid");
> > > > > }
> > > > > if (pidArg.charAt(0) == '-') {
> > > > > + if (pidArg.charAt(1) == 'F') {
> > > > > + // Allow -F<pid> to bypass the pid check
> > > > > + pid = Long.parseLong(pidArg.substring(2));
> > > > > + if (pid != 0) {
> > > > > + singlePid = String.valueOf(pid);
> > > > > + }
> > > > > + bypassPid = true;
> > > > > + } else {
> > > > > throw new IllegalArgumentException("Unrecognized " +
> > > > > pidArg);
> > > > > }
> > > > > + }
> > > > > try {
> > > > > - long pid = Long.parseLong(pidArg);
> > > > > + pid = Long.parseLong(pidArg);
> > > > > if (pid != 0) {
> > > > > singlePid = String.valueOf(pid);
> > > > > }
> > > > > @@ -170,4 +181,21 @@
> > > > > public Collection<String> getVirtualMachinePids() {
> > > > > return this.getVirtualMachinePids(null);
> > > > > }
> > > > > +
> > > > > + // Check that pid matches a known running Java process
> > > > > + public static boolean checkJavaPid(String pid) {
> > > > > + // Skip the perf data pid visibility check if "-F<pid>"
> > > > > requested.
> > > > > + if (bypassPid) {
> > > > > + return true;
> > > > > }
> > > > > + List<VirtualMachineDescriptor> l = VirtualMachine.list();
> > > > > + boolean found = false;
> > > > > + for (VirtualMachineDescriptor vmd: l) {
> > > > > + if (vmd.id().equals(pid)) {
> > > > > + found = true;
> > > > > + break;
> > > > > + }
> > > > > + }
> > > > > + return found;
> > > > > + }
> > > > > +}
> > > > > On 2/15/19, 10:24 AM, Daniel D. Daugherty wrote:
> > > > > > On 2/15/19 8:44 AM, Gary Adams wrote:
> > > > > > > Confirmed
> > > > > > > -XX:-UsePerfData
> > > > > > >
> > > > > > > prevents visibility to jps, but allows direct access via pid.
> > > > > > >
> > > > > > > The new check would block access before the attach is attempted.
> > > > > > >
> > > > > > > May be best to close this bug as "will not fix".
> > > > > > > Requires a valid Java process pid.
> > > > > >
> > > > > > Or make it a best effort solution. The idea that a jmap on a non-Java
> > > > > > PID could kill that PID violates the "do no harm" principle for an
> > > > > > observer tool.
> > > > > >
> > > > > > Of what I have seen on the thread so far, I like these:
> > > > > >
> > > > > > - make the PID check on the specified PID only (should be pretty
> > > > > > fast)
> > > > > > - add a force option that tries to attach to the PID regardless
> > > > > > of what the sanity check says; that will solve the -XX:-UsePerfData
> > > > > > problem.
> > > > > >
> > > > > > Writing/updating tests for this is going to be "interesting".
> > > > > >
> > > > > > Dan
> > > > > >
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > On 2/15/19, 8:29 AM, Bernd Eckenfels wrote:
> > > > > > > > I wonder, instead of listing all VMs, would it be better to
> > > > > > > > check only the target PID?
> > > > > > > >
> > > > > > > > BTW speaking of hs_perf files: is it supposed to work without
> > > > > > > > -XX:-UserPerfData also?
> > > > > > > >
> > > > > > > > Gruss
> > > > > > > > Bernd
> > > > > > > >
> > > > > > > > Gruss
> > > > > > > > Bernd
> > > > > > > > --
> > > > > > > > http://bernd.eckenfels.net
> > > > > > > > ------------------------------------------------------------------------ \
> > > > > > > >
> > > > > > > > *Von:* Gary Adams <gary.adams@oracle.com>
> > > > > > > > *Gesendet:* Freitag, Februar 15, 2019 2:19 PM
> > > > > > > > *An:* gary.adams@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). Its not the best Situation if your
> > > > > > > > > > diagnostic attempts are slow down in such a situation.
> > > > > > > > > >
> > > > > > > > > > Also in the past not all VMs might be recognized by the list
> > > > > > > > > > function, a more targeted attach could still succeed. Is that
> > > > > > > > > > addressed since the container-PID changes? In both cases a
> > > > > > > > > > force option would help.
> > > > > > > > > >
> > > > > > > > > > Gruss
> > > > > > > > > > Bernd
> > > > > > > > > >
> > > > > > > > > > Gruss
> > > > > > > > > > Bernd
> > > > > > > > > > --
> > > > > > > > > > http://bernd.eckenfels.net
> > > > > > > > > > ------------------------------------------------------------------------ \
> > > > > > > > > >
> > > > > > > > > > *Von:* serviceability-dev
> > > > > > > > > > <serviceability-dev-bounces@openjdk.java.net> im Auftrag von
> > > > > > > > > > gary.adams@oracle.com
> > > > > > > > > > *Gesendet:* Freitag, Februar 15, 2019 12:07 PM
> > > > > > > > > > *An:* Thomas Stüfe; David Holmes; Chris Plummer
> > > > > > > > > > *Cc:* OpenJDK Serviceability
> > > > > > > > > > *Betreff:* Re: RFR: JDK-8149461: jmap kills process if
> > > > > > > > > > non-java pid is specified in the command line
> > > > > > > > > > Let me clarify a few things about the proposed fix.
> > > > > > > > > >
> > > > > > > > > > The VirtualMachine.list() mechanism is based on
> > > > > > > > > > Java processes that are up and running.
> > > > > > > > > > During VM startup when agent libraries are loaded,
> > > > > > > > > > the fact is recorded in the filesystem that a Java process
> > > > > > > > > > is eligible for an attach request.
> > > > > > > > > >
> > > > > > > > > > This is a much smaller list than scanning for all the
> > > > > > > > > > running processes and works across all supported
> > > > > > > > > > platforms. It also doesn't rely on fragile command line
> > > > > > > > > > parsing to determine a Java launcher is used.
> > > > > > > > > >
> > > > > > > > > > I believe most of the reported hang situations
> > > > > > > > > > are not for the first level information for pid and
> > > > > > > > > > command line requests. I believe the hangs are due
> > > > > > > > > > to the second level requests that actually attach
> > > > > > > > > > to the process and issue a command to the running
> > > > > > > > > > Java process.
> > > > > > > > > >
> > > > > > > > > > The overhead for the pid check should be relatively small.
> > > > > > > > > > In a standalone command for a one time check at startup
> > > > > > > > > > with 10's of Java processes. I know the documentation
> > > > > > > > > > states the user is responsible for verifying with jps
> > > > > > > > > > that a Java process pid or vmid is provided. But the cost
> > > > > > > > > > of human error can be a terminated process.
> > > > > > > > > >
> > > > > > > > > > Selection by name also has some drawbacks when multiple
> > > > > > > > > > copies of a command are running at the same time.
> > > > > > > > > >
> > > > > > > > > > Running "jcmd 0 ..." is the documented way to run a command on
> > > > > > > > > > all running Java processes.
> > > > > > > > > >
> > > > > > > > > > May I count you as a reviewer on the current changeset?
> > > > > > > > > >
> > > > > > > > > > On 2/15/19 3:54 AM, Thomas Stüfe wrote:
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > On Fri, Feb 15, 2019 at 3:26 AM David Holmes
> > > > > > > > > > > <david.holmes@oracle.com <mailto:david.holmes@oracle.com>>
> > > > > > > > > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > > Gary,
> > > > > > > > > > >
> > > > > > > > > > > What is the overhead of doing the validation? How do we
> > > > > > > > > > > list VMs? Do we
> > > > > > > > > > > need to examine every running process to get the list of
> > > > > > > > > > > VMs? Wouldn't
> > > > > > > > > > > it be better to check the given process is a VM rather than
> > > > > > > > > > > checking all
> > > > > > > > > > > potential VM processes?
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > I think this is a valid point. Listing VMs is normally quick
> > > > > > > > > > > (just use jcmd without any parms) but I have seen this fail
> > > > > > > > > > > or hang rarely in situations where I then still was able to
> > > > > > > > > > > talk to my VM via pid. I never investigated this but I would
> > > > > > > > > > > not like to be unable to connect to my VM just because some
> > > > > > > > > > > rogue VM mailfunctions.
> > > > > > > > > > >
> > > > > > > > > > > This would be an argument for the alternative I offered in my
> > > > > > > > > > > first answer - just use the proc file system or a similar
> > > > > > > > > > > mechanism to check only the pid you plan on sending sigquit
> > > > > > > > > > > to...
> > > > > > > > > > >
> > > > > > > > > > > I think there is an onus of responsibility on the user to
> > > > > > > > > > > provide a
> > > > > > > > > > > correct pid here, rather than trying to make this foolproof.
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Oh but it can happen quite easily. For example, by pulling up
> > > > > > > > > > > an older jcmd from your shell history and forgetting to
> > > > > > > > > > > modify the pid. Thank god for the command name argument
> > > > > > > > > > > option on jcmd, which I now use mostly.
> > > > > > > > > > >
> > > > > > > > > > > We also had a customer which tried to find all VMs on his
> > > > > > > > > > > machine by attempting to attach with jcmd to every process,
> > > > > > > > > > > killing them left and right :)
> > > > > > > > > > >
> > > > > > > > > > > ... Thomas
> > > > > > > > > > >
> > > > > > > > > > > Thanks,
> > > > > > > > > > > David
> > > > > > > > > > >
> > > > > > > > > > > On 15/02/2019 5:12 am, Gary Adams wrote:
> > > > > > > > > > > > The following commands present a similar kill process
> > > > > > > > > > > behavior:
> > > > > > > > > > > > jcmd
> > > > > > > > > > > > jinfo
> > > > > > > > > > > > jmap
> > > > > > > > > > > > jstack
> > > > > > > > > > > >
> > > > > > > > > > > > The following commands do not attach.
> > > > > > > > > > > > jstat sun.jvmstat.monitor.MonitorException "not found"
> > > > > > > > > > > > jps no pid arguments
> > > > > > > > > > > >
> > > > > > > > > > > > This update moves the checkJavaPid method into the
> > > > > > > > > > > > common/ProcessArgumentsMatcher.java
> > > > > > > > > > > > and applies the check before the pid is used for an
> > > > > > > > > > > attach operation.
> > > > > > > > > > > >
> > > > > > > > > > > > Revised webrev:
> > > > > > > > > > > http://cr.openjdk.java.net/~gadams/8149461/webrev.01/
> > > > > > > > > > > <http://cr.openjdk.java.net/%7Egadams/8149461/webrev.01/>
> > > > > > > > > > > >
> > > > > > > > > > > > On 2/14/19, 12:07 PM, Chris Plummer wrote:
> > > > > > > > > > > > > Hi Gary,
> > > > > > > > > > > > >
> > > > > > > > > > > > > What about the other tools that attach to a user
> > > > > > > > > > > specified process. Do
> > > > > > > > > > > > > they also have this issue?
> > > > > > > > > > > > >
> > > > > > > > > > > > > thanks,
> > > > > > > > > > > > >
> > > > > > > > > > > > > Chris
> > > > > > > > > > > > >
> > > > > > > > > > > > > On 2/14/19 8:35 AM, Gary Adams wrote:
> > > > > > > > > > > > > > There is an old reported problem that using jmap on a
> > > > > > > > > > > process that is
> > > > > > > > > > > > > > not running
> > > > > > > > > > > > > > Java could cause the process to terminate. This is due
> > > > > > > > > > > to the SIGQUIT
> > > > > > > > > > > > > > used
> > > > > > > > > > > > > > when attaching to the process.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > It is a fairly simple operation to validate that the
> > > > > > > > > > > pid matches one
> > > > > > > > > > > > > > of the known
> > > > > > > > > > > > > > running Java processes using VirtualMachine.list().
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Issue:
> > > > > > > > > > > https://bugs.openjdk.java.net/browse/JDK-8149461
> > > > > > > > > > > <https://bugs.openjdk.java.net/browse/JDK-8149461>
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Proposed fix:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > diff --git
> > > > > > > > > > > a/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java
> > > > > > > > > > > > > > b/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java
> > > > > > > > > > > > > > ---
> > > > > > > > > > > a/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java
> > > > > > > > > > > > > > +++
> > > > > > > > > > > b/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java
> > > > > > > > > > > > > > @@ -1,5 +1,5 @@
> > > > > > > > > > > > > > /*
> > > > > > > > > > > > > > - * Copyright (c) 2005, 2018, Oracle and/or its
> > > > > > > > > > > affiliates. All
> > > > > > > > > > > > > > rights reserved.
> > > > > > > > > > > > > > + * Copyright (c) 2005, 2019, Oracle and/or its
> > > > > > > > > > > affiliates. All
> > > > > > > > > > > > > > rights reserved.
> > > > > > > > > > > > > > * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS
> > > > > > > > > > > FILE HEADER.
> > > > > > > > > > > > > > *
> > > > > > > > > > > > > > * This code is free software; you can redistribute it
> > > > > > > > > > > and/or modify it
> > > > > > > > > > > > > > @@ -30,6 +30,7 @@
> > > > > > > > > > > > > > import java.io.InputStream;
> > > > > > > > > > > > > > import java.io
> > > > > > > > > > > <http://java.io>.UnsupportedEncodingException;
> > > > > > > > > > > > > > import java.util.Collection;
> > > > > > > > > > > > > > +import java.util.List;
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > import com.sun.tools.attach.VirtualMachine;
> > > > > > > > > > > > > > import com.sun.tools.attach.VirtualMachineDescriptor;
> > > > > > > > > > > > > > @@ -125,6 +126,10 @@
> > > > > > > > > > > > > > private static void executeCommandForPid(String
> > > > > > > > > > > pid, String
> > > > > > > > > > > > > > command, Object ... args)
> > > > > > > > > > > > > > throws AttachNotSupportedException,
> > > > > > > > > > > IOException,
> > > > > > > > > > > > > > UnsupportedEncodingException {
> > > > > > > > > > > > > > + // Before attaching, confirm that pid is a
> > > > > > > > > > > known Java process
> > > > > > > > > > > > > > + if (!checkJavaPid(pid)) {
> > > > > > > > > > > > > > + throw new AttachNotSupportedException();
> > > > > > > > > > > > > > + }
> > > > > > > > > > > > > > VirtualMachine vm = VirtualMachine.attach(pid);
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > // Cast to HotSpotVirtualMachine as this is an
> > > > > > > > > > > > > > @@ -196,6 +201,19 @@
> > > > > > > > > > > > > > executeCommandForPid(pid, "dumpheap", filename,
> > > > > > > > > > > liveopt);
> > > > > > > > > > > > > > }
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > + // Check that pid matches a known running Java
> > > > > > > > > > > process
> > > > > > > > > > > > > > + static boolean checkJavaPid(String pid) {
> > > > > > > > > > > > > > + List<VirtualMachineDescriptor> l =
> > > > > > > > > > > VirtualMachine.list();
> > > > > > > > > > > > > > + boolean found = false;
> > > > > > > > > > > > > > + for (VirtualMachineDescriptor vmd: l) {
> > > > > > > > > > > > > > + if (vmd.id <http://vmd.id>().equals(pid)) {
> > > > > > > > > > > > > > + found = true;
> > > > > > > > > > > > > > + break;
> > > > > > > > > > > > > > + }
> > > > > > > > > > > > > > + }
> > > > > > > > > > > > > > + return found;
> > > > > > > > > > > > > > + }
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > private static void
> > > > > > > > > > > checkForUnsupportedOptions(String[] args) {
> > > > > > > > > > > > > > // Check arguments for -F, -m, and non-numeric
> > > > > > > > > > > value
> > > > > > > > > > > > > > // and warn the user that SA is not supported
> > > > > > > > > > > anymore
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic