RFR: 8327114: Attach in Linux may have wrong behaviour when pid == ns_pid (Kubernetes debug container) [v2]
Larry Cable
duke at openjdk.org
Mon May 6 18:33:58 UTC 2024
On Mon, 6 May 2024 17:29:05 GMT, Sebastian Lövdahl <duke at openjdk.org> wrote:
>> 8327114: Attach in Linux may have wrong behaviour when pid == ns_pid (Kubernetes debug container)
>
> Sebastian Lövdahl has updated the pull request incrementally with one additional commit since the last revision:
>
> Reworked attach logic
On 5/6/24 10:35 AM, Sebastian Lövdahl wrote:
>
> I pushed an updated attempt at this now with d3e26a0
> <https://urldefense.com/v3/__https://github.com/openjdk/jdk/commit/d3e26a0c444e06ba9757fd528d72d83f56cd098b__;!!ACWV5N9M2RV99hQ!M_KzQgiC8WkHTfJnvTw6zsw7l0RACDgJU3ShDs0R1dAWE_IxEinuX1_Uqom0UPt96Bl6iEqHc-yUUmz5svPHZrq9ZQ$>.
> Local testing and |make test
> TEST="jtreg:test/hotspot/jtreg/containers"| + |make test
> TEST="jtreg:test/hotspot/jtreg/serviceability"| indicate that all the
> known use-cases work.
>
> Still eager to see what you come up with @larry-cable
> <https://urldefense.com/v3/__https://github.com/larry-cable__;!!ACWV5N9M2RV99hQ!M_KzQgiC8WkHTfJnvTw6zsw7l0RACDgJU3ShDs0R1dAWE_IxEinuX1_Uqom0UPt96Bl6iEqHc-yUUmz5svNMPdGFLg$>.
> |createAttachFile| could still be improved for example. And I would
> also be interested in looking into writing some test for the elevated
> privileges case.
>
> —
> Reply to this email directly, view it on GitHub
> <https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/19055*issuecomment-2096564990__;Iw!!ACWV5N9M2RV99hQ!M_KzQgiC8WkHTfJnvTw6zsw7l0RACDgJU3ShDs0R1dAWE_IxEinuX1_Uqom0UPt96Bl6iEqHc-yUUmz5svNUwHWtZA$>,
> or unsubscribe
> <https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/ANTA67SGOTAXCKY2TO2OBDDZA65N5AVCNFSM6AAAAABHDNNTT6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJWGU3DIOJZGA__;!!ACWV5N9M2RV99hQ!M_KzQgiC8WkHTfJnvTw6zsw7l0RACDgJU3ShDs0R1dAWE_IxEinuX1_Uqom0UPt96Bl6iEqHc-yUUmz5svO2ymONGw$>.
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
/*
diff --git
a/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
b/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
index 81d4fd259ed..c148dbd61b7 100644
--- a/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
+++ b/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
@@ -34,6 +34,7 @@
import java.nio.file.Path;
import java.nio.file.Paths;
import java.nio.file.Files;
+import java.util.Optional;
import static java.nio.charset.StandardCharsets.UTF_8;
@@ -46,8 +47,28 @@ public class VirtualMachineImpl extends
HotSpotVirtualMachine {
// location is the same for all processes, otherwise the tools
// will not be able to find all Hotspot processes.
// Any changes to this needs to be synchronized with HotSpot.
- private static final String tmpdir = "/tmp";
+ private static final Path TMPDIR = Path.of("/tmp");
+
+ private static final Path PROC = Path.of("/proc");
+ private static final Path NS_MNT = Path.of("ns/mnt");
+ private static final Path SELF = PROC.resolve("self");
+ private static final Path TMP = Path.of("tmp");
+
+ private static final Optional<Path> SELF_MNT_NS;
+
+ static {
+ Path mountns = null;
+ try {
+ mountns = Files.readSymbolicLink(SELF.resolve(NS_MNT));
+ } catch (IOException _) {
+ // do nothing
+ } finally {
+ SELF_MNT_NS = Optional.ofNullable(mountns);
+ }
+ }
+
String socket_path;
+
/**
* Attaches to the target VM
*/
@@ -235,24 +256,36 @@ private File createAttachFile(int pid, int ns_pid)
throws IOException {
}
private String findTargetProcessTmpDirectory(int pid, int ns_pid)
throws IOException {
- String root;
- if (pid != ns_pid) {
+ final var procPid = PROC.resolve(Integer.toString(pid));
+
+ Optional<Path> tgtMountNS = Optional.empty();
+
+ try {
+ tgtMountNS =
Optional.ofNullable(Files.readSymbolicLink(procPid.resolve(NS_MNT))); //
attempt to read the tgt's mnt ns id...
+ } catch (IOException _) { // if we fail to read the tgt's mnt
ns id then we either dont have access or it no longer exists!
+ if (!Files.exists(procPid)) throw new
IOException(String.format("unable to attach, %s non-existent! process:
%d terminated", procPid, pid));
+
+ // ok so if we get here we have failed to read the tgt's
mnt ns, but the tgt process still exists ... we do not have privs to
read its procfs
+ }
+
+ final boolean sameMountNS = SELF_MNT_NS.isPresent() &&
SELF_MNT_NS.equals(tgtMountNS); // will be false if we did not read the
tgt's mnt ns
+
+ if (!sameMountNS || pid != ns_pid) {
// A process may not exist in the same mount namespace as
the caller, e.g.
// if we are trying to attach to a JVM process inside a
container.
// Instead, attach relative to the target root filesystem
as exposed by
// procfs regardless of namespaces.
- String procRootDirectory = "/proc/" + pid + "/root";
- if (!Files.isReadable(Path.of(procRootDirectory))) {
- throw new IOException(
- String.format("Unable to access root directory
%s " +
- "of target process %d", procRootDirectory, pid));
+ final var procRootDirectory = procPid.resolve("root");
+
+ if (Files.isReadable(procRootDirectory))
+ return procRootDirectory.resolve(TMP).toString();
+ else if (Files.exists(procPid)) { // process is still
alive... so its a permissions issue... fallback but this may also fail
+ return TMPDIR.toString();
+ } else {
+ throw new IOException(String.format("Unable to access
root directory %s " + "of target process %d",
procRootDirectory.toString(), pid));
}
-
- root = procRootDirectory + "/" + tmpdir;
- } else {
- root = tmpdir;
- }
- return root;
+ } else
+ return TMPDIR.toString(); // we are in the same mnt ns
}
/*
--------------ZiY5EhYyDoaC0Fy7Oxy0DC0T
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: 8bit
<!DOCTYPE html><html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
<br>
<br>
<div class="moz-cite-prefix">On 5/6/24 10:35 AM, Sebastian Lövdahl
wrote:<br>
</div>
<blockquote type="cite" ***@***.***">
<p dir="auto">I pushed an updated attempt at this now with <a class="commit-link" data-hovercard-type="commit" data-hovercard-url="https://github.com/openjdk/jdk/commit/d3e26a0c444e06ba9757fd528d72d83f56cd098b/hovercard" href="https://urldefense.com/v3/__https://github.com/openjdk/jdk/commit/d3e26a0c444e06ba9757fd528d72d83f56cd098b__;!!ACWV5N9M2RV99hQ!M_KzQgiC8WkHTfJnvTw6zsw7l0RACDgJU3ShDs0R1dAWE_IxEinuX1_Uqom0UPt96Bl6iEqHc-yUUmz5svPHZrq9ZQ$" moz-do-not-send="true"><tt>d3e26a0</tt></a>. Local testing and
<code class="notranslate">make test
TEST="jtreg:test/hotspot/jtreg/containers"</code> + <code class="notranslate">make test
TEST="jtreg:test/hotspot/jtreg/serviceability"</code> indicate
that all the known use-cases work.</p>
<p dir="auto">Still eager to see what you come up with <a class="user-mention notranslate" data-hovercard-type="user" data-hovercard-url="/users/larry-cable/hovercard" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://urldefense.com/v3/__https://github.com/larry-cable__;!!ACWV5N9M2RV99hQ!M_KzQgiC8WkHTfJnvTw6zsw7l0RACDgJU3ShDs0R1dAWE_IxEinuX1_Uqom0UPt96Bl6iEqHc-yUUmz5svNMPdGFLg$" ***@***.***</a>. <code class="notranslate">createAttachFile</code> could still be
improved for example. And I would also be interested in looking
into writing some test for the elevated privileges case.</p>
<p>—<br>
Reply to this email directly, <a href="https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/19055*issuecomment-2096564990__;Iw!!ACWV5N9M2RV99hQ!M_KzQgiC8WkHTfJnvTw6zsw7l0RACDgJU3ShDs0R1dAWE_IxEinuX1_Uqom0UPt96Bl6iEqHc-yUUmz5svNUwHWtZA$" moz-do-not-send="true">view it on GitHub</a>, or <a href="https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/ANTA67SGOTAXCKY2TO2OBDDZA65N5AVCNFSM6AAAAABHDNNTT6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJWGU3DIOJZGA__;!!ACWV5N9M2RV99hQ!M_KzQgiC8WkHTfJnvTw6zsw7l0RACDgJU3ShDs0R1dAWE_IxEinuX1_Uqom0UPt96Bl6iEqHc-yUUmz5svO2ymONGw$" moz-do-not-send="true">unsubscribe</a>.<br>
You are receiving this because you were mentioned.<img alt="" moz-do-not-send="true" width="1" height="1"><span>Message ID:
<span><openjdk/jdk/pull/19055/c2096564990</span><span>@</span><span>github</span><span>.</span><span>com></span></span></p>
</blockquote>
<br>
<br>
/*<br>
diff --git
a/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java
b/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java<br>
index 81d4fd259ed..c148dbd61b7 100644<br>
---
a/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java<br>
+++
b/src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java<br>
@@ -34,6 +34,7 @@<br>
import java.nio.file.Path;<br>
import java.nio.file.Paths;<br>
import java.nio.file.Files;<br>
+import java.util.Optional;<br>
<br>
import static java.nio.charset.StandardCharsets.UTF_8;<br>
<br>
@@ -46,8 +47,28 @@ public class VirtualMachineImpl extends
HotSpotVirtualMachine {<br>
// location is the same for all processes, otherwise the tools<br>
// will not be able to find all Hotspot processes.<br>
// Any changes to this needs to be synchronized with HotSpot.<br>
- private static final String tmpdir = "/tmp";<br>
+ private static final Path TMPDIR = Path.of("/tmp");<br>
+<br>
+ private static final Path PROC = Path.of("/proc");<br>
+ private static final Path NS_MNT = Path.of("ns/mnt");<br>
+ private static final Path SELF = PROC.resolve("self");<br>
+ private static final Path TMP = Path.of("tmp");<br>
+<br>
+ private static final Optional<Path> SELF_MNT_NS;<br>
+<br>
+ static {<br>
+ Path mountns = null;<br>
+ try {<br>
+ mountns = Files.readSymbolicLink(SELF.resolve(NS_MNT));<br>
+ } catch (IOException _) {<br>
+ // do nothing<br>
+ } finally {<br>
+ SELF_MNT_NS = Optional.ofNullable(mountns);<br>
+ }<br>
+ }<br>
+<br>
String socket_path;<br>
+<br>
/**<br>
* Attaches to the target VM<br>
*/<br>
@@ -235,24 +256,36 @@ private File createAttachFile(int pid, int
ns_pid) throws IOException {<br>
}<br>
<br>
private String findTargetProcessTmpDirectory(int pid, int
ns_pid) throws IOException {<br>
- String root;<br>
- if (pid != ns_pid) {<br>
+ final var procPid = PROC.resolve(Integer.toString(pid));<br>
+<br>
+ Optional<Path> tgtMountNS = Optional.empty();<br>
+<br>
+ try {<br>
+ tgtMountNS =
Optional.ofNullable(Files.readSymbolicLink(procPid.resolve(NS_MNT)));
// attempt to read the tgt's mnt ns id...<br>
+ } catch (IOException _) { // if we fail to read the tgt's
mnt ns id then we either dont have access or it no longer exists!<br>
+ if (!Files.exists(procPid)) throw new
IOException(String.format("unable to attach, %s non-existent!
process: %d terminated", procPid, pid));<br>
+<br>
+ // ok so if we get here we have failed to read the
tgt's mnt ns, but the tgt process still exists ... we do not have
privs to read its procfs<br>
+ }<br>
+<br>
+ final boolean sameMountNS = SELF_MNT_NS.isPresent()
&& SELF_MNT_NS.equals(tgtMountNS); // will be false if we
did not read the tgt's mnt ns<br>
+<br>
+ if (!sameMountNS || pid != ns_pid) {<br>
// A process may not exist in the same mount namespace
as the caller, e.g.<br>
// if we are trying to attach to a JVM process inside a
container.<br>
// Instead, attach relative to the target root
filesystem as exposed by<br>
// procfs regardless of namespaces.<br>
- String procRootDirectory = "/proc/" + pid + "/root";<br>
- if (!Files.isReadable(Path.of(procRootDirectory))) {<br>
- throw new IOException(<br>
- String.format("Unable to access root
directory %s " +<br>
- "of target process %d",
procRootDirectory, pid));<br>
+ final var procRootDirectory = procPid.resolve("root");<br>
+<br>
+ if (Files.isReadable(procRootDirectory))<br>
+ return procRootDirectory.resolve(TMP).toString();<br>
+ else if (Files.exists(procPid)) { // process is still
alive... so its a permissions issue... fallback but this may also
fail<br>
+ return TMPDIR.toString();<br>
+ } else {<br>
+ throw new IOException(String.format("Unable to
access root directory %s " + "of target process %d",
procRootDirectory.toString(), pid));<br>
}<br>
-<br>
- root = procRootDirectory + "/" + tmpdir;<br>
- } else {<br>
- root = tmpdir;<br>
- }<br>
- return root;<br>
+ } else<br>
+ return TMPDIR.toString(); // we are in the same mnt ns<br>
}<br>
<br>
/*<br>
<br>
</body>
</html>
--------------ZiY5EhYyDoaC0Fy7Oxy0DC0T--
-------------
PR Comment: https://git.openjdk.org/jdk/pull/19055#issuecomment-2096660046
More information about the serviceability-dev
mailing list