RFR: 8327114: Attach in Linux may have wrong behaviour when pid == ns_pid (Kubernetes debug container) [v2]

[Sebastian Lövdahl]

On Mon, 6 May 2024 18:31:06 GMT, Larry Cable <duke at openjdk.org> wrote:

>> 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 TMPD...

Thanks for all the deep thinking you're doing here @larry-cable, appreciated. And sorry for the delay in my response, I'll try to get more time devoted to this during the coming week.

> I did some thinking on this issue over the weekend and came up with an
> idea that *may* improve the probability of an attach succeeding in the
> case that the target has elevated privileges and the jcmd is not in the
> same mnt namespace as the target JVM.

I haven't fully digested the patches you have provided yet, but one question this far. In these cases, is it not a requirement that `jcmd` is run as `root`? So even if the target process is run with elevated privileges, attaching would always work. Or is there some way to attach from host to container with a non-`root` user that I'm missing?

One thing I would like to eventually achieve is to have [JDK-8226919](https://bugs.openjdk.org/browse/JDK-8226919) and this fix backported to the current LTS releases. Would it make more sense to fix [JDK-8327114](https://bugs.openjdk.org/browse/JDK-8327114) with as few changes as possible, and use that for backports, and do more extensive improvements in a separate follow-up that don't need backporting?

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

PR Comment: https://git.openjdk.org/jdk/pull/19055#issuecomment-2106339738