Review for 8001185

[bill.pittore at oracle.com]

I think I addressed all the issues here. As to the one about 
NEW_C_HEAP_ARRAY and strcpy(), I'm not sure you wanted me to do anything 
or you were just noting the issue. That particular pattern is used in 
other places in the VM.
New webrev at http://cr.openjdk.java.net/~bpittore/8001185/webrev.01/

thanks,
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
>>