On Wed, 18 Jun 2025 16:40:53 GMT, Sergey Chernyshev <[email protected]> wrote:
>> Hi all, >> >> I would like to propose a fix for JDK-8319589. This will allow jcmd and jps >> running as root to get the complete list of JVMs running by all users, and >> to attach from root to non-root JVMs. Previously, JDK-8197387 introduced the >> same approach on Linux. >> >> This change affects macOS, that uses "secure" per-user temporary >> directories. It only affects JVMs running as root, the behavior in >> non-privileged JVMs remains unchanged. >> >> Jcmd and jps rely on LocalVmManager to get the initial list of the local >> VMs. The LocalVmManager uses sun.jvmstat.PlatformSupport to get the list of >> temp directories, where it searches for user's PerfData directory such as >> "hsperfdata_<username\>". In macosx the temp directories are per-user, the >> temp path is returned by confstr(_CS_DARWIN_USER_TEMP_DIR). The per-user >> directories are mode 700 and so they are read-protected from non-privileged >> users and can be accessed by the owner and the root. >> >> Both jps and jcmd (HotSpotAttachProvider) create MonitoredVm objects, that >> have PerfDataBuffer that performs attachment to the target. Only the >> attachable VMs are listed in jcmd output. >> >> The proposed patch changes the list of directories returned by the >> PlatformSupport#getTemporaryDirectories() in VMs running as root. The list >> is later used in VirtualMachineImpl (jdk.attach). It changes also the way >> mmap_attach_shared() searches for hsperfdata_<username\>/<pid\> files to map >> the shared memory. Mmap_attach_shared() and VirtualMachineImpl (via >> PlatformSupport) list the content of /var/folders, where the temp >> directories are located, more specificly the temp directories are >> /var/folders/<BUCKET\>/<ENCODED_UUID_UID\>/T as hinted in [1]. The full list >> is returned by newly added PlatformSupportImpl#getTemporaryDirectories(). >> >> The attaching client's VirtualMachineImpl needs the target process's temp >> directory to find .java<pid\> and create .attach<pid\> files. It uses the >> list returned by PlatformSupportImpl#getTemporaryDirectories() and the >> ProcessHandle of the target process to search for user's PerfData directory, >> e.g. hsperfdata_<username\>, which is in the target process's temp >> directory, exactly where it expects to see the .java<pid\> in return on >> sending SIGQUIT to the target VM. >> >> Mmap_attach_shared() traverses the /var/folders in get_user_tmp_dir() and >> looks for a hsperfdata_<username\> folder. If that folder is found in >> /var/folders/*/*/T, that means the temp folder corresponds to the >> <username\> and to the ... > > Sergey Chernyshev has updated the pull request incrementally with two > additional commits since the last revision: > > - Update > src/jdk.attach/macosx/classes/sun/tools/attach/VirtualMachineImpl.java > > Co-authored-by: Andrey Turbanov <[email protected]> > - addressed review comments This seems reasonable but I am far from expert in this area of macOS, nor in the Java side of this code. Serviceability need to take a good look at this. I few minor nits below. Thanks src/hotspot/os/bsd/os_bsd.cpp line 893: > 891: // > 892: static const char VAR_FOLDERS[] = "/var/folders/"; > 893: int os::Bsd::get_user_tmp_dir_macos(const char* user, int vmid, char > *output_path, int output_size /* = PATH_MAX */) { Suggestion: int os::Bsd::get_user_tmp_dir_macos(const char* user, int vmid, char* output_path, int output_size) { src/hotspot/os/bsd/os_bsd.cpp line 912: > 910: // absolute path to the bucket > 911: char bucket[PATH_MAX]; > 912: int b = snprintf(bucket, PATH_MAX, "%s%s/", VAR_FOLDERS, > bucket_de->d_name); Suggestion: int b = os::snprintf(bucket, PATH_MAX, "%s%s/", VAR_FOLDERS, bucket_de->d_name); Just for clarity src/hotspot/os/bsd/os_bsd.cpp line 936: > 934: // > 935: char perfdata_path[PATH_MAX]; > 936: int p = snprintf(perfdata_path, PATH_MAX, "%s%s/T/%s_%s/", > bucket, subbucket_de->d_name, PERFDATA_NAME, user); Suggestion: int p = os::snprintf(perfdata_path, PATH_MAX, "%s%s/T/%s_%s/", bucket, subbucket_de->d_name, PERFDATA_NAME, user); src/hotspot/os/bsd/os_bsd.cpp line 947: > 945: > 946: // the return value of snprintf is not checked for the second > time > 947: return snprintf(output_path, output_size, "%s%s/T", bucket, > subbucket_de->d_name); Suggestion: return os::snprintf(output_path, output_size, "%s%s/T", bucket, subbucket_de->d_name); src/hotspot/os/bsd/os_bsd.hpp line 68: > 66: > 67: #ifdef __APPLE__ > 68: static int get_user_tmp_dir_macos(const char* user, int vmid, char > *output_buffer, int buffer_size /* = PATH_MAX */); Suggestion: static int get_user_tmp_dir_macos(const char* user, int vmid, char* output_buffer, int buffer_size); The buffer size is up to the caller. src/hotspot/os/posix/perfMemory_posix.cpp line 1180: > 1178: int nspid = LINUX_ONLY(os::Linux::get_namespace_pid(vmid)) > NOT_LINUX(-1); > 1179: const char* luser = NOT_MACOS(get_user_name(vmid, &nspid, CHECK)) > 1180: MACOS_ONLY(get_user_name(os::Bsd::get_process_uid(vmid))); Suggestion: const char* luser = NOT_MACOS(get_user_name(vmid, &nspid, CHECK)) MACOS_ONLY(get_user_name(os::Bsd::get_process_uid(vmid))); src/jdk.attach/macosx/classes/sun/tools/attach/VirtualMachineImpl.java line 31: > 29: import com.sun.tools.attach.spi.AttachProvider; > 30: > 31: import sun.jvmstat.PlatformSupport; This is convenient but I'm not sure it is appropriate. Need the serviceability folk to comment. src/jdk.attach/macosx/classes/sun/tools/attach/VirtualMachineImpl.java line 49: > 47: // the latter can be changed by the user. > 48: // Any changes to this needs to be synchronized with HotSpot. > 49: private static final String tmpdir; So we can't cache this any more? ------------- PR Review: https://git.openjdk.org/jdk/pull/25824#pullrequestreview-3255835341 PR Review Comment: https://git.openjdk.org/jdk/pull/25824#discussion_r2370839037 PR Review Comment: https://git.openjdk.org/jdk/pull/25824#discussion_r2370840143 PR Review Comment: https://git.openjdk.org/jdk/pull/25824#discussion_r2370841710 PR Review Comment: https://git.openjdk.org/jdk/pull/25824#discussion_r2370842039 PR Review Comment: https://git.openjdk.org/jdk/pull/25824#discussion_r2370844530 PR Review Comment: https://git.openjdk.org/jdk/pull/25824#discussion_r2370846681 PR Review Comment: https://git.openjdk.org/jdk/pull/25824#discussion_r2370851193 PR Review Comment: https://git.openjdk.org/jdk/pull/25824#discussion_r2370853656
