Code Review fix for 8005044 remove crufty '_g' support from HS runtime code
harold seigel
harold.seigel at oracle.com
Wed Dec 19 09:39:29 PST 2012
>> I say file the bug merely to document the issue and get this code
checked in
I agree.
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/d9015fd0/attachment-0001.html
More information about the hotspot-runtime-dev
mailing list