RFR: 8307977: jcmd and jstack broken for target processes running with elevated capabilities

[Sebastian Lövdahl]

On Tue, 6 Feb 2024 17:08:43 GMT, Kevin Walls <kevinw at openjdk.org> wrote:

> Does CAP_NET_BIND_SERVICE cause any issues for createAttachFile(int pid, int ns_pid) where it creates the .attach file in the current directory - it starts by trying "/proc/" + pid + "/cwd/" + ".attach_pid" + ns_pid, regardless of ns_pid.
>
> I'm curious if that always fails in the situation that causes the issue in this bug.
If so looks like it would catch an IOException and then use findTargetProcessTmpDirectory, but wonder if we should predict it go straight there.

Hi @kevinjwalls, and thank you for taking a look!

To make sure we're on the same page, is what you are asking if something like this would make sense (on top of the current state of the PR)?


diff --git src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
index 81d4fd259ed..c06c972b39a 100644
--- src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
+++ src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
@@ -221,16 +221,19 @@ private File findSocketFile(int pid, int ns_pid) throws IOException {
     // checks for the file.
     private File createAttachFile(int pid, int ns_pid) throws IOException {
         String fn = ".attach_pid" + ns_pid;
-        String path = "/proc/" + pid + "/cwd/" + fn;
-        File f = new File(path);
-        try {
-            // Do not canonicalize the file path, or we will fail to attach to a VM in a container.
-            f.createNewFile();
-        } catch (IOException x) {
+
+        File f;
+        if (pid != ns_pid) {
+            String path = "/proc/" + pid + "/cwd/" + fn;
+            f = new File(path);
+        } else {
             String root = findTargetProcessTmpDirectory(pid, ns_pid);
             f = new File(root, fn);
-            f.createNewFile();
         }
+
+        // Do not canonicalize the file path, or we will fail to attach to a VM in a container.
+        f.createNewFile();
+
         return f;
     }
 


That is, if we know that `pid` and `ns_pid` are equal, do not even try to create the file in `/proc/<pid>/cwd`.

That's a good question. I tried to minimize the changes because I'm so unfamiliar with JDK internals and also don't have a good understanding of all the different use-cases that need to work.

I tried out the diff above locally using the reproducer steps from https://github.com/openjdk/jdk/pull/17628#issuecomment-1916589081. It seems to work equally fine in the case of a systemd unit using `AmbientCapabilities=CAP_NET_BIND_SERVICE`, and also in the case of attaching against a JVM running inside a Docker container.

The `test/hotspot/jtreg/containers` and `test/hotspot/jtreg/serviceability` tests all pass too.

That said, I'm still more confident in the current state of the PR, as it more closely follows what has existed before. But if you believe that this is a better way of handling it, I'm fine with that too.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/17628#issuecomment-1933588889