Code Review fix for 8005044 remove crufty '_g' support from HS runtime code
Coleen Phillimore
coleen.phillimore at oracle.com
Wed Dec 19 10:18:31 PST 2012
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.
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20121219/d78c0d66/attachment-0001.html
More information about the hotspot-runtime-dev
mailing list