On Wed, 6 Apr 2022 12:19:32 GMT, Kevin Walls <kev...@openjdk.org> wrote:
>> jcmd uses >> src/jdk.internal.jvmstat/linux/classes/sun/jvmstat/PlatformSupportImpl.java >> to scan temporary directories to find out processes in the container. It >> checks inode to ensure the temp directory is not conflicted. However inode >> maybe same value between the container and others. Thus we should check >> device id for that case. >> >> For example I saw following case on [distroless >> cc-debian11](https://github.com/GoogleContainerTools/distroless/blob/main/cc/README.md) >> container. I started rescue:jdk19 container with sharing PID namespace. >> `/proc/1/root/tmp` is different from `/tmp` on rescue:jdk19, but they are >> same inode value. However we can see the differense in device id. >> >> >> $ podman run -it --rm --entrypoint=sh --pid=container:fa39662f7352 >> rescue:jdk19 >> / # >> / # stat /tmp >> File: /tmp >> Size: 29 Blocks: 0 IO Block: 4096 directory >> Device: efh/239d Inode: 135674931 Links: 1 >> Access: (1777/drwxrwxrwt) Uid: ( 0/ root) Gid: ( 0/ root) >> Access: 2022-04-05 08:51:37.000000000 >> Modify: 2022-04-05 08:51:37.000000000 >> Change: 2022-04-05 08:51:37.000000000 >> >> / # stat /proc/1/root/tmp >> File: /proc/1/root/tmp >> Size: 29 Blocks: 0 IO Block: 4096 directory >> Device: e1h/225d Inode: 135674931 Links: 1 >> Access: (1777/drwxrwxrwt) Uid: ( 0/ root) Gid: ( 0/ root) >> Access: 2022-04-05 08:51:37.000000000 >> Modify: 2022-04-05 08:50:42.000000000 >> Change: 2022-04-05 08:50:42.000000000 > > Hi, > So it's that the /tmp inode can be the same, between the host and a container. > You might like some text for the bug description as that could be clearer: > ----------- > > jcmd uses > src/jdk.internal.jvmstat/linux/classes/sun/jvmstat/PlatformSupportImpl.java > to scan for all temporary directories: the host temp dir, and (through the > /proc filesystem) the temp dirs of running containers. > > It checks the inode value to ensure a temp directory under /proc is not the > same as the host temp directory. > However the inode can have the same value in the container and host. Thus we > should check device id additionally. > > ----------- > > I think the changes look good. > > VM's temp directory (from os::get_temp_directory()) is a constant, i.e. /tmp, > in current implementations, so keeping the inode looks safe. > > Just being picky with the English, "isSameWithTemporaryDirectory" is odd (we > would say something is "the same as" rather than "with"). > It could be called "tempDirectoryEquals" ? 8-) > > Maybe the comment could also be clearer: > > 116 * Host and container devices could have the same inode value, > 117 * so we also need to check the device id. @kevinjwalls Thanks for your comment! I updated JBS and this PR. Could you review again? ------------- PR: https://git.openjdk.java.net/jdk/pull/8103
