Review for 8001185

[David Holmes]

On 31/10/2012 6:12 AM, bill.pittore at oracle.com wrote:
> I think I addressed all the issues here. As to the one about

Yes - thanks.

> 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.

Ok. Just wanted to flag it.

David
-----

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