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

Wed Feb 1 22:04:24 PST 2012

The problem is only with comments. Staffan and I discussed
this off thread.

Dan

On 2/1/12 9:15 PM, David Holmes wrote:
> 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
>>>>>>
>>>>