Code Review fix for 8005044 remove crufty '_g' support from HS runtime code
Daniel D. Daugherty
daniel.daugherty at oracle.com
Wed Dec 19 18:59:00 UTC 2012
Resending...adding back the other aliases...
Bug filed:
JDK-8005262possible gamma launcher issues
https://jbs.oracle.com/bugs/browse/JDK-8005262
Dan
On 12/19/12 11:18 AM, Coleen Phillimore wrote:
> On 12/19/2012 12:39 PM, harold seigel wrote:
>> >> I say file the bug merely to document the issue and get this code
>> checked in
>> I agree.
>
> Harold, Thank you for looking at this change closely.
> Dan, please file a bug about the fake path and include this writeup
> below, because I think I hit this bug with testing CDS with the gamma
> launcher and it might explain it.
> Thanks,
> Coleen
>
>>
>> Harold
>>
>> On 12/19/2012 12:11 PM, Ron Durbin wrote:
>>>
>>> Harold
>>>
>>> I would say first off this is not a bug that has manifested itself yet.
>>>
>>> So it is low priority.
>>>
>>> That being said this exposure is not introduced or made more risky
>>> by my changes.
>>>
>>> This is the only issue that we have against this code review and it
>>> does not belong to the changes being reviewed=.
>>>
>>> I say file the bug merely to document the issue and get this code
>>> checked in.
>>>
>>> Ron
>>>
>>> *From:*Daniel D. Daugherty
>>> *Sent:* Wednesday, December 19, 2012 9:30 AM
>>> *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
>>>
>>> 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\\
>>> <file:///%5C%5Cjre%5Cbin%5C>");
>>> 1749 if (0 != _access(buf, 0)) {
>>> 1750 jio_snprintf(jrebin_p, buflen-len, "\\bin\\
>>> <file:///%5C%5Cbin%5C>");
>>> 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
>>> <mailto:hotspot-runtime-dev at openjdk.java.net>; build-dev;
>>> serviceability-dev at openjdk.java.net
>>> <mailto: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
>>> <mailto:hotspot-runtime-dev at openjdk.java.net>; build-dev;
>>> serviceability-dev at openjdk.java.net
>>> <mailto: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
>>> <http://cr.openjdk.java.net/%7Edcubed/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.
>>>
>
More information about the build-dev
mailing list