Request for Review: 7132199: sun/management/jmxremote/bootstrap/JvmstatCountersTest.java failing on all platforms

[David Holmes]

This fix has been pushed but Dan's comments seem to indicate there is a 
problem. I confess I can't keep track of which functions are creating 
and/or looking for which file in which location and which VM is doing 
the creating/looking.

David

On 1/02/2012 1:21 AM, Daniel D. Daugherty wrote:
> Summary: The .java_pid socket/door is always created in the
> os::get_temp_directory() which is "/tmp" on BSD, Linux and
> Solaris. On MacOS X, it can be either the "secure per-user
> temporary directory" or "/tmp".
>
> The .attach_pid file is created by the Java side
> createAttachFile(). That code first tries to create the
> file in the process' working directory and if that fails,
> then it attempts to create the file in tmpdir.
>
> src/solaris/classes/sun/tools/attach/LinuxVirtualMachine.java
> lines 40-44: The new comment is right for .java_pid, but
> not quite right for .attach_pid. See createAttachFile()
> method on line 280; .attach_pid creation is first
> attempted in the Java process' current working dir
> and then in tmpdir.
>
> src/solaris/classes/sun/tools/attach/SolarisVirtualMachine.java
> lines 41-45: The new comment is right for .java_pid, but
> not quite right for .attach_pid. See createAttachFile()
> method on line 215; .attach_pid creation is first
> attempted in the Java process' current working dir
> and then in tmpdir.
>
> test/ProblemList.txt
> No comments.
>
>
> Linux HSX gory details:
>
> The following block of code only creates the .java_pid socket in the
> directory named by os::get_temp_directory(). On Linux, that function
> is hardcoded to return "/tmp".
>
> src/os/linux/vm/attachListener_linux.cpp:
>
> 170 // Initialization - create a listener socket and bind it to a file
> 171
> 172 int LinuxAttachListener::init() {
> 173 char path[UNIX_PATH_MAX]; // socket file
> 174 char initial_path[UNIX_PATH_MAX]; // socket file during setup
> 175 int listener; // listener socket (file descriptor
> )
> 176
> 177 // register function to cleanup
> 178 ::atexit(listener_cleanup);
> 179
> 180 int n = snprintf(path, UNIX_PATH_MAX, "%s/.java_pid%d",
> 181 os::get_temp_directory(), os::current_process_id());
> 182 if (n < (int)UNIX_PATH_MAX) {
> 183 n = snprintf(initial_path, UNIX_PATH_MAX, "%s.tmp", path);
> 184 }
> 185 if (n >= (int)UNIX_PATH_MAX) {
> 186 return -1;
> 187 }
>
> This block of code checks the process' current directory for an .attach_pid
> file before it checks os::get_temp_directory(). However, that check is
> just to see if the attach mechanism should be started. If it does get
> spoofed, there is little harm.
>
> 464 // If the file .attach_pid<pid> exists in the working directory
> 465 // or /tmp then this is the trigger to start the attach mechanism
> 466 bool AttachListener::is_init_trigger() {
> 467 if (init_at_startup() || is_initialized()) {
> 468 return false; // initialized at startup or already ini
> tialized
> 469 }
> 470 char fn[PATH_MAX+1];
> 471 sprintf(fn, ".attach_pid%d", os::current_process_id());
> 472 int ret;
> 473 struct stat64 st;
> 474 RESTARTABLE(::stat64(fn, &st), ret);
> 475 if (ret == -1) {
> 476 snprintf(fn, sizeof(fn), "%s/.attach_pid%d",
> 477 os::get_temp_directory(), os::current_process_id());
> 478 RESTARTABLE(::stat64(fn, &st), ret);
> 479 }
> 480 if (ret == 0) {
> 481 // simple check to avoid starting the attach mechanism when
> 482 // a bogus user creates the file
> 483 if (st.st_uid == geteuid()) {
> 484 init();
> 485 return true;
> 486 }
> 487 }
> 488 return false;
> 489 }
>
>
> Solaris HSX gory details:
>
> This code assumes both files are always in /tmp/...:
>
> src/os/solaris/dtrace/jvm_dtrace.c
>
> 170 #define ATTACH_FILE_PATTERN "/tmp/.attach_pid%d"
> 171
> 172 /* fill-in the name of attach file name in given buffer */
> 173 static void fill_attach_file_name(char* path, int len, pid_t pid) {
> 174 memset(path, 0, len);
> 175 sprintf(path, ATTACH_FILE_PATTERN, pid);
> 176 }
> 177
> 178 #define DOOR_FILE_PATTERN "/tmp/.java_pid%d"
> 179
> 180 /* open door file for the given JVM */
> 181 static int open_door(pid_t pid) {
> 182 char path[PATH_MAX + 1];
> 183 int fd;
> 184
> 185 sprintf(path, DOOR_FILE_PATTERN, pid);
> 186 fd = file_open(path, O_RDONLY);
> 187 if (fd < 0) {
> 188 set_jvm_error(JVM_ERR_CANT_OPEN_DOOR);
> 189 print_debug("cannot open door file %s\n", path);
> 190 return -1;
> 191 }
> 192 print_debug("opened door file %s\n", path);
> 193 if (check_permission(path) != 0) {
> 194 set_jvm_error(JVM_ERR_DOOR_FILE_PERMISSION);
> 195 print_debug("check permission failed for %s\n", path);
> 196 file_close(fd);
> 197 fd = -1;
> 198 }
> 199 return fd;
> 200 }
> 201
> 202 /* create attach file for given process */
> 203 static int create_attach_file(pid_t pid) {
> 204 char path[PATH_MAX + 1];
> 205 int fd;
> 206 fill_attach_file_name(path, sizeof(path), pid);
> 207 fd = file_open(path, O_CREAT | O_RDWR);
> 208 if (fd < 0) {
> 209 set_jvm_error(JVM_ERR_CANT_CREATE_ATTACH_FILE);
> 210 print_debug("cannot create file %s\n", path);
> 211 } else {
> 212 print_debug("created attach file %s\n", path);
> 213 }
> 214 return fd;
> 215 }
>
> The following block of code only creates the .java_pid door in the
> directory named by os::get_temp_directory(). On Solaris, that function
> is hardcoded to return "/tmp".
>
> src/os/solaris/vm/attachListener_solaris.cpp
>
> 367 // Create the door
> 368 int SolarisAttachListener::create_door() {
> 369 char door_path[PATH_MAX+1];
> 370 char initial_path[PATH_MAX+1];
> 371 int fd, res;
> 372
> 373 // register exit function
> 374 ::atexit(listener_cleanup);
> 375
> 376 // create the door descriptor
> 377 int dd = ::door_create(enqueue_proc, NULL, 0);
> 378 if (dd < 0) {
> 379 return -1;
> 380 }
> 381
> 382 // create initial file to attach door descriptor
> 383 snprintf(door_path, sizeof(door_path), "%s/.java_pid%d",
> 384 os::get_temp_directory(), os::current_process_id());
> 385 snprintf(initial_path, sizeof(initial_path), "%s.tmp", door_path);
> 386 RESTARTABLE(::creat(initial_path, S_IRUSR | S_IWUSR), fd);
> 387 if (fd == -1) {
> 388 debug_only(warning("attempt to create %s failed", initial_path));
> 389 ::door_revoke(dd);
> 390 return -1;
> 391 }
> 392 assert(fd >= 0, "bad file descriptor");
> 393 RESTARTABLE(::close(fd), res);
>
> This block of code checks the process' current directory for an .attach_pid
> file before it checks os::get_temp_directory(). However, that check is
> just to see if the attach mechanism should be started. If it does get
> spoofed, there is little harm.
>
> 603 // If the file .attach_pid<pid> exists in the working directory
> 604 // or /tmp then this is the trigger to start the attach mechanism
> 605 bool AttachListener::is_init_trigger() {
> 606 if (init_at_startup() || is_initialized()) {
> 607 return false; // initialized at startup or already ini
> tialized
> 608 }
> 609 char fn[PATH_MAX+1];
> 610 sprintf(fn, ".attach_pid%d", os::current_process_id());
> 611 int ret;
> 612 struct stat64 st;
> 613 RESTARTABLE(::stat64(fn, &st), ret);
> 614 if (ret == -1) {
> 615 snprintf(fn, sizeof(fn), "%s/.attach_pid%d",
> 616 os::get_temp_directory(), os::current_process_id());
> 617 RESTARTABLE(::stat64(fn, &st), ret);
> 618 }
> 619 if (ret == 0) {
> 620 // simple check to avoid starting the attach mechanism when
> 621 // a bogus user creates the file
> 622 if (st.st_uid == geteuid()) {
> 623 init();
> 624 return true;
> 625 }
> 626 }
> 627 return false;
> 628 }
>
>
> BSD/MacOS X HSX gory details:
>
> This code assumes both files are always in /tmp/...:
>
> src/os/bsd/dtrace/jvm_dtrace.c
>
> 170 #define ATTACH_FILE_PATTERN "/tmp/.attach_pid%d"
> 171
> 172 /* fill-in the name of attach file name in given buffer */
> 173 static void fill_attach_file_name(char* path, int len, pid_t pid) {
> 174 memset(path, 0, len);
> 175 sprintf(path, ATTACH_FILE_PATTERN, pid);
> 176 }
> 177
> 178 #define DOOR_FILE_PATTERN "/tmp/.java_pid%d"
> 179
> 180 /* open door file for the given JVM */
> 181 static int open_door(pid_t pid) {
> 182 char path[PATH_MAX + 1];
> 183 int fd;
> 184
> 185 sprintf(path, DOOR_FILE_PATTERN, pid);
> 186 fd = file_open(path, O_RDONLY);
> 187 if (fd < 0) {
> 188 set_jvm_error(JVM_ERR_CANT_OPEN_DOOR);
> 189 print_debug("cannot open door file %s\n", path);
> 190 return -1;
> 191 }
> 192 print_debug("opened door file %s\n", path);
> 193 if (check_permission(path) != 0) {
> 194 set_jvm_error(JVM_ERR_DOOR_FILE_PERMISSION);
> 195 print_debug("check permission failed for %s\n", path);
> 196 file_close(fd);
> 197 fd = -1;
> 198 }
> 199 return fd;
> 200 }
> 201
> 202 /* create attach file for given process */
> 203 static int create_attach_file(pid_t pid) {
> 204 char path[PATH_MAX + 1];
> 205 int fd;
> 206 fill_attach_file_name(path, sizeof(path), pid);
> 207 fd = file_open(path, O_CREAT | O_RDWR);
> 208 if (fd < 0) {
> 209 set_jvm_error(JVM_ERR_CANT_CREATE_ATTACH_FILE);
> 210 print_debug("cannot create file %s\n", path);
> 211 } else {
> 212 print_debug("created attach file %s\n", path);
> 213 }
> 214 return fd;
> 215 }
>
> The following block of code only creates the .java_pid socket in the
> directory named by os::get_temp_directory(). On BSD, that function
> is hardcoded to return "/tmp". On MacOS X, that function returns
> the path of the "secure per-user temporary directory" if one is
> configured or "/tmp" if one is not.
>
> src/os/bsd/vm/attachListener_bsd.cpp
>
> 170 // Initialization - create a listener socket and bind it to a file
> 171
> 172 int BsdAttachListener::init() {
> 173 char path[UNIX_PATH_MAX]; // socket file
> 174 char initial_path[UNIX_PATH_MAX]; // socket file during setup
> 175 int listener; // listener socket (file descriptor
> )
> 176
> 177 // register function to cleanup
> 178 ::atexit(listener_cleanup);
> 179
> 180 int n = snprintf(path, UNIX_PATH_MAX, "%s/.java_pid%d",
> 181 os::get_temp_directory(), os::current_process_id());
> 182 if (n < (int)UNIX_PATH_MAX) {
> 183 n = snprintf(initial_path, UNIX_PATH_MAX, "%s.tmp", path);
> 184 }
> 185 if (n >= (int)UNIX_PATH_MAX) {
> 186 return -1;
> 187 }
>
> This block of code checks the process' current directory for an .attach_pid
> file before it checks os::get_temp_directory(). However, that check is
> just to see if the attach mechanism should be started. If it does get
> spoofed, there is little harm.
>
> 476 // If the file .attach_pid<pid> exists in the working directory
> 477 // or /tmp then this is the trigger to start the attach mechanism
> 478 bool AttachListener::is_init_trigger() {
> 479 if (init_at_startup() || is_initialized()) {
> 480 return false; // initialized at startup or already ini
> tialized
> 481 }
> 482 char path[PATH_MAX + 1];
> 483 int ret;
> 484 struct stat st;
> 485
> 486 snprintf(path, PATH_MAX + 1, "%s/.attach_pid%d",
> 487 os::get_temp_directory(), os::current_process_id());
> 488 RESTARTABLE(::stat(path, &st), ret);
> 489 if (ret == 0) {
> 490 // simple check to avoid starting the attach mechanism when
> 491 // a bogus user creates the file
> 492 if (st.st_uid == geteuid()) {
> 493 init();
> 494 return true;
> 495 }
> 496 }
> 497 return false;
> 498 }
>
>
>
> On 1/31/12 7:32 AM, Daniel D. Daugherty wrote:
>> Sorry, sent that last one as "reply to list" instead of "reply to all"...
>>
>> I think you need to hold up on this one. I don't think the
>> assumption that .java_pid and .attach_pid files are always
>> in /tmp is a good one.
>>
>> More in a little bit.
>>
>> Dan
>>
>> On 1/31/12 1:10 AM, Staffan Larsen wrote:
>>> Moving "/tmp" to a static field, made it easier to write a comment
>>> explaining the rationale as well.
>>>
>>> Updated webrev: http://cr.openjdk.java.net/~sla/7132199/webrev.02/
>>> <http://cr.openjdk.java.net/%7Esla/7132199/webrev.02/>
>>>
>>> Thanks,
>>> /Staffan
>>>
>>> On 31 jan 2012, at 01:23, David Holmes wrote:
>>>
>>>> On 31/01/2012 3:28 AM, Dmitry Samersoff wrote:
>>>>> On 2012-01-30 16:28, Staffan Larsen wrote:
>>>>>>> 2. If you decide to hardcode "/tmp" please, create a global constant
>>>>>>> for it.
>>>>>>
>>>>>> I don't agree that this would make the code easier to read or
>>>>>> maintain.
>>>>>> I should, however, include a comment saying that the file is
>>>>>> always in
>>>>>> /tmp regardless of the value of java.io.tmpdir.
>>>>
>>>> Staffan: I still think changing the static field tmpdir to refer to
>>>> "/tmp" is cleaner then putting "/tmp" in all the use-sites.
>>>>
>>>>> /tmp is common but not mandatory, especially if we speak about
>>>>> embedded
>>>>> systems.
>>>>
>>>> Dmitry: The point is that the VM will always put the file in /tmp.
>>>> That's wrong but the issue here is making the management Java code
>>>> match the hotspot code.
>>>>
>>>>> Native code should use P_tmpdir constant from stdio.h rather than
>>>>> hardcode "/tmp".
>>>>>
>>>>> As we can't access it from java I recommend to create a global
>>>>> constant
>>>>> somewhere to reduce possible future porting efforts.
>>>>>
>>>>>
>>>>>> Changing the tmpdir static would be a smaller fix, but all the cwd
>>>>>> code
>>>>>> would then remain. Yes, HotSpot never writes to cwd.
>>>>>
>>>>> I agree with Staffan, that looks for socket/door in cwd should be
>>>>> removed.
>>>>
>>>> Ok, if it is never needed then remove it.
>>>>
>>>> David
>>>>
>>>>> -Dmitry
>>>>>
>>>