Hi Bill,
A few minor comments.
thread.cpp:
3680 // Try to load the agent from the standard dll directory
3681 if (os::dll_build_name(buffer, sizeof(buffer),
Arguments::get_dll_dir(),
3682 name)) {
3683 library = os::dll_load(buffer, ebuf, sizeof ebuf);
3684 }
The indentation of the if-block is wrong. Same here:
3709 if (os::dll_build_name(buffer, sizeof(buffer), ns, name)) {
3710 library = os::dll_load(buffer, ebuf, sizeof ebuf);
3711 }
hotspot uses 2-space indent (mostly). The indentation problem exists in
all the files where you added if-blocks.
---
os.hpp
// Builds a platform-specific full library path given a ld path and
lib name
! static bool dll_build_name(char* buffer, size_t size,
The comment should indicate what the bool return means.
---
os.cpp
1167 strcpy(inpath, path);
I know inpath was created to fit path, so the change to use strcpy
instead of strncpy is safe, but it might get flagged by a static
analysis tool that doesn't understand NEW_C_HEAP_ARRAY. Just to be clear
the bug in the original code is that:
strncpy(inpath, path, strlen(path));
should be
strncpy(inpath, path, strlen(path)+1);
---
os_windows.cpp
1157 // Quietly truncates on buffer overflow. Should be an error.
1158 if (pnamelen + strlen(fname) + 10 > buflen) {
1159 return retval;
The comment needs updating as you no longer truncate the buffer. Same
applies to all the other os variants.
Cheers,
David
On 27/10/2012 2:21 AM, BILL PITTORE wrote:
> This bug deals with parsing sun.boot.library.path (dll_dir) when it has
> multiple paths. I found these problems while fixing JDK-7200297. The
> latter bug showed up on an embedded platform that does not have the
> ability to set LD_LIBRARY_PATH for shared library searches.
> If you add -Dsun.boot.library.path=<dir> to command line you end up with
> dll_dir with multiple paths. When -agentlib option is used, code in
> os_<platform>.cpp splits out the individual paths and attempts to find
> the library in those paths. First problem is in os::split_path() where
> the incoming path is copied into a malloced buffer. Terminating null is
> not added. Result of this is that last path in list of paths will not
> get searched correctly (in os::dll_build_name()) since the snprintf will
> copy random amount of garbage after pathname. I supposes it's possible
> that reading way past end of malloc'd path could result in page fault if
> malloc'd memory at end of mapped memory block.
> Another problem is that code in dll_build_name() leaves last full path
> attempted in the buffer that is returned to caller. Caller will then try
> to load that file even if dll_build_name() did not find the file. So one
> extra (possibly failing) load attempt.
> Next problem is that dll_build_name could return NULL string (*buffer ==
> '\0') if buffer too small or previous bug fixed. Caller should check for
> that otherwise (on linux/solaris) calling dlopen() with null string
> returns handle of executable; not what you're expecting. Changed the API
> for dll_build_name to return a bool so it is easy for caller to test
> whether or not returned buffer is valid.
>
> Webrev is at: http://cr.openjdk.java.net/~bpittore/8001185/webrev.00/
>
> thanks,
> bill
>