RFR(M): 8038201: Clean up misleading usage of malloc() in init_system_properties_values()
Vladimir Kozlov
vladimir.kozlov at oracle.com
Thu Mar 27 19:01:18 UTC 2014
On 3/27/14 11:53 AM, Dmitry Samersoff wrote:
> Goetz,
>
> Looks good for me!
+1.
Dmitry,
Do you want me to push it into Comp repo or you will do it into RT repo?
Thanks,
Vladimir
>
> -Dmitry
>
>
> On 2014-03-27 18:56, Lindenmaier, Goetz wrote:
>> Hi Dmitry,
>>
>> thanks for the thorough review and catching the forgotten FREEs!
>> I had a look at set_boot_path(). That will never return false,
>> as it also calls NEW_C_HEAP_ARRAY that will abort the VM.
>> In addition it would be wrong to return, there should be a
>> call to vm_exit_...
>> So I removed the test and return statement.
>> I added the other missing FREEs.
>>
>> I also implemented your proposal with only one sprintf
>> for the ld_library_path, and did so for bsd, aix and linux
>> to get it more similar. Solaris seems too different to
>> adapt to that scheme.
>>
>> http://cr.openjdk.java.net/~goetz/webrevs/8038201-sec/webrev.03/
>>
>> Best regards,
>> Goetz.
>>
>>
>>
>> -----Original Message-----
>> From: Dmitry Samersoff [mailto:dmitry.samersoff at oracle.com]
>> Sent: Donnerstag, 27. März 2014 10:49
>> To: Lindenmaier, Goetz; hotspot-dev at openjdk.java.net; ppc-aix-port-dev at openjdk.java.net
>> Subject: Re: RFR(M): 8038201: Clean up misleading usage of malloc() in init_system_properties_values()
>>
>> Goetz,
>>
>> Thank you for doing it - code is much cleaner now.
>>
>> os_aix.cpp:
>> Did you forget FREE_C_HEAP_ARRAY() at ll.548 and ll.576?
>>
>> os_bsd.cpp:
>> missed FREE_C_HEAP_ARRAY() at ll. 389, ll. 477
>>
>> ll.407 sprintf could be moved to ll.420 under else
>>
>>
>> ll.497 (and below) a trick like one below:
>>
>> char *l = ::getenv("JAVA_LIBRARY_PATH");
>> char *v = ::getenv("DYLD_LIBRARY_PATH");
>> char *v_colon = ":";
>>
>> if (l != NULL || v != NULL) {
>>
>> if (v == NULL){ v = "", v_colon = ""; };
>> if (l == NULL){ l = "", v_colon = ""; };
>>
>> t = (char *) ... /* allocate memory for all strings */
>>
>> sprintf(t, "%s%s%s:%s", v,v_colon, l, ld_library_path);
>> if (ld_library_path != buf){ ... /* free ld_library_path */ }
>> ld_library_path = t;
>> }
>>
>> might be useful to assemble the path in one shot,
>> without extra copying
>>
>>
>> ll.520 As you appending constant value ":.", you can just
>> take it into account at ll. 495
>>
>>
>> os_linux.cpp:
>> missed FREE_C_HEAP_ARRAY() at ll. 400
>>
>> os_solaris.cpp:
>> missed FREE_C_HEAP_ARRAY() at ll. 714
>>
>>
>> -Dmitry
>>
>> On 2014-03-27 12:54, Lindenmaier, Goetz wrote:
>>> Hi,
>>>
>>> please have a look at this new webrev:
>>> http://cr.openjdk.java.net/~goetz/webrevs/8038201-sec/webrev.02/
>>>
>>> Dmitry, sorry I forgot to reply to all in my last mail. I copied the missing parts in here, below.
>>>
>>> Avoiding big stack allocations I wanted to remove the buf[MAXPATHLEN]
>>> at the beginning of the function, too. That's why I said I can't get along
>>> with a single allocation. strlen(v) is only known later.
>>>
>>> I could redesign the whole code, computing sizes first, and then
>>> doing a single allocation, but I think that doesn't help the
>>> readability of the code.
>>>
>>> So now I compute the max size of all buffers I can estimate at the beginning
>>> and do a single allocation for them.
>>> Also, bsd contains an extra section for __APPLE__.
>>>
>>> Best regards,
>>> Goetz.
>>>
>>>
>>>
>>> Goetz,
>>>
>>>> The allocation in 556 must know about the length of the
>>>> getenv("LIBPATH") result, which may contain several paths.
>>>
>>> You can use
>>>
>>> MAX(
>>> strlen(v) + 1 + sizeof(DEFAULT_LIBPATH) + 1,
>>> MAXPATHLEN + sizeof(EXTENSIONS_DIR) + sizeof(ENDORSED_DIR)
>>> )
>>>
>>> as the size for a buffer at ll. 556 and have one allocation.
>>>
>>> But I'm fine with two different allocations.
>>>
>>> Thank you for addressing other concerns.
>>>
>>> -Dmitry
>>>
>>> On 2014-03-25 17:55, Lindenmaier, Goetz wrote:
>>>> Hi Dmitry,
>>>>
>>>> see my comments inline.
>>>>
>>>>> -----Original Message-----
>>>>> From: Dmitry Samersoff [mailto:dmitry.samersoff at oracle.com]
>>>>> Sent: Dienstag, 25. März 2014 11:12
>>>>> To: Lindenmaier, Goetz; hotspot-dev at openjdk.java.net; ppc-aix-port-dev at openjdk.java.net
>>>>> Subject: Re: RFR(M): 8038201: Clean up misleading usage of malloc() in init_system_properties_values()
>>>>>
>>>>> Goetz,
>>>>>
>>>>> os_aix.cpp: 565
>>>>>
>>>>> It might be better to allocate one buffer that satisfy all three
>>>>> sprintf() calls at ll. 556 and free it at ll. 574
>>>> I need at least two allocations. The first one has a statically known size. At
>>>> least more or less, as I know there will be only one path of unknown size in the buffer.
>>>> The allocation in 556 must know about the length of the getenv("LIBPATH") result,
>>>> which may contain several paths.
>>>> Also, in the other files, paths are concatenated, so at least two buffers are needed.
>>>>
>>>>> os_bsd.cpp: 350
>>>>>
>>>>> I would prefer to have the single #ifdef __APPLE__ block, it makes
>>>>> code much more readable.
>>>> I'll fix this.
>>>>
>>>>> os_bsd.cpp: 485
>>>>>
>>>>> you replace malloc to on-stack allocation of more than 4096 bytes. I
>>>>> think this allocation should be done through NEW_C_HEAP_ARRAY.
>>>> > see also comments to os_aix above - you can allocate large enough
>>>>> buffer once and than reuse it.
>>>> Is there a boundary at 4k? There was a char buf[MAXPATHLEN] in the
>>>> code anyways, so I thought a few bytes more would not matter.
>>>> The space required by the existing buf can be reused by the C compiler.
>>>>
>>>> But I'll fix that and only use allocated memory.
>>>> I'll remove the buf that was already there and replace all spaces
>>>> I can estimate by MAXPATHLEN + some offset by a single, allocated buffer.
>>>> I'll do this the same on all four platforms.
>>>>
>>>>> os_linux.cpp: 434
>>>>> the same comments as above.
>>>>>
>>>>> os_solaris.cpp: 728
>>>>>
>>>>> As soon as you doing cleanup, I would love to have this code changed
>>>>> a bit. It comes from solaris man page long time ago and doesn't respect
>>>>> hotspot convention of using underscore to indicate class global variables.
>>>>>
>>>>> Could you change it to something like:
>>>>> Dl_serinfo info_sz, *info;
>>>> Yes, I'll fix that.
>>>>
>>>> Best regards,
>>>> Goetz.
>>>>
>>>>
>>>>
>>>
>>>
>>> if (dlinfo(RTLD_SELF, RTLD_DI_SERINFOSIZE, (void *)&info_sz) == -1) {
>>> ...
>>>
>>> info = (Dl_serinfo*)NEW_C_HEAP_ARRAY(char, info_sz.dls_size, mtInternal);
>>> ...
>>>
>>> os_solaris.cpp: 829
>>> The same comment about on-stack allocation as for os_bsd.cpp
>>>
>>> -Dmitry
>>>
>>> On 2014-03-25 01:50, Lindenmaier, Goetz wrote:
>>>> Hi,
>>>>
>>>> please review and test this change. I please need a sponsor.
>>>> http://cr.openjdk.java.net/~goetz/webrevs/8038201-sec/webrev.00/
>>>>
>>>> This change addresses the implementation of init_system_properties_values
>>>> on aix, linux, solaris and bsd.
>>>> In init_system_properties_values a macro was defined mapping malloc to
>>>> NEW_C_HEAP_ARRAY. NEW_C_HEAP_ARRAY checks for successful allocation
>>>> or exits the VM. The code of this method handles the allocated pointers as
>>>> allocated with real malloc, i.e., there were checks for NULL and calls to free().
>>>>
>>>> This change replaces the macro malloc with NEW_C_HEAP_ARRAY and removes
>>>> the unnecessary checks making the code clearer. Also, it uses a local array where
>>>> possible.
>>>>
>>>> The allocated memory is passed to calls that end up at SystemProperty::set_value().
>>>> set_value copies the strings passed. Thus the memory allocated in
>>>> init_system_properties_values must be freed after these calls. Most of these
>>>> frees were missing.
>>>>
>>>> This change adds the missing frees.
>>>>
>>>> Testing this change I ran into a warning in ppc code which I fixed, too.
>>>> I did some local test. I'll get broader tests by Wednesday.
>>>>
>>>> Best regards,
>>>> Goetz.
>>>>
>>>>
>>>
>>>
>>
>>
>
>
More information about the ppc-aix-port-dev
mailing list