On Mon, 6 May 2024 17:29:05 GMT, Sebastian Lövdahl <d...@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
