Review for 8001185

[bill.pittore at oracle.com]

Thanks. I'll fix those indents and the comment.

bill



On 10/27/2012 3:38 AM, David Holmes wrote:
> 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
>>