On Wed, 1 May 2024 17:30:05 GMT, Larry Cable <d...@openjdk.org> wrote:
>> src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java line >> 217: >> >>> 215: // Instead, attach relative to the target root filesystem as >>> exposed by >>> 216: // procfs regardless of namespaces. >>> 217: String root = "/proc/" + pid + "/root/" + tmpdir; >> >> Helping myself and other future readers understand this: the problem with >> the previous implementation is that the code _assumed_ that the tmpdir could >> be accessed this way (`/proc/<pid>/root/<tmpdir>`). In other words: >> >> * The code for creating the socket would correctly check if `pid != ns_pid` >> and then act accordingly (`/proc/<pid>/root/<tmpdir>` or just plain >> `<tmpdir>`) >> * The code for reading the socket would not have the check the above. It >> would resort to always use `/proc/<pid>/root/<tmpdir>`. >> * For certain scenarios (`CAP_NET_BIND_SERVICE`-processes, as described in >> https://github.com/openjdk/jdk/pull/17628#issuecomment-1916589081), we would >> get a `Permission denied` when trying to access the temporary directory like >> this. >> >> What this PR does is to ensure that the same `pid != ns_pid` check is used >> both when creating and reading the socket, and fall back to `<tmpdir>` when >> no namespacing is being used. This seems to work better for these processes >> with elevated permissions. > > should it not be comparing pid namespace ids and not pids? Do you mean that it should compare the input PID against the outermost (leftmost) PID in the `NSpid` list from `/proc/<pid>/status` and not innermost (rightmost) as is done right now? What would be the benefit of that? Or did you mean something else? I'm working on a fix for https://bugs.openjdk.org/browse/JDK-8327114 right now, and it occurred to me that there is a tiny risk of `pid != ns_pid` not evaluating to `true` even though the processes are in different PID namespaces (because two different PID namespaces can have the same PIDs). I think it could be mitigated by always trying `/proc/<pid>/root/tmp` first, and if it cannot be read, fall back to `/tmp`. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/17628#discussion_r1586563442
