RFR(M): 8038201: Clean up misleading usage of malloc() in init_system_properties_values()
Vladimir Kozlov
vladimir.kozlov at oracle.com
Tue Apr 8 20:32:50 UTC 2014
Hi Dmitry,
Could you tell when you are planning to backport these changes into 8u20?
Thanks,
Vladimir
On 4/1/14 5:50 AM, Dmitry Samersoff wrote:
> Goetz,
>
> Push done.
>
> Let it bakes for some times in JDK9 (at least two nightlies) than I'll
> push it to 8u20
>
> -Dmitry
>
> On 2014-03-27 23:52, Lindenmaier, Goetz wrote:
>> Thanks Dmitry and Vladimir!
>>
>> Best regards,
>> Goetz.
>>
>> -----Original Message-----
>> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
>> Sent: Thursday, March 27, 2014 8:18 PM
>> To: Dmitry Samersoff; 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()
>>
>> Thank you, Dmitry
>>
>> We need to backport it into 8u20 too.
>>
>> Regards,
>> Vladimir
>>
>> On 3/27/14 12:16 PM, Dmitry Samersoff wrote:
>>> Vladimir,
>>>
>>> I can push it to RT next week.
>>>
>>> -Dmitry
>>>
>>> On 2014-03-27 23:01, Vladimir Kozlov wrote:
>>>> 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 hotspot-dev
mailing list