On Tue, 6 Feb 2024 17:08:43 GMT, Kevin Walls <[email protected]> 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
