Code Review fix for 8005044 remove crufty '_g' support from HS runtime code
Karen Kinnear
karen.kinnear at oracle.com
Wed Dec 19 08:36:56 PST 2012
Sounds like time to file a separate bug to track that issue down (particularly the bsd issue)
and move on with this one.
Thank you so much Dan for the history and details,
Karen
On Dec 19, 2012, at 11:30 AM, Daniel D. Daugherty wrote:
> Adding the other aliases back in...
>
> Harold,
>
> This question:
>
> > Another nit: Is it worth checking that all of
> > "/hotspot/libjvm.so" could get copied to buf? (i.e.: that
> > buflen-len > strlen("/hotspot/libjvm.so")
>
> is straight forward and the short answer is: yes, that
> code makes assumptions that "/hotspot/libjvm.so" will
> fit into 'buf'. It should not do that.
>
> It is Ron's call as to whether he wants to fix this existing
> bug as part of this work. It is easy enough to fix the obvious
> part of the bug, but... as is normal with HotSpot, there
> is more going on here than meets the eye...
>
> The slightly longer answer is that it probably doesn't
> matter because the path being created here is a fake.
> As long as the "/hotspot/l" part gets copied, then the
> callers of os::jvm_path() will be able to handle the
> fakery.
>
> Of course, there is an even longer answer. Read on if
> you want gory details (and a history lesson).
>
> This gamma launcher stuff is giving me an absolute headache.
> Unfortunately, there is a lot of history involved here...
>
> Here are the variations:
>
> src/os/bsd/vm/os_bsd.cpp:
>
> 1715 if (Arguments::created_by_gamma_launcher()) {
> 1716 // Support for the gamma launcher. Typical value for buf is
> 1717 // "<JAVA_HOME>/jre/lib/<arch>/<vmtype>/libjvm". If "/jre/lib/" appears at
> 1718 // the right place in the string, then assume we are installed in a JDK and
> 1719 // we're done. Otherwise, check for a JAVA_HOME environment variable and
> 1720 // construct a path to the JVM being overridden.
>
> <snip>
>
> 1762 // If the path exists within JAVA_HOME, add the JVM library name
> 1763 // to complete the path to JVM being overridden. Otherwise fallback
> 1764 // to the path to the current library.
> 1765 if (0 == access(buf, F_OK)) {
> 1766 // Use current module name "libjvm"
> 1767 len = strlen(buf);
> 1768 snprintf(buf + len, buflen-len, "/libjvm%s", JNI_LIB_SUFFIX);
> 1769 } else {
> 1770 // Fall back to path of current library
> 1771 rp = realpath(dli_fname, buf);
> 1772 if (rp == NULL)
> 1773 return;
> 1774 }
>
> src/os/linux/vm/os_linux.cpp:
>
> 2206 if (Arguments::created_by_gamma_launcher()) {
> 2207 // Support for the gamma launcher. Typical value for buf is
> 2208 // "<JAVA_HOME>/jre/lib/<arch>/<vmtype>/libjvm.so". If "/jre/lib/" appears at
> 2209 // the right place in the string, then assume we are installed in a JDK and
> 2210 // we're done. Otherwise, check for a JAVA_HOME environment variable and fix
> 2211 // up the path so it looks like libjvm.so is installed there (append a
> 2212 // fake suffix hotspot/libjvm.so).
>
> <snip>
>
> 2243 if (0 == access(buf, F_OK)) {
> 2244 // Use current module name "libjvm.so"
> 2245 len = strlen(buf);
> 2246 snprintf(buf + len, buflen-len, "/hotspot/libjvm.so");
> 2247 } else {
> 2248 // Go back to path of .so
> 2249 rp = realpath(dli_fname, buf);
> 2250 if (rp == NULL)
> 2251 return;
> 2252 }
>
> src/os/solaris/vm/os_solaris.cpp:
>
> 2496 if (Arguments::created_by_gamma_launcher()) {
> 2497 // Support for the gamma launcher. Typical value for buf is
> 2498 // "<JAVA_HOME>/jre/lib/<arch>/<vmtype>/libjvm.so". If "/jre/lib/" appears at
> 2499 // the right place in the string, then assume we are installed in a JDK and
> 2500 // we're done. Otherwise, check for a JAVA_HOME environment variable and fix
> 2501 // up the path so it looks like libjvm.so is installed there (append a
> 2502 // fake suffix hotspot/libjvm.so).
>
> <snip>
>
> 2539 if (0 == access(buf, F_OK)) {
> 2540 // Use current module name "libjvm.so"
> 2541 len = strlen(buf);
> 2542 snprintf(buf + len, buflen-len, "/hotspot/libjvm.so");
> 2543 } else {
> 2544 // Go back to path of .so
> 2545 realpath((char *)dlinfo.dli_fname, buf);
> 2546 }
>
> src/os/windows/vm/os_windows.cpp:
>
> 1732 buf[0] = '\0';
> 1733 if (Arguments::created_by_gamma_launcher()) {
> 1734 // Support for the gamma launcher. Check for an
> 1735 // JAVA_HOME environment variable
> 1736 // and fix up the path so it looks like
> 1737 // libjvm.so is installed there (append a fake suffix
> 1738 // hotspot/libjvm.so).
> 1739 char* java_home_var = ::getenv("JAVA_HOME");
> 1740 if (java_home_var != NULL && java_home_var[0] != 0) {
> 1741
> 1742 strncpy(buf, java_home_var, buflen);
> 1743
> 1744 // determine if this is a legacy image or modules image
> 1745 // modules image doesn't have "jre" subdirectory
> 1746 size_t len = strlen(buf);
> 1747 char* jrebin_p = buf + len;
> 1748 jio_snprintf(jrebin_p, buflen-len, "\\jre\\bin\\");
> 1749 if (0 != _access(buf, 0)) {
> 1750 jio_snprintf(jrebin_p, buflen-len, "\\bin\\");
> 1751 }
> 1752 len = strlen(buf);
> 1753 jio_snprintf(buf + len, buflen-len, "hotspot\\jvm.dll");
> 1754 }
> 1755 }
>
>
> Linux, Solaris and Windows all recognize that if they
> get to the point where they are appending a "hotspot/..."
> path to the jvm_path value, then they are making a fake
> path. Why are they making a fake path? Well, the gamma
> launcher lets someone use a libjvm.so/jvm.dll that is
> not located in the usual place in a JDK. And, you get
> to use that libjvm.so/jvm.dll WITH THAT JDK. Sounds
> pretty cool and it is pretty cool... until you realize
> that there is code in the VM that tries to find other
> libraries _relative_ to the libjvm.so/jvm.dll that is
> running. Ouch! So enter the above code blocks that fake
> a path to the libjvm.so/jvm.dll so that callers to
> os::jvm_path() don't get confused.
>
> To make matters more convoluted... yes, I know this is
> convoluted enough... The os_bsd.cpp doesn't seem to
> realize that it is creating a fake path here. At one
> point, the os_bsd.cpp code was a copy of os_linux.cpp
> code, but it was obviously tweaked. It will take more
> investigation to figure out the right course of action
> for os_bsd.cpp
>
> Dan
>
>
>
> On 12/19/12 7:51 AM, harold seigel wrote:
>> Hi Ron,
>>
>> I'm ok with using snprinft().
>>
>> Another nit: Is it worth checking that all of "/hotspot/libjvm.so" could get copied to buf? (i.e.: that buflen-len > strlen("/hotspot/libjvm.so")
>>
>> Thanks, Harold
>>
>> On 12/19/2012 9:17 AM, Ron Durbin wrote:
>>>
>>>
>>> Harold,
>>>
>>> Did you have any more points or counter points to make on the use of snprintf and strncpy ?
>>>
>>> Ron
>>> From: Ron Durbin
>>> Sent: Tuesday, December 18, 2012 4:50 PM
>>> To: Daniel Daugherty; hotspot-runtime-dev at openjdk.java.net; build-dev; serviceability-dev at openjdk.java.net
>>> Subject: RE: Code Review fix for 8005044 remove crufty '_g' support from HS runtime code
>>>
>>> Thanks for the quick reviews by all.
>>>
>>> I will agree with Dan on this one, sprint will stay.
>>> Ron
>>>
>>>
>>> From: Daniel D. Daugherty
>>> Sent: Tuesday, December 18, 2012 4:03 PM
>>> To: hotspot-runtime-dev at openjdk.java.net; build-dev; serviceability-dev at openjdk.java.net
>>> Subject: Re: Code Review fix for 8005044 remove crufty '_g' support from HS runtime code
>>>
>>> Sorry, I lost my manners somewhere... :-(
>>>
>>> Thanks for the fast review!
>>>
>>> Dan
>>>
>>> On 12/18/12 3:57 PM, Daniel D. Daugherty wrote:
>>> Adding back in the other aliases...
>>>
>>> Harold,
>>>
>>> The equivalent of:
>>>
>>> snprintf(buf, n, str);
>>>
>>> is:
>>>
>>> strncpy(buf, str, n - 1);
>>> buf[n - 1] = '\0';
>>>
>>> because snprintf() will only write out (n - 1) bytes from
>>> 'str' to 'buf' and then write a NUL in the last slot.
>>> strncpy() copies up to 'n' bytes from 'str' to 'buf'.
>>> strncpy() will not copy past a NUL character in 'str', but
>>> it will also not NUL terminate 'buf' if a NUL is not found
>>> in 'str' before the limit 'n' is reached.
>>>
>>> Dan
>>>
>>> On 12/18/12 2:48 PM, harold seigel wrote:
>>> Hi Ron,
>>>
>>> I think these calls to snprintf() in os_solaris.cpp and the other os_....cpp files could be replaced with calls to strncpy(). It might be a little simpler.
>>>
>>> Otherwise, it looks good.
>>>
>>> Thanks, Harold
>>>
>>> 2539 if (0 == access(buf, F_OK)) {
>>> 2540 // Use current module name "libjvm.so"
>>> 2541 len = strlen(buf);
>>> 2542 snprintf(buf + len, buflen-len, "/hotspot/libjvm.so");
>>> 2543 } else {
>>> 2544 // Go back to path of .so
>>> 2545 realpath((char *)dlinfo.dli_fname, buf); 2546 }
>>>
>>> On 12/18/2012 3:46 PM, Daniel D. Daugherty wrote:
>>> Greetings,
>>>
>>> I'm sponsoring this code review request from Ron Durbin. This change
>>> is targeted at JDK8/HSX-25 in the RT_Baseline repo.
>>>
>>> Dan
>>>
>>> Sending again with correct subject line, bug URLs and webrev URL.
>>>
>>>
>>> Intro:
>>>
>>> This set of changes removes the runtime support for generation of debug versions that follow _g semantics.
>>>
>>> Defect:
>>> JDK-8005044 remove crufty '_g' support from HS runtime code
>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8005044
>>> https://jbs.oracle.com/bugs/browse/JDK-8005044
>>>
>>>
>>> Webrev
>>> http://cr.openjdk.java.net/~dcubed/for_rdurbin/8005044-webrev/0
>>>
>>>
>>> Details:
>>> Files have been modified to remove all reference and support for debug versions that follow _g semantics.
>>>
>>> Testing:
>>>
>>> Passed JPRT last night:
>>>
>>> Additional Testing In process: (suggested by Dan):
>>>
>>> src/share/vm/runtime/arguments.cpp
>>> - test with shared archive creation and use; see the e-mail
>>> from Coleen
>>>
>>> src/share/tools/ProjectCreator/ProjectCreator.java
>>> - just a usage message; visual inspection of the code
>>>
>>> src/os/windows/vm/os_windows.cpp
>>> - comments only; no testing needed
>>>
>>> src/os/{bsd,linux,solaris}/vm/os_{bsd,linux,solaris}.cpp
>>> - the only code changes come into play when the "gamma"
>>> launcher is used
>>> - and when JAVA_HOME refers to a valid JDK, the function
>>> fakes up a JVM path so that callers using the JVM path
>>> to find other things in the JDK will work.
>>> - I can't find any way that the actual JVM path value
>>> that is returned is exposed
>>> - I don't see a way to test this other than have a debug
>>> printf() or manual code inspection.
>>>
>>>
>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20121219/d2f8c98f/attachment-0001.html
More information about the serviceability-dev
mailing list